Re: [PATCH V7] amd64-mpx: initialize bnd register before performing inferior calls.

[Pedro Alves <palves at redhat dot com>]

On 01/31/2017 03:13 PM, Walfred Tedeschi wrote:
> This patch initializes the bnd registers before executing the inferior
> call.  BND registers can be in arbitrary values at the moment of the
> inferior call.  In case the function being called uses as part of the
> parameters bnd register, e.g. when passing a pointer as parameter, the
> current value of the register will be used.  This can cause boundary
> violations that are not due to a real bug or even desired by the user.
> In this sense the best to be done is set the bnd registers to allow
> access to the whole memory, i.e. initialized state, before pushing the
> inferior call.

This explains the reason for clearing better ...

> +
> +  /* When MPX is enabled, all bnd registers have to be initialized
> +     before the call.  This avoids an undesired bound violation
> +     during the function's execution.  */
> +void
> +i387_reset_bnd_regs (struct gdbarch *gdbarch, struct regcache *regcache)

... than this, IMO.  The comment in the code doesn't talk about
"arbitrary values", for example.  In any case, this comment should be next to
the infcall code in question, not here, since it won't make sense for
any other call site that decided to call this function in the future,
unrelated to inferior function calls.  Note how "the call" is
assuming this is talking about an infcall, but that's only clear because
we have the context of the patch; it won't be clear to anyone reading
the code after if is merged.

Also, comment is oddly indented to two spaces too much.

> +/* Set all bnd registers to the INIT state.  INIT state means
> +   all memory range can be accessed.  */
> +extern void i387_reset_bnd_regs (struct gdbarch *gdbarch,
> +			         struct regcache *regcache);

s/all memory range/all memory/  I think.

> 
> 2017-01-12  Walfred Tedeschi <walfred.tedeschi@intel.com>
> 
> gdb/ChangeLog:
> 
> 	* i387-tdep.h (i387_reset_bnd_regs): Add function definition.
> 	* i387-tdep.c (i387_reset_bnd_regs): Add function implementation.

s/Add/New/

> 	* i386-tdep.c (i386_push_dummy_call): Call i387_reset_bnd_regs.
> 	* amd64-tdep (amd64_push_dummy_call): Call i387_reset_bnd_regs.
> 

>  #endif /* i387-tdep.h */


> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-call.c b/gdb/testsuite/gdb.arch/i386-mpx-call.c
> new file mode 100644
> index 0000000..896e63d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-call.c
> @@ -0,0 +1,106 @@
> +/* Test for inferior function calls MPX context.
...
> +
> +#include <stdio.h>

Do we need to include stdio.h?  Would stdlib.h instead do?

> +#include "x86-cpuid.h"
> +
> +#define OUR_SIZE    5

Should this gain a describing comment?   Might not be
clear what this is about.

> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
> +    [list debug nowarnings additional_flags=${comp_flags}]] } {

Why "nowarnings" ?

> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +gdb_test_multiple "print have_mpx ()" "have mpx" {
> +    -re ".*= 1\r\n$gdb_prompt " {
> +        pass "check whether processor supports MPX"
> +    }
> +    -re ".*= 0\r\n$gdb_prompt " {
> +        untested "processor does not support MPX; skipping tests"
> +        return
> +    }
> +}
> +
> +# Needed by the return command.
> +gdb_test_no_output "set confirm off"
> +
> +set bound_reg " = \\\{lbound = $hex, ubound = $hex\\\}.*"
> +
> +set break "bkpt 1."
> +gdb_breakpoint [gdb_get_line_number "${break}"]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p upper (x, a, b, c, d, 0)" " = 1"\
> +    "test the call of int function - int"
> +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
> +    " = \\\(int \\\*\\\) $hex" "test the call of function - ptr"

All tests test something, so the "test the" is redundant.
Also doesn't "int function - int" have a redundant "int" ?

Thanks,
Pedro Alves