From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17288 invoked by alias); 30 Sep 2014 14:24:35 -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 17272 invoked by uid 89); 30 Sep 2014 14:24:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.5 required=5.0 tests=AWL,BAYES_50,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; Tue, 30 Sep 2014 14:24:34 +0000 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 30 Sep 2014 07:24:07 -0700 X-ExtLoop1: 1 Received: from wtedesch-mobl2.ger.corp.intel.com (HELO [172.28.205.47]) ([172.28.205.47]) by fmsmga002.fm.intel.com with ESMTP; 30 Sep 2014 07:24:04 -0700 Message-ID: <542ABD03.7060707@intel.com> Date: Tue, 30 Sep 2014 14:24:00 -0000 From: Walfred Tedeschi User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Eli Zaretskii CC: palves@redhat.com, mark.kettenis@xs4all.nl, gdb-patches@sourceware.org Subject: Re: [PATCH] Add support for bound table in the Intel MPX context. References: <1412062148-22808-1-git-send-email-walfred.tedeschi@intel.com> <83vbo5kxal.fsf@gnu.org> In-Reply-To: <83vbo5kxal.fsf@gnu.org> Content-Type: text/plain; charset="iso-8859-15"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00898.txt.bz2 Am 9/30/2014 4:17 PM, schrieb Eli Zaretskii: Eli, Thank you very much for your prompt review. See my answers below: >> From: Walfred Tedeschi >> Cc: gdb-patches@sourceware.org, Walfred Tedeschi >> Date: Tue, 30 Sep 2014 09:29:08 +0200 >> >> In order to investigate the contents of the MPX boundary table two new c= ommands >> are added to GDB. "mpx-info-bounds" and "mpx-set-bounds" are used to di= splay >> and set values on the MPX bound table. > Thanks. > >> * NEWS: List new commands for MPX support. > I don't see this part in the changeset you've sent. I think it has got lost will add it! >> doc: >> * gdb.texinfo: Add documentation about "mpx-info-bounds" >> and "mpx-set-bounds" > Please state the name of the node in which you make the changes (as if > it were a function). Ok! >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -21482,6 +21482,16 @@ be returned in a register. >> @kindex show struct-convention >> Show the current setting of the convention to return @code{struct}s >> from functions. >> + >> +@item mpx-info-bound @var{pointer storage} > Shouldn't this go into the next subsection, which describes features > specific to the MPX support? Yes it should! Will move. >> +@kindex mpx-info-bound >> +Displays the bounds of a pointer given its storage. > What is a "pointer's storage"? We should explain that here, otherwise > this text is entirely unclear. I also think that we should say a bit > more about the bounds, so that readers will understand what this > feature is about and how to use it to their advantage. > >> +@item mpx-set-bound @var{storage} @var{lbound} @var{ubound} >> +@kindex mpx-set-bound >> +Set the bounds of a pointer in the map given its storage. This command = takes > ^^ > Two spaces between sentences, please. Sorry! >> +three parameters @var{storage} is the pointers storage and @var{lbound}= and > Please add a colon ":" after "parameters", and a comma "," between > "storage" and "and". > >> +@var{ubound} are lower and upper bounds new values respectivelly. > "are the new values for the lower and the upper bound, respectively" > > (Only 1 'l' in "respectively".) > >> +static void >> +i386_mpx_set_bounds (char *args, int from_tty) >> +{ >> + CORE_ADDR bd_base =3D 0; >> + CORE_ADDR addr, lower, upper; >> + CORE_ADDR bt_entry_addr =3D 0; >> + CORE_ADDR bt_entry[4]; >> + int ret; >> + char *addr_str, *lower_str, *upper_str, *tmp; >> + >> + if (!i386_mpx_enabled ()) >> + error ("MPX not supported on this target."); >> + >> + if (!args) >> + error ("Address of pointer variable expected."); >> + >> + addr_str =3D strtok (args, " "); >> + if (!addr_str) >> + error ("Missing address of the pointer in the command."); >> + >> + lower_str =3D strtok (NULL, " "); >> + if (!lower_str) >> + error ("Missing lower bound in the command."); >> + >> + upper_str =3D strtok (NULL, " "); >> + if (!upper_str) >> + error ("Missing upper bound in the command."); >> + >> + addr =3D parse_and_eval_address (addr_str); >> + lower =3D parse_and_eval_address (lower_str); >> + upper =3D parse_and_eval_address (upper_str); >> + >> + bd_base =3D i386_mpx_bd_base (); >> + bt_entry_addr =3D i386_mpx_get_bt_entry (addr, bd_base); >> + >> + ret =3D target_read_memory (bt_entry_addr, (gdb_byte *) bt_entry, >> + sizeof (bt_entry)); >> + if (ret) >> + error ("Cannot read bounds table entry at 0x%lx", (long) bt_entry_a= ddr); >> + >> + bt_entry[0] =3D (uint64_t) lower; >> + bt_entry[1] =3D ~(uint64_t) upper; >> + >> + ret =3D target_write_memory (bt_entry_addr, (gdb_byte *) bt_entry, >> + sizeof (bt_entry)); >> + >> + if (ret) >> + error ("Cannot write bounds table entry at 0x%lx", (long) bt_entry_= addr); >> + else >> + i386_mpx_print_bounds (bt_entry); >> +} > Why aren't error messages in this function inside _() ? Have to fix it! Thanks a lot again! For the next review you will see all those changes addressed. Best regards, -Fred Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052