From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100408 invoked by alias); 8 Jun 2015 16:45:19 -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 100398 invoked by uid 89); 8 Jun 2015 16:45:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_NONE,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mtaout20.012.net.il Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Jun 2015 16:45:14 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0NPM00C00WXLZ000@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Mon, 08 Jun 2015 19:45:04 +0300 (IDT) Received: from HOME-C4E4A596F7 ([87.69.4.28]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0NPM00C7FX73R1B0@a-mtaout20.012.net.il>; Mon, 08 Jun 2015 19:45:03 +0300 (IDT) Date: Mon, 08 Jun 2015 16:45:00 -0000 From: Eli Zaretskii Subject: Re: [PATCH v3] Add support for bound table in the Intel MPX context. In-reply-to: <1433779588-483-1-git-send-email-walfred.tedeschi@intel.com> To: Walfred Tedeschi Cc: brobecker@adacore.com, gdb-patches@sourceware.org, walfred.tedeschi@intel.com Reply-to: Eli Zaretskii Message-id: <83r3pmkv3y.fsf@gnu.org> References: <1433779588-483-1-git-send-email-walfred.tedeschi@intel.com> X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00117.txt.bz2 > From: Walfred Tedeschi > Cc: gdb-patches@sourceware.org, Walfred Tedeschi > Date: Mon, 8 Jun 2015 18:06:28 +0200 > > Intel(R) Memory protection bound information are located in register > to be tested using the MPX new instructions. Since the number of > bound registers are limited a table is used to provide storage for > bounds during run-time. > > In order to investigate the contents of the MPX bound table two new > commands are added to GDB. "show mpx bound" and "set mpx bound" are > used to display and set values on the MPX bound table. Thanks. > doc: > * gdb.texinfo: Add documentation about "show mpx bound" > and "set mpx bound". This log entry should mention the name(s) of the node(s) in which you made changes. > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -90,6 +90,11 @@ tui enable > tui disable > Explicit commands for enabling and disabling tui mode. > > +show mpx bound > +set mpx bound on i386 and amd64 > + for Intel(R) MPX. Support for bound table investigation on > + Intel(R) MPX enabled applications. The "for Intel(R) MPX." part is redundant, and is also an incomplete sentence. I suggest to remove it. > +Bounds can also be stored in bounds tables, which are stored in > +application memory. Pointers bounds are stored into the bounds table > +by means of the pointers location address. I suggest to rephrase the last sentence, so as to avoid the use of passive tense: These tables store bounds for pointers by specifying an address of a buffer along with its bounds. Also, please make sure to leave 2 spaces between sentences, not 1. > Evaluating and changing bounds > +located in bound tables is therefore interesting while investigating bugs > +on MPX context. New commands are then added for this purpose: ^^^^ ^^^^^^^^^^^^^^ "bugs related to MPX" sounds better, I think. Also, these "new commands" will cease being new with time, so I suggest to say simply "@value{GDBN} provides commands for this purpose." > +@item show mpx bound @var{pointer} > +@kindex show mpx bound > +Displays the bounds of a pointer @var{pointer}. ^^^^^^^^ "Display", to be consistent with our style of describing commands. Also, you can simply say "Display the bounds of the given @var{pointer}.", there's no need to repeat the word. > +@item set mpx bound @var{pointer}, @var{lbound}, @var{ubound} > +@kindex set mpx bound > +Set the bounds of a pointer in the map. What map? You didn't mention any maps before. > +This command takes three parameters: @var{pointer} is the pointers "@var{pointer} is the pointer whose bounds are to be changed" > +value, @var{lbound} and @var{ubound}, are new values for lower > +and upper bounds respectively. ^ No need for that comma. The documentation parts are okay with those fixes. > + stat = regcache_raw_read_unsigned (rcache, tdep->bndcfgu_regnum, &ret); > + > + if (stat != REG_VALID) > + error (_("BNDCFGU register invalid, read status %d."), stat); Is it wise to call a variable by the name of a widely used function? Some peculiar environments might have a macro that redirects the call to a replacement function (e.g., Gnulib might use rpl_stat), which might cause trouble here. IMO, it is safer to use a different name. > + error (_("Wrong number of arguments; Missing lower and upper bound.")); Please use a colon ':' rather than a semi-colon ';'. Also, "Missing" shouldn't be capitalized. > + add_cmd ("bound", no_class, i386_mpx_info_bounds, > + "show the memory bounds for a given array/pointer storage\ "Show", with a capital S. > + add_cmd ("bound", no_class, i386_mpx_set_bounds, > + "set the memory bounds for a given array/pointer storage\ "Set", likewise. Thanks.