This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] 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: palves at redhat dot com, mark dot kettenis at xs4all dot nl, gdb-patches at sourceware dot org, walfred dot tedeschi at intel dot com
- Date: Tue, 30 Sep 2014 17:17:22 +0300
- Subject: Re: [PATCH] Add support for bound table in the Intel MPX context.
- Authentication-results: sourceware.org; auth=none
- References: <1412062148-22808-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: Tue, 30 Sep 2014 09:29:08 +0200
>
> In order to investigate the contents of the MPX boundary table two new commands
> are added to GDB. "mpx-info-bounds" and "mpx-set-bounds" are used to display
> 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.
> 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).
> --- 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?
> +@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.
> +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 = 0;
> + CORE_ADDR addr, lower, upper;
> + CORE_ADDR bt_entry_addr = 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 = strtok (args, " ");
> + if (!addr_str)
> + error ("Missing address of the pointer in the command.");
> +
> + lower_str = strtok (NULL, " ");
> + if (!lower_str)
> + error ("Missing lower bound in the command.");
> +
> + upper_str = strtok (NULL, " ");
> + if (!upper_str)
> + error ("Missing upper bound in the command.");
> +
> + addr = parse_and_eval_address (addr_str);
> + lower = parse_and_eval_address (lower_str);
> + upper = parse_and_eval_address (upper_str);
> +
> + bd_base = i386_mpx_bd_base ();
> + bt_entry_addr = i386_mpx_get_bt_entry (addr, bd_base);
> +
> + ret = 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_addr);
> +
> + bt_entry[0] = (uint64_t) lower;
> + bt_entry[1] = ~(uint64_t) upper;
> +
> + ret = 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 _() ?