Re: [PATCH] Add support for bound table in the Intel MPX context.

[Joel Brobecker <brobecker at adacore dot com>]

> 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.
> 
> 2014-08-12  Mircea Gherzan  <mircea.gherzan@intel.com>
> 	    Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* i386-tdep.c (MPX_BASE_MASK, MPX_BD_MASK, MPX_BT_MASK, MPX_BD_MASK_32,
> 	MPX_BT_MASK_32): New macros.
> 	(i386_mpx_set_bounds): New function that implements
> 	the "mpx set-bounds" command.
> 	(i386_mpx_enabled) Helper function to test MPX availability.
> 	(i386_mpx_bd_base) Helper function to calculate the base directory
> 	address. (i386_mpx_get_bt_entry) Helper function to access a bound
> 	table entry. (i386_mpx_print_bounds) Effectively display bound
> 	information. (_initialize_i386_tdep): Add new commands
> 	to commands list.
> 	* NEWS: List new commands for MPX support.
> 
> testsuite:
> 
> 	* gdb.arch/i386-mpx-map.c: New file.
> 	* gdb.arch/i386-mpx-map.exp: New File.
> 
> doc:
> 	* gdb.texinfo: Add documentation about "mpx-info-bounds"
> 	and "mpx-set-bounds"

Sorry again for the late review. In the future, please give us a couple
of weeks to review, and then feel free to ping us weekly afterwards.

For the new commands, since they are mpx-specific, I would prefer
if they were prefixed, rather than in the global "domain". I don't
know how the other maintainers feel about that, but I'd rather keep
non-prefixed commands to commands that are indeed general.

I would like to propose...
    show mpx bound
    set mpx bound lbound ubound
... but the fact that setting the mpx bounds takes 2 arguments,
this is somewhat of a departure from the typical "set" command.
So I sent an RFC to discuss this separately:
https://www.sourceware.org/ml/gdb-patches/2014-11/msg00550.html
Please do not change your code until we have a resolution of
the above.

Comments on the code below...

> +/* Show the MPX bounds for the given array/pointer.  */
> +
> +#define MPX_BASE_MASK (~(ULONGEST)0xfff)

Space after "(ULONGEST)".

> +
> +static unsigned long
> +i386_mpx_bd_base (void)

All new functions should have an introductory comment. We decided to
make no exceptions, even for obvious functions. However, for functions
that implement a given interface, a simple one-line comment saying so
suffices. Eg:

    /* Implement the "mpx-set-bound" command.  */

Or:

    /* Implement the "frame_align" gdbarch method.  */

Or:

    /* Implement the "to_open" target_ops method.  */

The idea in the last couple of examples is that the expected behavior
of that routine is expected to match what the gdbarch/target_ops vector
documents. So, no need to duplicate it (which makes also changing the
interface less labour intensive, as we only need to adjust the doco
at a single location).

Can you add those to all new functions you're defining here?

> +  struct regcache *rcache;
> +  struct gdbarch_tdep *tdep;
> +  ULONGEST ret;
> +  enum register_status stat;
> +  struct gdb_exception except;
> +
> +  rcache = get_current_regcache ();
> +  tdep = gdbarch_tdep (get_regcache_arch (rcache));
> +
> +  stat = regcache_raw_read_unsigned (rcache, tdep->bndcfgu_regnum, &ret);
> +
> +  if (stat != REG_VALID)
> +    error (_("BNDCFGU register invalid, read status %d."), stat);
> +
> +  return ret & MPX_BASE_MASK;
> +}
> +
> +/* Check if the current target is MPX enabled.  */
> +
> +int
> +i386_mpx_enabled (void)

Can you explain why this function is made global rather than static?

> +/* Pure pointer math for 64 bit address translation.  */
> +static CORE_ADDR
> +i386_mpx_get_bt_entry (CORE_ADDR ptr, CORE_ADDR bd_base)

Another GDB Coding Style rule (sorry...):
Can you always have an empty line between a function's documentation
and the function's start?

Also, can you adjust the comment to actually say what it does,
rather than talk about how it does it?

> +{
> +  ULONGEST offset1;
> +  ULONGEST offset2;
> +  ULONGEST mpx_bd_mask, bd_ptr_r_shift, bd_ptr_l_shift;
> +  ULONGEST bt_mask, bt_select_r_shift, bt_select_l_shift;
> +  CORE_ADDR bd_entry_addr;
> +  CORE_ADDR bt_addr;
> +  CORE_ADDR bd_entry;
> +
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  int ret = 0;

Any reason for the empty space above? As it turns out, it tricked
me into thinking that the gdbarch assignment wasn't also a variable
declaration, and so I thought that "ret" was declared in the middle
of code, which is currently not allowed.

> +  ret = target_read_memory (bd_entry_addr, (gdb_byte *) &bd_entry,
> +			    sizeof (bd_entry));

That looks suspicious - I think you're making the assumption that
host and target have the same endianness. To read a pointer from
inferior memory, you have read_memory_typed_address which should
be doing what you are looking for.

> +
> +  if (ret)
> +    error (_("Cannot read bounds directory entry at 0x%lx."),
> +	   (long) bd_entry_addr);
> +
> +  if ((bd_entry & 0x1) == 0)
> +    error (_("Invalid bounds directory entry at 0x%lx."),
> +	   (long) bd_entry_addr);

If you use read_memory_typed_address, then the first error message
should disappear. But if not, then use %s combined with paddress
in both error messages to format CORE_ADDR values.

> +static void
> +i386_mpx_info_bounds (char *args, int from_tty)
> +{
> +  CORE_ADDR bd_base = 0;
> +  CORE_ADDR addr;
> +  CORE_ADDR bt_entry_addr = 0;
> +  CORE_ADDR bt_entry[4];
> +  int ret;
> +
> +  if (!i386_mpx_enabled ())
> +    error (_("MPX not supported on this target."));
> +
> +  if (!args)
> +    error (_("Address of pointer variable expected."));

For pointer comparisons, the GDB project uses explicit comparisons.
So can you replace "!args" by "args != NULL"?

> +  addr = parse_and_eval_address (args);
> +
> +  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));

Same as above - I think there is an endianness issue, here.

> +  if (ret)
> +    error (_("Cannot read bounds table entry at %lx."), bt_entry_addr);

If you decide to implement the read above such that you keep
the error message, then use paddress to print the address.

> +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.");

Missing i18n marker.

> +
> +  if (!args)
> +    error ("Address of pointer variable expected.");

args != NULL again.
Missing i18n marker. There are other instances of these below,
but it's unclear if that code is going to stay. So I will allow
myself to skip mentioning those, but could you please make sure
you fix them all?

> +  addr_str = strtok (args, " ");

Please use gdb_buildargv to parse the arguments instead of doing it
yourself via strtok. Besides, those function modify its argument,
which means your function has an unexpected side-effect, and it
would also stop working the day we make "args" parameter a const.
I suspect this is also going to simplify a bit the verification
of the number of arguments.

> +  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);

Same as before regarding reading an address from inferior memory.

> +  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));

Same thing here. You'll need to convert the address to target
endianness first, and then write that.

> +  if (ret)
> +    error ("Cannot write bounds table entry at 0x%lx", (long) bt_entry_addr);
> +  else
> +    i386_mpx_print_bounds (bt_entry);

Personally, I would not re-print the bounds. If the user really wants
to confirm his operation, I'd let him call the command to show it
again.

> diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
> index e0950a3..42da850 100644
> --- a/gdb/i386-tdep.h
> +++ b/gdb/i386-tdep.h
> @@ -434,4 +434,7 @@ extern int i386_stap_is_single_operand (struct gdbarch *gdbarch,
>  extern int i386_stap_parse_special_token (struct gdbarch *gdbarch,
>  					  struct stap_parse_info *p);
>  
> +int
> +i386_mpx_enabled (void);

This is related to my question asking whether you really need this
function to be non-static. If the answer is yes, for some reason,
The function's name should not be be on the first column of the next
line (this is reserved for function implementations, in order to
facilitate locating them via grep). So:

int i386_mpx_enabled (void);

> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-map.c b/gdb/testsuite/gdb.arch/i386-mpx-map.c
> new file mode 100644
> index 0000000..441131a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-map.c
[...]
> +#include <stdio.h>

Is stdio.h really needed here? Using headers such as this one make
it much harder to run this test on bare-metal targets.

> +#include <stdlib.h>
> +
> +#include "x86-cpuid.h"
> +
> +#ifndef NOINLINE
> +#define NOINLINE __attribute__ ((noinline))
> +#endif
> +
> +#define SIZE  5
> +
> +typedef int T;
> +
> +unsigned int have_mpx (void) NOINLINE;
> +
> +unsigned int NOINLINE
> +have_mpx (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +	return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ebx & bit_MPX) == bit_MPX)
> +	return 1;
> +      else
> +	return 0;
> +    }
> +  return 0;
> +}
> +
> +void
> +foo (T * p)

No space between "*" and "p", please.

> +{
> +  T *x;

Empty line after the end of the local variable declarations.

> +#if defined  __GNUC__ && !defined __INTEL_COMPILER
> +  __bnd_store_ptr_bounds (p, &p);
> +#endif
> +  x = p + SIZE - 1;
> +#if defined  __GNUC__ && !defined __INTEL_COMPILER
> +  __bnd_store_ptr_bounds (x, &x);
> +#endif
> +  return;			/* after-assign */
> +}
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      T *a = NULL;

Same here, please (empty line).

> +      a = calloc (SIZE, sizeof (T));	/* after-decl */
> +#if defined  __GNUC__ && !defined __INTEL_COMPILER
> +      __bnd_store_ptr_bounds (a, &a);
> +#endif
> +      foo (a);				/* after-alloc */
> +      free (a);
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-map.exp b/gdb/testsuite/gdb.arch/i386-mpx-map.exp
> new file mode 100644
> index 0000000..539e1dd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-map.exp
> @@ -0,0 +1,80 @@
> +# Copyright 2014 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp. <mircea.gherzan@intel.com>,
> +#                            <walfred.tedeschi@intel.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-fmpx -I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> + [list debug nowarnings additional_flags=${comp_flags}]] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +	untested "could not run to main"
> +	return -1
> +}
> +
> +set supports_mpx 0
> +set test "probe MPX support"
> +send_gdb "print have_mpx ()\r"
> +
> +gdb_test_multiple "print have_mpx()" $test {
> +    -re ".. = 1\r\n$gdb_prompt $" {
> +        pass $test
> +        set supports_mpx 1
> +    }
> +    -re ".. = 0\r\n$gdb_prompt $" {
> +        pass $test
> +    }
> +}

I think there is a problem here, and you're probably sending
the "print have_mpx()" command twice. Please remove the call
to "send_gdb" which we try to avoid as much as possible", and
see if the test still works. The part that really surprises me
is that you'd think that this could cause synchronization between
sent commands and expected output in the rest of the testcase
to be off by one command. How did it still work?

> +if { !$supports_mpx } {
> +    unsupported "processor does not support MPX"
> +    return
> +}
> +
> +gdb_breakpoint [ gdb_get_line_number "after-decl" ]
> +gdb_breakpoint [ gdb_get_line_number "after-alloc" ]
> +gdb_breakpoint [ gdb_get_line_number "after-assign" ]
> +
> +gdb_test "mpx-info-bounds 0x0" "Invalid bounds directory entry at 0x\[0-9a-fA-F\]+." "NULL address of the pointer"

You can use $hex for matching addreses. I'll let you adjust the rest
of this file without explicitly pointing out the locations where you
can do so...

> +gdb_continue_to_breakpoint "after-decl" ".*after-decl.*"
> +#gdb_test "mpx-info-bounds a" "Invalid bounds directory entry at 0x\[0-9a-fA-F\]+." "pointer instead of pointer address"
> +# Fix size

Please do not introduce commented-out code, at least not without
explicitly explaining why.

> +gdb_test "mpx-info-bounds &a" "Null bounds on map: pointer value = 0x0+." "address of a NULL pointer"

You might want to escape the last dot, if you want to make sure
that value is 0.

> +gdb_test "mpx-info-bounds &x" "\\\{lbound = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\}: pointer value = 0x\[0-9a-fA-F\]+, size = -1, metadata = 0x0+." "pointer after assignment"

Same here for the last dot.

-- 
Joel