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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----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