From: Walfred Tedeschi <walfred.tedeschi@intel.com>
To: Eli Zaretskii <eliz@gnu.org>
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.
Date: Tue, 30 Sep 2014 14:24:00 -0000 [thread overview]
Message-ID: <542ABD03.7060707@intel.com> (raw)
In-Reply-To: <83vbo5kxal.fsf@gnu.org>
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 <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.
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 = 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 _() ?
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
next prev parent reply other threads:[~2014-09-30 14:24 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-30 7:29 Walfred Tedeschi
2014-09-30 14:17 ` Eli Zaretskii
2014-09-30 14:24 ` Walfred Tedeschi [this message]
2014-09-30 7:39 Walfred Tedeschi
2014-11-23 5:22 ` Joel Brobecker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=542ABD03.7060707@intel.com \
--to=walfred.tedeschi@intel.com \
--cc=eliz@gnu.org \
--cc=gdb-patches@sourceware.org \
--cc=mark.kettenis@xs4all.nl \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).