From: "Metzger, Markus T" <markus.t.metzger@intel.com>
To: Pedro Alves <palves@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH v2 2/2] ari, btrace: avoid unsigned long long
Date: Mon, 13 Jul 2015 07:20:00 -0000 [thread overview]
Message-ID: <A78C989F6D9628469189715575E55B233316BEC8@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <559FD11C.3080001@redhat.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
next prev parent reply other threads:[~2015-07-13 7:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-09 6:09 [PATCH v2 1/2] record: set stop_pc in "record goto" command Markus Metzger
2015-07-09 6:08 ` [PATCH v2 2/2] ari, btrace: avoid unsigned long long Markus Metzger
2015-07-09 11:15 ` Pedro Alves
2015-07-10 7:17 ` Metzger, Markus T
2015-07-10 14:05 ` Pedro Alves
2015-07-13 7:20 ` Metzger, Markus T [this message]
2015-07-13 9:35 ` Pedro Alves
2015-07-09 10:38 ` [PATCH v2 1/2] record: set stop_pc in "record goto" command Pedro Alves
2015-07-13 7:39 ` Metzger, Markus T
2015-07-13 9:36 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=A78C989F6D9628469189715575E55B233316BEC8@IRSMSX104.ger.corp.intel.com \
--to=markus.t.metzger@intel.com \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).