This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3] Add support for bound table in the Intel MPX context.
- From: Eli Zaretskii <eliz at gnu dot org>
- To: Walfred Tedeschi <walfred dot tedeschi at intel dot com>
- Cc: brobecker at adacore dot com, gdb-patches at sourceware dot org, walfred dot tedeschi at intel dot com
- Date: Mon, 08 Jun 2015 19:44:49 +0300
- Subject: Re: [PATCH v3] Add support for bound table in the Intel MPX context.
- Authentication-results: sourceware.org; auth=none
- References: <1433779588-483-1-git-send-email-walfred dot tedeschi at intel dot com>
- Reply-to: Eli Zaretskii <eliz at gnu dot org>
> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Cc: gdb-patches@sourceware.org, Walfred Tedeschi <walfred.tedeschi@intel.com>
> 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.