[PATCH v2 2/2] ari, btrace: avoid unsigned long long

[PATCH v2 2/2] ari, btrace: avoid unsigned long long

[Markus Metzger]

Fix the ARI warning about the use of unsigned long long.  We can't use ULONGEST
as this is defined unsigned long on 64-bit systems.  Use unsigned long to hold
the buffer size inside GDB and __u64 when interfacing the kernel.

2015-07-09  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* nat/linux-btrace.c (perf_event_read): Change the type of DATA_HEAD.
	(perf_event_read_all): Change the type of SIZE and DATA_HEAD.
	(perf_event_read_bts): Change the type of SIZE and READ.
	(linux_enable_bts): Change the type of SIZE, PAGES, DATA_SIZE,
	and DATA_OFFSET.  Move DATA_SIZE declaration.  Restrict the buffer size
	to UINT_MAX.  Check for overflows when using DATA_HEAD from the perf
	mmap page.
	(linux_enable_pt): Change the type of PAGES and SIZE.  Restrict the
	buffer size to UINT_MAX.
	(linux_read_bts): Change the type of BUFFER_SIZE, SIZE, DATA_HEAD, and
	DATA_TAIL.
	* nat/linux-btrace.c (struct perf_event_buffer)<size, data_head>
	<last_head>: Change type.
---
 gdb/nat/linux-btrace.c | 70 ++++++++++++++++++++++++++++++++------------------
 gdb/nat/linux-btrace.h |  6 ++---
 2 files changed, 48 insertions(+), 28 deletions(-)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b6e13d3..cc1e5a5 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -111,12 +111,13 @@ perf_event_new_data (const struct perf_event_buffer *pev)
    The caller is responsible for freeing the memory.  */
 
 static gdb_byte *
-perf_event_read (const struct perf_event_buffer *pev, unsigned long data_head,
+perf_event_read (const struct perf_event_buffer *pev, __u64 data_head,
 		 unsigned long size)
 {
   const gdb_byte *begin, *end, *start, *stop;
   gdb_byte *buffer;
-  unsigned long data_tail, buffer_size;
+  unsigned long buffer_size;
+  __u64 data_tail;
 
   if (size == 0)
     return NULL;
@@ -151,13 +152,14 @@ static void
 perf_event_read_all (struct perf_event_buffer *pev, gdb_byte **data,
 		     unsigned long *psize)
 {
-  unsigned long data_head, size;
+  unsigned long size;
+  __u64 data_head;
 
   data_head = *pev->data_head;
 
   size = pev->size;
   if (data_head < size)
-    size = data_head;
+    size = (unsigned long) data_head;
 
   *data = perf_event_read (pev, data_head, size);
   *psize = size;
@@ -270,11 +272,11 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
 static VEC (btrace_block_s) *
 perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
 		     const uint8_t *end, const uint8_t *start,
-		     unsigned long long size)
+		     unsigned long size)
 {
   VEC (btrace_block_s) *btrace = NULL;
   struct perf_event_sample sample;
-  unsigned long long read = 0;
+  unsigned long read = 0;
   struct btrace_block block = { 0, 0 };
   struct regcache *regcache;
 
@@ -642,7 +644,8 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   struct perf_event_mmap_page *header;
   struct btrace_target_info *tinfo;
   struct btrace_tinfo_bts *bts;
-  unsigned long long size, pages, data_offset, data_size;
+  unsigned long size, pages;
+  __u64 data_offset;
   int pid, pg;
 
   tinfo = xzalloc (sizeof (*tinfo));
@@ -674,7 +677,7 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
     goto err_out;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
+  pages = (((unsigned long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -692,12 +695,13 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
       size_t length;
 
       size = pages * PAGE_SIZE;
-      length = size + PAGE_SIZE;
 
-      /* Check for overflows.  */
-      if ((unsigned long long) length < size)
+      /* Don't ask for more than we can represent in the configuration.  */
+      if (UINT_MAX < size)
 	continue;
 
+      length = size + PAGE_SIZE;
+
       /* The number of pages we request needs to be a power of two.  */
       header = mmap (NULL, length, PROT_READ, MAP_SHARED, bts->file, 0);
       if (header != MAP_FAILED)
@@ -708,23 +712,33 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
     goto err_file;
 
   data_offset = PAGE_SIZE;
-  data_size = size;
 
 #if defined (PERF_ATTR_SIZE_VER5)
   if (offsetof (struct perf_event_mmap_page, data_size) <= header->size)
     {
+      __u64 data_size;
+
       data_offset = header->data_offset;
       data_size = header->data_size;
+
+      size = (unsigned int) data_size;
+
+      /* Check for overflows.  */
+      if ((__u64) size != data_size)
+	{
+	  munmap ((void *) header, (size_t) size + PAGE_SIZE);
+	  goto err_file;
+	}
     }
 #endif /* defined (PERF_ATTR_SIZE_VER5) */
 
   bts->header = header;
   bts->bts.mem = ((const uint8_t *) header) + data_offset;
-  bts->bts.size = data_size;
+  bts->bts.size = size;
   bts->bts.data_head = &header->data_head;
-  bts->bts.last_head = 0;
+  bts->bts.last_head = 0ull;
 
-  tinfo->conf.bts.size = data_size;
+  tinfo->conf.bts.size = (unsigned int) size;
   return tinfo;
 
  err_file:
@@ -746,7 +760,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   struct perf_event_mmap_page *header;
   struct btrace_target_info *tinfo;
   struct btrace_tinfo_pt *pt;
-  unsigned long long pages, size;
+  unsigned long pages, size;
   int pid, pg, errcode, type;
 
   if (conf->size == 0)
@@ -788,7 +802,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   header->aux_offset = header->data_offset + header->data_size;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
+  pages = (((unsigned long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
@@ -806,12 +820,12 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
       size_t length;
 
       size = pages * PAGE_SIZE;
-      length = size;
 
-      /* Check for overflows.  */
-      if ((unsigned long long) length < size)
+      /* Don't ask for more than we can represent in the configuration.  */
+      if (UINT_MAX < size)
 	continue;
 
+      length = size;
       header->aux_size = size;
 
       pt->pt.mem = mmap (NULL, length, PROT_READ, MAP_SHARED, pt->file,
@@ -827,7 +841,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   pt->pt.size = size;
   pt->pt.data_head = &header->aux_head;
 
-  tinfo->conf.pt.size = size;
+  tinfo->conf.pt.size = (unsigned int) size;
   return tinfo;
 
  err_conf:
@@ -938,7 +952,8 @@ linux_read_bts (struct btrace_data_bts *btrace,
 {
   struct perf_event_buffer *pevent;
   const uint8_t *begin, *end, *start;
-  unsigned long long data_head, data_tail, buffer_size, size;
+  unsigned long buffer_size, size;
+  __u64 data_head, data_tail;
   unsigned int retries = 5;
 
   pevent = &tinfo->variant.bts.bts;
@@ -961,6 +976,8 @@ linux_read_bts (struct btrace_data_bts *btrace,
 
       if (type == BTRACE_READ_DELTA)
 	{
+	  __u64 data_size;
+
 	  /* Determine the number of bytes to read and check for buffer
 	     overflows.  */
 
@@ -971,9 +988,12 @@ linux_read_bts (struct btrace_data_bts *btrace,
 	    return BTRACE_ERR_OVERFLOW;
 
 	  /* If the buffer is smaller than the trace delta, we overflowed.  */
-	  size = data_head - data_tail;
-	  if (buffer_size < size)
+	  data_size = data_head - data_tail;
+	  if (buffer_size < data_size)
 	    return BTRACE_ERR_OVERFLOW;
+
+	  /* DATA_SIZE <= BUFFER_SIZE and therefore fits into a long.  */
+	  size = (unsigned long) data_size;
 	}
       else
 	{
@@ -982,7 +1002,7 @@ linux_read_bts (struct btrace_data_bts *btrace,
 
 	  /* Adjust the size if the buffer has not overflowed, yet.  */
 	  if (data_head < size)
-	    size = data_head;
+	    size = (unsigned long) data_head;
 	}
 
       /* Data_head keeps growing; the buffer itself is circular.  */
diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index b680bf5..5fcaf79 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -38,13 +38,13 @@ struct perf_event_buffer
   const uint8_t *mem;
 
   /* The size of the mapped memory in bytes.  */
-  unsigned long long size;
+  unsigned long size;
 
   /* A pointer to the data_head field for this buffer. */
-  volatile unsigned long long *data_head;
+  volatile __u64 *data_head;
 
   /* The data_head value from the last read.  */
-  unsigned long long last_head;
+  __u64 last_head;
 };
 
 /* Branch trace target information for BTS tracing.  */
-- 
1.8.3.1

[PATCH v2 1/2] record: set stop_pc in "record goto" command

[Markus Metzger]

When navigating in the recorded execution trace via "record goto", we do not
set stop_pc.  This may trigger an internal error in infrun.c when stepping
from that location.  Set it.

(gdb) rec full
(gdb) c
Continuing.

Breakpoint 1, foo (void) at foo.c:42
42             x = y
(gdb) rn
foo (void)
    at foo.c:41
41             y = x
(gdb) rec go end
Go forward to insn number 98724
    at foo.c:42
42             x = y
(gdb) n
infrun.c:2382: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

2015-07-09  Markus Metzger <markus.t.metzger@intel.com>

gdb/
	* record-btrace.c (record_btrace_goto_begin, record_btrace_goto_end)
	record_btrace_goto): Move call to print_stack_frame ...
	(record_btrace_set_replay): ... here.  Set stop_pc.
	* record-full.c (record_full_goto_entry): Set stop_pc.

testsuite/
	* gdb.btrace/record_goto-step.exp: New.
---
 gdb/record-btrace.c                           |  9 ++----
 gdb/record-full.c                             |  1 +
 gdb/testsuite/gdb.btrace/record_goto-step.exp | 46 +++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/record_goto-step.exp

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 969e01b..6f4ee66 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2212,6 +2212,9 @@ record_btrace_set_replay (struct thread_info *tp,
 
   /* Start anew from the new replay position.  */
   record_btrace_clear_histories (btinfo);
+
+  stop_pc = regcache_read_pc (get_current_regcache ());
+  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
 }
 
 /* The to_goto_record_begin method of target record-btrace.  */
@@ -2226,8 +2229,6 @@ record_btrace_goto_begin (struct target_ops *self)
 
   btrace_insn_begin (&begin, &tp->btrace);
   record_btrace_set_replay (tp, &begin);
-
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
 }
 
 /* The to_goto_record_end method of target record-btrace.  */
@@ -2240,8 +2241,6 @@ record_btrace_goto_end (struct target_ops *ops)
   tp = require_btrace_thread ();
 
   record_btrace_set_replay (tp, NULL);
-
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
 }
 
 /* The to_goto_record method of target record-btrace.  */
@@ -2267,8 +2266,6 @@ record_btrace_goto (struct target_ops *self, ULONGEST insn)
     error (_("No such instruction."));
 
   record_btrace_set_replay (tp, &it);
-
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
 }
 
 /* The to_execution_direction target method.  */
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 43e8be2..1520811 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1869,6 +1869,7 @@ record_full_goto_entry (struct record_full_entry *p)
 
   registers_changed ();
   reinit_frame_cache ();
+  stop_pc = regcache_read_pc (get_current_regcache ());
   print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1);
 }
 
diff --git a/gdb/testsuite/gdb.btrace/record_goto-step.exp b/gdb/testsuite/gdb.btrace/record_goto-step.exp
new file mode 100644
index 0000000..bbf0b77
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/record_goto-step.exp
@@ -0,0 +1,46 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013-2015 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <markus.t.metzger@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# check for btrace support
+if { [skip_btrace_tests] } { return -1 }
+
+standard_testfile record_goto.c
+
+# start inferior
+if [prepare_for_testing record_goto-step.exp $testfile $srcfile] {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+set bp [gdb_get_line_number "fun4.3" $srcfile]
+gdb_breakpoint $srcfile:$bp
+
+# record the execution until we hit a breakpoint
+gdb_test_no_output "record btrace"
+gdb_continue_to_breakpoint "cont to $bp" ".*$srcfile:$bp.*"
+
+# reverse-step, then jump to the end of the trace
+gdb_test "reverse-next" ".*fun4\.2.*"
+gdb_test "record goto end" ".*fun4\.3.*"
+
+# test that we can continue stepping
+gdb_test "next" ".*fun4\.4.*"
-- 
1.8.3.1

Re: [PATCH v2 1/2] record: set stop_pc in "record goto" command

[Pedro Alves]

Thanks.

On 07/09/2015 07:08 AM, Markus Metzger wrote:
> When navigating in the recorded execution trace via "record goto", we do not
> set stop_pc.  This may trigger an internal error in infrun.c when stepping
> from that location.  Set it.
> 
> (gdb) rec full
> (gdb) c
> Continuing.
> 
> Breakpoint 1, foo (void) at foo.c:42
> 42             x = y
> (gdb) rn
> foo (void)
>     at foo.c:41
> 41             y = x
> (gdb) rec go end
> Go forward to insn number 98724
>     at foo.c:42
> 42             x = y
> (gdb) n
> infrun.c:2382: internal-error: resume: Assertion `sig != GDB_SIGNAL_0' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)

I realized that we had context in our minds from the private
chats that might be missing from the archives/logs.

I think we should extend the commit log a bit to connect the
dots between "stop_pc not set" <-> "internal error".  E.g.,:

~~~
This happens because there's a breakpoint at PC when the "next"
is issued, so that breapoint should be immediately stepped over.
That should have been detected/done by proceed, here:

  if (addr == (CORE_ADDR) -1)
    {
      if (pc == stop_pc
	  && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here
	  && execution_direction != EXEC_REVERSE)
	/* There is a breakpoint at the address we will resume at,
	   step one instruction before inserting breakpoints so that
	   we do not stop right away (and report a second hit at this
	   breakpoint).

	   Note, we don't do this in reverse, because we won't
	   actually be executing the breakpoint insn anyway.
	   We'll be (un-)executing the previous instruction.  */
	tp->stepping_over_breakpoint = 1;

But since stop_pc was stale, the pc == stop_pc check failed, and left the
breakpont at PC inserted.

~~~~~~

> +set bp [gdb_get_line_number "fun4.3" $srcfile]
> +gdb_breakpoint $srcfile:$bp
> +
> +# record the execution until we hit a breakpoint
> +gdb_test_no_output "record btrace"
> +gdb_continue_to_breakpoint "cont to $bp" ".*$srcfile:$bp.*"
> +

Seems like these put line number in the test message
in gdb.sum.  Please tweak the messages to avoid that,
so that if the test lines change in the future, the test
messages remain stable.

> +# reverse-step, then jump to the end of the trace
> +gdb_test "reverse-next" ".*fun4\.2.*"
> +gdb_test "record goto end" ".*fun4\.3.*"
> +
> +# test that we can continue stepping

Please mention the breakpoint here.  E.g.,:

# test that we can continue stepping, even though
# there's a breakpoint at PC.

> +gdb_test "next" ".*fun4\.4.*"

Fix these and you're good to go.  Please push.

Thanks,
Pedro Alves

Re: [PATCH v2 2/2] ari, btrace: avoid unsigned long long

[Pedro Alves]

On 07/09/2015 07:08 AM, Markus Metzger wrote:
> Fix the ARI warning about the use of unsigned long long.  We can't use ULONGEST
> as this is defined unsigned long on 64-bit systems.

But, what exactly would break?

> Use unsigned long to hold
> the buffer size inside GDB 

Why not use size_t instead then?

> and __u64 when interfacing the kernel.

Thanks,
Pedro Alves

RE: [PATCH v2 2/2] ari, btrace: avoid unsigned long long

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, July 9, 2015 1:15 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 2/2] ari, btrace: avoid unsigned long long
> 
> On 07/09/2015 07:08 AM, Markus Metzger wrote:
> > Fix the ARI warning about the use of unsigned long long.  We can't use
> ULONGEST
> > as this is defined unsigned long on 64-bit systems.
> 
> But, what exactly would break?

I changed the commit message to this:

    Fix the ARI warning about the use of unsigned long long.  We can't use
    ULONGEST as this is defined unsigned long on 64-bit systems.  This will
    result in a compile error when storing a pointer to an unsigned long long
    structure field (declared in perf_evene.h as __u64) in a ULONGEST * variable.
    
    Use unsigned long to hold the buffer size inside GDB and __u64 when
    interfacing the kernel.


Is that OK?

> > Use unsigned long to hold
> > the buffer size inside GDB
> 
> Why not use size_t instead then?

It's another typedef.  And without a clearly defined size.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 2/2] ari, btrace: avoid unsigned long long

[Pedro Alves]

On 07/10/2015 08:16 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: Pedro Alves [mailto:palves@redhat.com]
>> Sent: Thursday, July 9, 2015 1:15 PM
>> To: Metzger, Markus T
>> Cc: gdb-patches@sourceware.org
>> Subject: Re: [PATCH v2 2/2] ari, btrace: avoid unsigned long long
>>
>> On 07/09/2015 07:08 AM, Markus Metzger wrote:
>>> Fix the ARI warning about the use of unsigned long long.  We can't use
>> ULONGEST
>>> as this is defined unsigned long on 64-bit systems.
>>
>> But, what exactly would break?
> 
> I changed the commit message to this:
> 
>     Fix the ARI warning about the use of unsigned long long.  We can't use
>     ULONGEST as this is defined unsigned long on 64-bit systems.  This will
>     result in a compile error when storing a pointer to an unsigned long long
>     structure field (declared in perf_evene.h as __u64) in a ULONGEST * variable.
>     
>     Use unsigned long to hold the buffer size inside GDB and __u64 when
>     interfacing the kernel.
> 
> 
> Is that OK?
> 

That's much clearer, thanks.  Typo in perf_evene.h.

>>> Use unsigned long to hold
>>> the buffer size inside GDB
>>
>> Why not use size_t instead then?
> 
> It's another typedef.  And without a clearly defined size.

But it's the right type to use to represent a buffer size,
and it's the type that mmap expects too.  So I'd find it
all self-documenting that way.  See adjusted version of your patch
below.  I added a few extra comments to clarify the "unsigned int"
and UINT_MAX uses, which were not clear at all to me before.

(build tested on 64-bit and 32-bit, but otherwise not
tested; I'm still with the machine that doesn't do btrace.)

WDYT?

From: Markus Metzger <markus.t.metzger@intel.com>
Date: 2015-07-10 14:55:53 +0100

---

 gdb/btrace.c               |    6 ++-
 gdb/common/btrace-common.h |   12 +++++-
 gdb/nat/linux-btrace.c     |   84 ++++++++++++++++++++++++++++----------------
 gdb/nat/linux-btrace.h     |    6 ++-
 4 files changed, 68 insertions(+), 40 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 731d237..cc442ce 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1414,17 +1414,17 @@ parse_xml_btrace_block (struct gdb_xml_parser *parser,
 
 static void
 parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
-	       gdb_byte **pdata, unsigned long *psize)
+	       gdb_byte **pdata, size_t *psize)
 {
   struct cleanup *cleanup;
   gdb_byte *data, *bin;
-  unsigned long size;
+  size_t size;
   size_t len;
 
   len = strlen (body_text);
   size = len / 2;
 
-  if ((size_t) size * 2 != len)
+  if (size * 2 != len)
     gdb_xml_error (parser, _("Bad raw data size."));
 
   bin = data = xmalloc (size);
diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h
index f22efc5..96df283 100644
--- a/gdb/common/btrace-common.h
+++ b/gdb/common/btrace-common.h
@@ -96,7 +96,10 @@ struct btrace_cpu
 
 struct btrace_config_bts
 {
-  /* The size of the branch trace buffer in bytes.  */
+  /* The size of the branch trace buffer in bytes.  This is an
+     unsigned int instead of a size_t because it is registered as
+     control variable for the "set record btrace bts buffer-size"
+     command.  */
   unsigned int size;
 };
 
@@ -104,7 +107,10 @@ struct btrace_config_bts
 
 struct btrace_config_pt
 {
-  /* The size of the branch trace buffer in bytes.  */
+  /* The size of the branch trace buffer in bytes.  This is an
+     unsigned int instead of a size_t because it is registered as
+     control variable for the "set record btrace pt buffer-size"
+     command.  */
   unsigned int size;
 };
 
@@ -152,7 +158,7 @@ struct btrace_data_pt
   gdb_byte *data;
 
   /* The size of DATA in bytes.  */
-  unsigned long size;
+  size_t size;
 };
 
 /* The branch trace data.  */
diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b6e13d3..3ccf833 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -111,12 +111,13 @@ perf_event_new_data (const struct perf_event_buffer *pev)
    The caller is responsible for freeing the memory.  */
 
 static gdb_byte *
-perf_event_read (const struct perf_event_buffer *pev, unsigned long data_head,
-		 unsigned long size)
+perf_event_read (const struct perf_event_buffer *pev, __u64 data_head,
+		 size_t size)
 {
   const gdb_byte *begin, *end, *start, *stop;
   gdb_byte *buffer;
-  unsigned long data_tail, buffer_size;
+  size_t buffer_size;
+  __u64 data_tail;
 
   if (size == 0)
     return NULL;
@@ -149,9 +150,10 @@ perf_event_read (const struct perf_event_buffer *pev, unsigned long data_head,
 
 static void
 perf_event_read_all (struct perf_event_buffer *pev, gdb_byte **data,
-		     unsigned long *psize)
+		     size_t *psize)
 {
-  unsigned long data_head, size;
+  size_t size;
+  __u64 data_head;
 
   data_head = *pev->data_head;
 
@@ -270,11 +272,11 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
 static VEC (btrace_block_s) *
 perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
 		     const uint8_t *end, const uint8_t *start,
-		     unsigned long long size)
+		     size_t size)
 {
   VEC (btrace_block_s) *btrace = NULL;
   struct perf_event_sample sample;
-  unsigned long long read = 0;
+  size_t read = 0;
   struct btrace_block block = { 0, 0 };
   struct regcache *regcache;
 
@@ -642,7 +644,8 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
   struct perf_event_mmap_page *header;
   struct btrace_target_info *tinfo;
   struct btrace_tinfo_bts *bts;
-  unsigned long long size, pages, data_offset, data_size;
+  size_t size, pages;
+  __u64 data_offset;
   int pid, pg;
 
   tinfo = xzalloc (sizeof (*tinfo));
@@ -674,16 +677,16 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
     goto err_out;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
+  pages = ((size_t) conf->size + PAGE_SIZE - 1) / PAGE_SIZE;
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
 
   /* The buffer size can be requested in powers of two pages.  Adjust PAGES
      to the next power of two.  */
-  for (pg = 0; pages != (1u << pg); ++pg)
-    if ((pages & (1u << pg)) != 0)
-      pages += (1u << pg);
+  for (pg = 0; pages != (((size_t) 1) << pg); ++pg)
+    if ((pages & (((size_t) 1) << pg)) != 0)
+      pages += (((size_t) 1) << pg);
 
   /* We try to allocate the requested size.
      If that fails, try to get as much as we can.  */
@@ -692,12 +695,14 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
       size_t length;
 
       size = pages * PAGE_SIZE;
-      length = size + PAGE_SIZE;
 
-      /* Check for overflows.  */
-      if ((unsigned long long) length < size)
+      /* Don't ask for more than we can represent in the configuration
+	 (with "set record btrace bts buffer-size").  */
+      if (UINT_MAX < size)
 	continue;
 
+      length = size + PAGE_SIZE;
+
       /* The number of pages we request needs to be a power of two.  */
       header = mmap (NULL, length, PROT_READ, MAP_SHARED, bts->file, 0);
       if (header != MAP_FAILED)
@@ -708,23 +713,33 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
     goto err_file;
 
   data_offset = PAGE_SIZE;
-  data_size = size;
 
 #if defined (PERF_ATTR_SIZE_VER5)
   if (offsetof (struct perf_event_mmap_page, data_size) <= header->size)
     {
+      __u64 data_size;
+
       data_offset = header->data_offset;
       data_size = header->data_size;
+
+      size = (size_t) data_size;
+
+      /* Check for overflow.  */
+      if ((__u64) size != data_size)
+	{
+	  munmap ((void *) header, size + PAGE_SIZE);
+	  goto err_file;
+	}
     }
 #endif /* defined (PERF_ATTR_SIZE_VER5) */
 
   bts->header = header;
   bts->bts.mem = ((const uint8_t *) header) + data_offset;
-  bts->bts.size = data_size;
+  bts->bts.size = size;
   bts->bts.data_head = &header->data_head;
   bts->bts.last_head = 0;
 
-  tinfo->conf.bts.size = data_size;
+  tinfo->conf.bts.size = (unsigned int) size;
   return tinfo;
 
  err_file:
@@ -746,7 +761,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   struct perf_event_mmap_page *header;
   struct btrace_target_info *tinfo;
   struct btrace_tinfo_pt *pt;
-  unsigned long long pages, size;
+  size_t size, pages;
   int pid, pg, errcode, type;
 
   if (conf->size == 0)
@@ -788,16 +803,16 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   header->aux_offset = header->data_offset + header->data_size;
 
   /* Convert the requested size in bytes to pages (rounding up).  */
-  pages = (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
+  pages = ((size_t) conf->size + PAGE_SIZE - 1) / PAGE_SIZE;
   /* We need at least one page.  */
   if (pages == 0)
     pages = 1;
 
   /* The buffer size can be requested in powers of two pages.  Adjust PAGES
      to the next power of two.  */
-  for (pg = 0; pages != (1u << pg); ++pg)
-    if ((pages & (1u << pg)) != 0)
-      pages += (1u << pg);
+  for (pg = 0; pages != (((size_t) 1) << pg); ++pg)
+    if ((pages & (((size_t) 1) << pg)) != 0)
+      pages += (((size_t) 1) << pg);
 
   /* We try to allocate the requested size.
      If that fails, try to get as much as we can.  */
@@ -806,12 +821,13 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
       size_t length;
 
       size = pages * PAGE_SIZE;
-      length = size;
 
-      /* Check for overflows.  */
-      if ((unsigned long long) length < size)
+      /* Don't ask for more than we can represent in the configuration
+	 (with "set record btrace pt buffer-size").  */
+      if (UINT_MAX < size)
 	continue;
 
+      length = size;
       header->aux_size = size;
 
       pt->pt.mem = mmap (NULL, length, PROT_READ, MAP_SHARED, pt->file,
@@ -827,7 +843,7 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   pt->pt.size = size;
   pt->pt.data_head = &header->aux_head;
 
-  tinfo->conf.pt.size = size;
+  tinfo->conf.pt.size = (unsigned int) size;
   return tinfo;
 
  err_conf:
@@ -938,7 +954,8 @@ linux_read_bts (struct btrace_data_bts *btrace,
 {
   struct perf_event_buffer *pevent;
   const uint8_t *begin, *end, *start;
-  unsigned long long data_head, data_tail, buffer_size, size;
+  size_t buffer_size, size;
+  __u64 data_head, data_tail;
   unsigned int retries = 5;
 
   pevent = &tinfo->variant.bts.bts;
@@ -961,6 +978,8 @@ linux_read_bts (struct btrace_data_bts *btrace,
 
       if (type == BTRACE_READ_DELTA)
 	{
+	  __u64 data_size;
+
 	  /* Determine the number of bytes to read and check for buffer
 	     overflows.  */
 
@@ -971,9 +990,12 @@ linux_read_bts (struct btrace_data_bts *btrace,
 	    return BTRACE_ERR_OVERFLOW;
 
 	  /* If the buffer is smaller than the trace delta, we overflowed.  */
-	  size = data_head - data_tail;
-	  if (buffer_size < size)
+	  data_size = data_head - data_tail;
+	  if (buffer_size < data_size)
 	    return BTRACE_ERR_OVERFLOW;
+
+	  /* DATA_SIZE <= BUFFER_SIZE and therefore fits into a size_t.  */
+	  size = data_size;
 	}
       else
 	{
@@ -982,7 +1004,7 @@ linux_read_bts (struct btrace_data_bts *btrace,
 
 	  /* Adjust the size if the buffer has not overflowed, yet.  */
 	  if (data_head < size)
-	    size = data_head;
+	    size = (size_t) data_head;
 	}
 
       /* Data_head keeps growing; the buffer itself is circular.  */
diff --git a/gdb/nat/linux-btrace.h b/gdb/nat/linux-btrace.h
index b680bf5..5ea87a8 100644
--- a/gdb/nat/linux-btrace.h
+++ b/gdb/nat/linux-btrace.h
@@ -38,13 +38,13 @@ struct perf_event_buffer
   const uint8_t *mem;
 
   /* The size of the mapped memory in bytes.  */
-  unsigned long long size;
+  size_t size;
 
   /* A pointer to the data_head field for this buffer. */
-  volatile unsigned long long *data_head;
+  volatile __u64 *data_head;
 
   /* The data_head value from the last read.  */
-  unsigned long long last_head;
+  __u64 last_head;
 };
 
 /* Branch trace target information for BTS tracing.  */

RE: [PATCH v2 2/2] ari, btrace: avoid unsigned long long

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Friday, July 10, 2015 4:05 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 2/2] ari, btrace: avoid unsigned long long


> >>> Use unsigned long to hold
> >>> the buffer size inside GDB
> >>
> >> Why not use size_t instead then?
> >
> > It's another typedef.  And without a clearly defined size.
> 
> But it's the right type to use to represent a buffer size,
> and it's the type that mmap expects too.  So I'd find it
> all self-documenting that way.  See adjusted version of your patch
> below.  I added a few extra comments to clarify the "unsigned int"
> and UINT_MAX uses, which were not clear at all to me before.
> 
> (build tested on 64-bit and 32-bit, but otherwise not
> tested; I'm still with the machine that doesn't do btrace.)
> 
> WDYT?

Now that you did all the editing, do you want to commit this?


>  static void
>  parse_xml_raw (struct gdb_xml_parser *parser, const char *body_text,
> -	       gdb_byte **pdata, unsigned long *psize)
> +	       gdb_byte **pdata, size_t *psize)
>  {
>    struct cleanup *cleanup;
>    gdb_byte *data, *bin;
> -  unsigned long size;
> +  size_t size;
>    size_t len;

Can be combined with the LEN declaration.

> 
>    len = strlen (body_text);
>    size = len / 2;
> 
> -  if ((size_t) size * 2 != len)
> +  if (size * 2 != len)
>      gdb_xml_error (parser, _("Bad raw data size."));

The check was originally meant to catch overflows.  Now it is just checking
that LEN is even, which might better be done as "len % 2 != 0".


>    /* Convert the requested size in bytes to pages (rounding up).  */
> -  pages = (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
> +  pages = ((size_t) conf->size + PAGE_SIZE - 1) / PAGE_SIZE;

Here, we trust that size_t is bigger than unsigned int.  I guess this was wrong
with unsigned long, as well.  This should better be:

    pages = conf->size / PAGE_SIZE + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);


>    /* We try to allocate the requested size.
>       If that fails, try to get as much as we can.  */
> @@ -692,12 +695,14 @@ linux_enable_bts (ptid_t ptid, const struct
> btrace_config_bts *conf)
>        size_t length;
> 
>        size = pages * PAGE_SIZE;
> -      length = size + PAGE_SIZE;
> 
> -      /* Check for overflows.  */
> -      if ((unsigned long long) length < size)
> +      /* Don't ask for more than we can represent in the configuration
> +	 (with "set record btrace bts buffer-size").  */
> +      if (UINT_MAX < size)
>  	continue;
> 
> +      length = size + PAGE_SIZE;

We still need to check for overflows.  This was also wrong before.


>    if (conf->size == 0)
> @@ -788,16 +803,16 @@ linux_enable_pt (ptid_t ptid, const struct
> btrace_config_pt *conf)
>    header->aux_offset = header->data_offset + header->data_size;
> 
>    /* Convert the requested size in bytes to pages (rounding up).  */
> -  pages = (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_SIZE;
> +  pages = ((size_t) conf->size + PAGE_SIZE - 1) / PAGE_SIZE;

See above.


Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v2 1/2] record: set stop_pc in "record goto" command

[Metzger, Markus T]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, July 9, 2015 12:38 PM
> To: Metzger, Markus T
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v2 1/2] record: set stop_pc in "record goto" command


> > When navigating in the recorded execution trace via "record goto", we do
> not
> > set stop_pc.  This may trigger an internal error in infrun.c when stepping
> > from that location.  Set it.
> >
> > (gdb) rec full
> > (gdb) c
> > Continuing.
> >
> > Breakpoint 1, foo (void) at foo.c:42
> > 42             x = y
> > (gdb) rn
> > foo (void)
> >     at foo.c:41
> > 41             y = x
> > (gdb) rec go end
> > Go forward to insn number 98724
> >     at foo.c:42
> > 42             x = y
> > (gdb) n
> > infrun.c:2382: internal-error: resume: Assertion `sig != GDB_SIGNAL_0'
> failed.
> > A problem internal to GDB has been detected,
> > further debugging may prove unreliable.
> > Quit this debugging session? (y or n)

Would this make sense for GDB 7.10, as well?

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 2/2] ari, btrace: avoid unsigned long long

[Pedro Alves]

On 07/13/2015 08:20 AM, Metzger, Markus T wrote:

> Now that you did all the editing, do you want to commit this?

I'd appreciate it if you could pick it up, update the commit
log, and address the points you raised (which I agree with).
Could you do that?

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] record: set stop_pc in "record goto" command

[Pedro Alves]

On 07/13/2015 08:39 AM, Metzger, Markus T wrote:

>>> (gdb) n
>>> infrun.c:2382: internal-error: resume: Assertion `sig != GDB_SIGNAL_0'
>> failed.
>>> A problem internal to GDB has been detected,
>>> further debugging may prove unreliable.
>>> Quit this debugging session? (y or n)
> 
> Would this make sense for GDB 7.10, as well?

I think so, yes.

Thanks,
Pedro Alves