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

Metzger, Markus T
2015-07-13T07:20:27Z

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