From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76331 invoked by alias); 13 Jul 2015 07:20:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 76320 invoked by uid 89); 13 Jul 2015 07:20:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_40,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mga11.intel.com Received: from mga11.intel.com (HELO mga11.intel.com) (192.55.52.93) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 13 Jul 2015 07:20:26 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP; 13 Jul 2015 00:20:16 -0700 X-ExtLoop1: 1 Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by orsmga002.jf.intel.com with ESMTP; 13 Jul 2015 00:20:14 -0700 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.171]) by irsmsx110.ger.corp.intel.com ([163.33.3.25]) with mapi id 14.03.0224.002; Mon, 13 Jul 2015 08:20:13 +0100 From: "Metzger, Markus T" To: Pedro Alves CC: "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 Message-ID: References: <1436422132-8936-1-git-send-email-markus.t.metzger@intel.com> <1436422132-8936-2-git-send-email-markus.t.metzger@intel.com> <559E57C0.90006@redhat.com> <559FD11C.3080001@redhat.com> In-Reply-To: <559FD11C.3080001@redhat.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00362.txt.bz2 > -----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. >=20 > 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. >=20 > (build tested on 64-bit and 32-bit, but otherwise not > tested; I'm still with the machine that doesn't do btrace.) >=20 > 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. >=20 > len =3D strlen (body_text); > size =3D len / 2; >=20 > - if ((size_t) size * 2 !=3D len) > + if (size * 2 !=3D 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 !=3D 0". > /* Convert the requested size in bytes to pages (rounding up). */ > - pages =3D (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_S= IZE; > + pages =3D ((size_t) conf->size + PAGE_SIZE - 1) / PAGE_SIZE; Here, we trust that size_t is bigger than unsigned int. I guess this was w= rong with unsigned long, as well. This should better be: pages =3D conf->size / PAGE_SIZE + ((conf->size % PAGE_SIZE) =3D=3D 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; >=20 > size =3D pages * PAGE_SIZE; > - length =3D size + PAGE_SIZE; >=20 > - /* 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; >=20 > + length =3D size + PAGE_SIZE; We still need to check for overflows. This was also wrong before. > if (conf->size =3D=3D 0) > @@ -788,16 +803,16 @@ linux_enable_pt (ptid_t ptid, const struct > btrace_config_pt *conf) > header->aux_offset =3D header->data_offset + header->data_size; >=20 > /* Convert the requested size in bytes to pages (rounding up). */ > - pages =3D (((unsigned long long) conf->size) + PAGE_SIZE - 1) / PAGE_S= IZE; > + pages =3D ((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