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

["Tedeschi, Walfred" <walfred.tedeschi@intel.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