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

["Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>]

Pedro,

Thanks a lot for your review!

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 ...
Yes, it was my intention. Do you see value to have the patch in then?
> > +
> > +  /* 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.
Agreed, I will change the comments and explanations!
> > +/* 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.
Yes! Thanks!
> > 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?
I will try to eliminate this dependency.
> > +#include "x86-cpuid.h"
> > +
> > +#define OUR_SIZE    5
> Should this gain a describing comment?   Might not be
> clear what this is about.
Yes! Better naming and a comment should help.
> > +
> > +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" ?
I will also modify it! (Old commit)

> > +    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" ?
Will change as well!
> Thanks,
> Pedro Alves

Thanks again!
Best regards,
/Fred
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928