[PATCH v3] ari, btrace: avoid unsigned long long

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] ari, btrace: avoid unsigned long long

Metzger, Markus T
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_event.h as __u64) in a ULONGEST * variable.

Use size_t to hold the buffer size inside GDB and __u64 when interfacing the
Linux kernel.

2015-07-13  Markus Metzger <[hidden email]>
               Pedro Alves <[hidden email]>

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.h (struct perf_event_buffer)<size, data_head>
        <last_head>: Change type.
        * common/btrace-common.h (struct btrace_dat_pt) <size>: Change type.
        * common/btrace-common.c (btrace_data_append): Change the type of
        SIZE.  Change case label.
        * btrace.c (parse_xml_raw): Change the type of SIZE.  Change oddness
        check.
---
 gdb/btrace.c               |  11 +++--
 gdb/common/btrace-common.c |   4 +-
 gdb/common/btrace-common.h |  12 ++++--
 gdb/nat/linux-btrace.c     | 102 ++++++++++++++++++++++++++++++---------------
 gdb/nat/linux-btrace.h     |   6 +--
 5 files changed, 87 insertions(+), 48 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 731d237..94942f4 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1414,19 +1414,18 @@ 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 len;
+  size_t len, size;
 
   len = strlen (body_text);
-  size = len / 2;
-
-  if ((size_t) size * 2 != len)
+  if (len % 2 != 0)
     gdb_xml_error (parser, _("Bad raw data size."));
 
+  size = len / 2;
+
   bin = data = xmalloc (size);
   cleanup = make_cleanup (xfree, data);
 
diff --git a/gdb/common/btrace-common.c b/gdb/common/btrace-common.c
index 95193eb..9d6c4a7 100644
--- a/gdb/common/btrace-common.c
+++ b/gdb/common/btrace-common.c
@@ -155,10 +155,10 @@ btrace_data_append (struct btrace_data *dst,
   dst->variant.pt.size = 0;
 
   /* fall-through.  */
- case BTRACE_FORMAT_BTS:
+ case BTRACE_FORMAT_PT:
   {
     gdb_byte *data;
-    unsigned long size;
+    size_t size;
 
     size = src->variant.pt.size + dst->variant.pt.size;
     data = xmalloc (size);
diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h
index f22efc5..c90a331 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 unsigned int and not size_t since it is registered as
+     control variable for "set record btrace bts buffer-size".  */
   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 unsigned int and not size_t since it is registered as
+     control variable for "set record btrace pt buffer-size".  */
   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..e51c8fa 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,15 +150,16 @@ 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;
 
   size = pev->size;
   if (data_head < size)
-    size = data_head;
+    size = (size_t) data_head;
 
   *data = perf_event_read (pev, data_head, size);
   *psize = size;
@@ -269,12 +271,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)
+     const uint8_t *end, const uint8_t *start, 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 +643,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,28 +676,36 @@ 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
+    + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);
   /* 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.  */
   for (; pages > 0; pages >>= 1)
     {
       size_t length;
+      __u64 data_size;
 
-      size = pages * PAGE_SIZE;
+      data_size = (__u64) pages * PAGE_SIZE;
+
+      /* Don't ask for more than we can represent in the configuration.  */
+      if ((__u64) UINT_MAX < data_size)
+ continue;
+
+      size = (size_t) data_size;
       length = size + PAGE_SIZE;
 
       /* Check for overflows.  */
-      if ((unsigned long long) length < size)
+      if ((__u64) length != data_size + PAGE_SIZE)
  continue;
 
       /* The number of pages we request needs to be a power of two.  */
@@ -708,23 +718,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 + 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 +766,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 pages, size;
   int pid, pg, errcode, type;
 
   if (conf->size == 0)
@@ -788,31 +808,39 @@ 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
+    + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);
   /* 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.  */
   for (; pages > 0; pages >>= 1)
     {
       size_t length;
+      __u64 data_size;
 
-      size = pages * PAGE_SIZE;
-      length = size;
+      data_size = (__u64) pages * PAGE_SIZE;
+
+      /* Don't ask for more than we can represent in the configuration.  */
+      if ((__u64) UINT_MAX < data_size)
+ continue;
+
+      size = (size_t) data_size;
 
       /* Check for overflows.  */
-      if ((unsigned long long) length < size)
+      if ((__u64) size != data_size)
  continue;
 
-      header->aux_size = size;
+      header->aux_size = data_size;
+      length = size;
 
       pt->pt.mem = mmap (NULL, length, PROT_READ, MAP_SHARED, pt->file,
  header->aux_offset);
@@ -827,7 +855,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 +966,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 +990,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 +1002,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 = (size_t) data_size;
  }
       else
  {
@@ -982,7 +1016,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.  */
--
1.8.3.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] ari, btrace: avoid unsigned long long

Mark Kettenis
> From: Markus Metzger <[hidden email]>
> Date: Mon, 13 Jul 2015 16:47:15 +0200
>
> 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_event.h as __u64) in a ULONGEST * variable.
>
> Use size_t to hold the buffer size inside GDB and __u64 when interfacing the
> Linux kernel.

Please don't propagate the use of Linux kernel types outside of the
linux kernel.  The ISO C spelling of __u64 is uint64_t.

Cheers,

Mark

> 2015-07-13  Markus Metzger <[hidden email]>
>                Pedro Alves <[hidden email]>
>
> 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.h (struct perf_event_buffer)<size, data_head>
> <last_head>: Change type.
> * common/btrace-common.h (struct btrace_dat_pt) <size>: Change type.
> * common/btrace-common.c (btrace_data_append): Change the type of
> SIZE.  Change case label.
> * btrace.c (parse_xml_raw): Change the type of SIZE.  Change oddness
> check.
> ---
>  gdb/btrace.c               |  11 +++--
>  gdb/common/btrace-common.c |   4 +-
>  gdb/common/btrace-common.h |  12 ++++--
>  gdb/nat/linux-btrace.c     | 102 ++++++++++++++++++++++++++++++---------------
>  gdb/nat/linux-btrace.h     |   6 +--
>  5 files changed, 87 insertions(+), 48 deletions(-)
>
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 731d237..94942f4 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1414,19 +1414,18 @@ 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 len;
> +  size_t len, size;
>  
>    len = strlen (body_text);
> -  size = len / 2;
> -
> -  if ((size_t) size * 2 != len)
> +  if (len % 2 != 0)
>      gdb_xml_error (parser, _("Bad raw data size."));
>  
> +  size = len / 2;
> +
>    bin = data = xmalloc (size);
>    cleanup = make_cleanup (xfree, data);
>  
> diff --git a/gdb/common/btrace-common.c b/gdb/common/btrace-common.c
> index 95193eb..9d6c4a7 100644
> --- a/gdb/common/btrace-common.c
> +++ b/gdb/common/btrace-common.c
> @@ -155,10 +155,10 @@ btrace_data_append (struct btrace_data *dst,
>    dst->variant.pt.size = 0;
>  
>    /* fall-through.  */
> - case BTRACE_FORMAT_BTS:
> + case BTRACE_FORMAT_PT:
>    {
>      gdb_byte *data;
> -    unsigned long size;
> +    size_t size;
>  
>      size = src->variant.pt.size + dst->variant.pt.size;
>      data = xmalloc (size);
> diff --git a/gdb/common/btrace-common.h b/gdb/common/btrace-common.h
> index f22efc5..c90a331 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 unsigned int and not size_t since it is registered as
> +     control variable for "set record btrace bts buffer-size".  */
>    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 unsigned int and not size_t since it is registered as
> +     control variable for "set record btrace pt buffer-size".  */
>    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..e51c8fa 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,15 +150,16 @@ 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;
>  
>    size = pev->size;
>    if (data_head < size)
> -    size = data_head;
> +    size = (size_t) data_head;
>  
>    *data = perf_event_read (pev, data_head, size);
>    *psize = size;
> @@ -269,12 +271,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)
> +     const uint8_t *end, const uint8_t *start, 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 +643,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,28 +676,36 @@ 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
> +    + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);
>    /* 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.  */
>    for (; pages > 0; pages >>= 1)
>      {
>        size_t length;
> +      __u64 data_size;
>  
> -      size = pages * PAGE_SIZE;
> +      data_size = (__u64) pages * PAGE_SIZE;
> +
> +      /* Don't ask for more than we can represent in the configuration.  */
> +      if ((__u64) UINT_MAX < data_size)
> + continue;
> +
> +      size = (size_t) data_size;
>        length = size + PAGE_SIZE;
>  
>        /* Check for overflows.  */
> -      if ((unsigned long long) length < size)
> +      if ((__u64) length != data_size + PAGE_SIZE)
>   continue;
>  
>        /* The number of pages we request needs to be a power of two.  */
> @@ -708,23 +718,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 + 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 +766,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 pages, size;
>    int pid, pg, errcode, type;
>  
>    if (conf->size == 0)
> @@ -788,31 +808,39 @@ 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
> +    + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);
>    /* 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.  */
>    for (; pages > 0; pages >>= 1)
>      {
>        size_t length;
> +      __u64 data_size;
>  
> -      size = pages * PAGE_SIZE;
> -      length = size;
> +      data_size = (__u64) pages * PAGE_SIZE;
> +
> +      /* Don't ask for more than we can represent in the configuration.  */
> +      if ((__u64) UINT_MAX < data_size)
> + continue;
> +
> +      size = (size_t) data_size;
>  
>        /* Check for overflows.  */
> -      if ((unsigned long long) length < size)
> +      if ((__u64) size != data_size)
>   continue;
>  
> -      header->aux_size = size;
> +      header->aux_size = data_size;
> +      length = size;
>  
>        pt->pt.mem = mmap (NULL, length, PROT_READ, MAP_SHARED, pt->file,
>   header->aux_offset);
> @@ -827,7 +855,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 +966,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 +990,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 +1002,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 = (size_t) data_size;
>   }
>        else
>   {
> @@ -982,7 +1016,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.  */
> --
> 1.8.3.1
>
>
Reply | Threaded
Open this post in threaded view
|

RE: [PATCH v3] ari, btrace: avoid unsigned long long

Metzger, Markus T
> -----Original Message-----
> From: Mark Kettenis [mailto:[hidden email]]
> Sent: Monday, July 13, 2015 5:36 PM
> To: Metzger, Markus T
> Cc: [hidden email]; [hidden email]
> Subject: Re: [PATCH v3] ari, btrace: avoid unsigned long long

Hello Mark,

> > 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_event.h as __u64) in a ULONGEST *
> variable.
> >
> > Use size_t to hold the buffer size inside GDB and __u64 when interfacing
> the
> > Linux kernel.
>
> Please don't propagate the use of Linux kernel types outside of the
> linux kernel.  The ISO C spelling of __u64 is uint64_t.

I restrict the use of __u64 to the lowest layer that deals with the kernel
interface.  Higher layers use GDB types.

The problem is that on 64-bit systems __u64 is defined unsigned long long,
whereas uint64_t is defined unsigned long.  GCC complains when I store
the address of a __u64 variable in a uint64_t * variable.


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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] ari, btrace: avoid unsigned long long

Pedro Alves-7
In reply to this post by Metzger, Markus T
On 07/13/2015 03:47 PM, Markus Metzger wrote:

> diff --git a/gdb/common/btrace-common.c b/gdb/common/btrace-common.c
> index 95193eb..9d6c4a7 100644
> --- a/gdb/common/btrace-common.c
> +++ b/gdb/common/btrace-common.c
> @@ -155,10 +155,10 @@ btrace_data_append (struct btrace_data *dst,
>    dst->variant.pt.size = 0;
>  
>    /* fall-through.  */
> - case BTRACE_FORMAT_BTS:
> + case BTRACE_FORMAT_PT:

Looks like an unrelated change that should be pushed separately.

>    {
>      gdb_byte *data;
> -    unsigned long size;
> +    size_t size;
>  



>    tinfo = xzalloc (sizeof (*tinfo));
> @@ -674,28 +676,36 @@ 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
> +    + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);

Per, http://www.gnu.org/prep/standards/standards.html#Formatting
wrap with ()s so that the second line aligns correctly:

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

>    /* 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
> +    + ((conf->size % PAGE_SIZE) == 0 ? 0 : 1);

Likewise.

Otherwise looks good to me.

Thanks,
Pedro Alves