[PATCH V5 5/5] ntel MPX bound violation handling
Walfred Tedeschi
walfred.tedeschi@intel.com
Tue Feb 2 10:09:00 GMT 2016
Am 1/28/2016 um 2:42 PM schrieb Pedro Alves:
Pedro,
I have tried to address all your comments.
For the tests I also verified that there was only one line in the log as
shown in the wiki.
Have though some comments below:
> "Intel" in $subject.
>
> On 01/22/2016 02:54 PM, Walfred Tedeschi wrote:
>> With Intel Memory Protection Extensions it was introduced the concept of
>> boundary violation. A boundary violations is presented to the inferior as
>> a segmentation fault having SIGCODE 3. This patch adds a
>> handler for a boundary violation extending the information displayed
>> when a bound violation is presented to the inferior. In the stop mode
>> case the debugger will also display the kind of violation: "upper" or
>> "lower", bounds and the address accessed.
>> On no stop mode the information will still remain unchanged. Additional
>> information about bound violations are not meaningful in that case user
>> does not know the line in which violation occurred as well.
>>
>> When the segmentation fault handler is stop mode the out puts will be
>> changed as exemplified below.
>>
>> The usual output of a segfault is:
>> Program received signal SIGSEGV, Segmentation fault
>> 0x0000000000400d7c in upper (p=0x603010, a=0x603030, b=0x603050,
>> c=0x603070, d=0x603090, len=7) at i386-mpx-sigsegv.c:68
>> 68 value = *(p + len);
>>
>> In case it is a bound violation it will be presented as:
>> Program received signal SIGSEGV, Segmentation fault
>> Upper bound violation while accessing address 0x7fffffffc3b3
>> Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3]
>> 0x0000000000400d7c in upper (p=0x603010, a=0x603030, b=0x603050,
>> c=0x603070, d=0x603090, len=7) at i386-mpx-sigsegv.c:68
>> 68 value = *(p + len);
>>
>> In mi mode the output of a segfault is:
>> *stopped,reason="signal-received",signal-name="SIGSEGV",
>> signal-meaning="Segmentation fault", frame={addr="0x0000000000400d7c",
>> func="upper",args=[{name="p", value="0x603010"},{name="a",value="0x603030"}
>> ,{name="b",value="0x603050"}, {name="c",value="0x603070"},
>> {name="d",value="0x603090"},{name="len",value="7"}],
>> file="i386-mpx-sigsegv.c",fullname="i386-mpx-sigsegv.c",line="68"},
>> thread-id="1",stopped-threads="all",core="6"
>>
>> in the case of a bound violation:
>> *stopped,reason="signal-received",signal-name="SIGSEGV",
>> signal-meaning="Segmentation fault",
>> sigcode-meaning="Upper bound violation",
>> lower-bound="0x603010",upper-bound="0x603023",bound-access="0x60302f",
>> frame={addr="0x0000000000400d7c",func="upper",args=[{name="p",
>> value="0x603010"},{name="a",value="0x603030"},{name="b",value="0x603050"},
>> {name="c",value="0x603070"},{name="d",value="0x603090"},
>> {name="len",value="7"}],file="i386-mpx-sigsegv.c",
>> fullname="i386-mpx-sigsegv.c",line="68"},thread-id="1",
>> stopped-threads="all",core="6"
>>
>> 2016-01-15 Walfred Tedeschi <walfred.tedeschi@intel.com>
>>
>> gdb/ChangeLog:
>>
>> * NEWS: Add entry for bound violation.
>> * amd64-linux-tdep.c (amd64_linux_init_abi_common):
>> Add handler for bound violation.
>> * gdbarch.sh (bound_violation_handler): New.
>> * gdbarch.c: Regenerate.
>> * gdbarch.h: Regenerate.
>> * i386-linux-tdep.c (i386_mpx_bound_violation_handler): New.
>> (i386_linux_init_abi): Use i386_mpx_bound_violation_handler.
>> * i386-linux-tdep.h (i386_mpx_bound_violation_handler) New.
>> * i386-tdep.c (i386_mpx_enabled): Add as external.
>> * i386-tdep.c (i386_mpx_enabled): Add as external.
>> * infrun.c (handle_segmentation_faults): New function.
>> (print_signal_received_reason): Use handle_segmentation_faults.
>> (normal_stop): Change order of observer in order to have the
>> inferior stopped for evaluation.
>>
>> gdb/testsuite/ChangeLog:
>>
>> * gdb.arch/i386-mpx-sigsegv.c: New file.
>> * gdb.arch/i386-mpx-sigsegv.exp: New file.
>> * gdb.arch/i386-mpx-simple_segv.c: New file.
>> * gdb.arch/i386-mpx-simple_segv.exp: New file.
>>
>> gdb/doc/ChangeLog:
>>
>> * gdb.texinfo (Signals): Add bound violation display hints for
>> a SIGSEGV.
>>
>> ---
>> gdb/NEWS | 15 +++
>> gdb/amd64-linux-tdep.c | 2 +
>> gdb/doc/gdb.texinfo | 25 +++++
>> gdb/gdbarch.c | 32 ++++++
>> gdb/gdbarch.h | 11 +++
>> gdb/gdbarch.sh | 6 ++
>> gdb/i386-linux-tdep.c | 46 +++++++++
>> gdb/i386-linux-tdep.h | 5 +
>> gdb/i386-tdep.c | 4 +-
>> gdb/i386-tdep.h | 2 +
>> gdb/infrun.c | 34 +++++++
>> gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c | 120 +++++++++++++++++++++++
>> gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp | 88 +++++++++++++++++
>> gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c | 66 +++++++++++++
>> gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp | 125 ++++++++++++++++++++++++
>> 15 files changed, 578 insertions(+), 3 deletions(-)
>> create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c
>> create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
>> create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c
>> create mode 100644 gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index d9cbb80..b8512d6 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,21 @@
>>
>> *** Changes since GDB 7.10
>>
>> +* Intel MPX boud violation handler.
>
> Typo, "bound".
>
> I think you may have meant s/handler/handling/.
>
>> +
>> + A boundary violations is presented to the inferior as
>> + a segmentation fault having SIGCODE 3. In this case
>> + GDB displays also the kind of violation (upper or lower),
>> + bounds, poiter value and the memory accessed, besides displaying
>> + the usual signal received and code location report.
>
> I think that talking about sigcode is a bit of implementer-speak.
> Also, a few typos and missing double-spaces.
>
> I don't understand what you mean by "pointer value and the
> memory accessed", specifically, what pointer value is that.
>
>> + As exemplified below:
>> + Program received signal SIGSEGV, Segmentation fault
>> + Upper bound violation while accessing address 0x7fffffffc3b3
>> + Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3].
>> + 0x0000000000400d7c in upper (p=0x603010, a=0x603030, b=0x603050,
>> + c=0x603070, d=0x603090, len=7) at i386-mpx-sigsegv.c:68
>
> The arguments in "upper()" don't add any info here, and are
> causing a line wrap. so I think you should remove them.
>
> The period at the end of:
>
> Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3].
>
> looks odd. That's not a sentence, and the real sentence above
> didn't get a period. I think you should remove it, saving
> some screen estate, and update all examples that might show it,
> here and docs.
>
> Thus, I suggest:
>
> * Intel MPX boundary violations
>
> Segmentation faults caused by Intel MPX boundary violations
> now display the kind of violation (upper or lower), the memory
> address accessed and the memory bounds, along with the usual
> signal received and code location.
>
> For example:
>
> Program received signal SIGSEGV, Segmentation fault
> Upper bound violation while accessing address 0x7fffffffc3b3
> Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3]
> 0x0000000000400d7c in upper () at i386-mpx-sigsegv.c:68
>
>> +
>> * Per-inferior thread numbers
>>
>> Thread numbers are now per inferior instead of global. If you're
>> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
>> index 21bcd99..8af15c4 100644
>> --- a/gdb/amd64-linux-tdep.c
>> +++ b/gdb/amd64-linux-tdep.c
>> @@ -1840,6 +1840,8 @@ amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch)
>> set_gdbarch_process_record_signal (gdbarch, amd64_linux_record_signal);
>>
>> set_gdbarch_get_siginfo_type (gdbarch, x86_linux_get_siginfo_type);
>> + set_gdbarch_bound_violation_handler(gdbarch,
>> + i386_mpx_bound_violation_handler);
>
> Space before parens.
>
>> }
>>
>> static void
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 7da31c8..f4373ed 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -5819,6 +5819,30 @@ $1 = (void *) 0x7ffff7ff7000
>>
>> Depending on target support, @code{$_siginfo} may also be writable.
>>
>> +Also on some targets a @code{SIGSEGV} can be related to
>> +boundary violations, i.e. accessing an address outside of
>> +allowed range. In those cases @value{GDBN} might displays additional
>> +information. In @code{STOP} mode @value{GDBN} displays the violation
>> +kind: "Upper" or "Lower", address accessed and bounds. In @code{NOSTOP}
>> +no additional information is displayed.
>
> I suggest adding a couple cindex's, and the following edit:
>
> @cindex Intel MPX boundary violations
> @cindex boundary violations, Intel MPX
> On some targets, a @code{SIGSEGV} can be caused by a boundary
> violation, i.e., accessing an address outside of the allowed range.
> In those cases @value{GDBN} may displays additional information, depending
> on how @value{GDBN} has been told to handle the signal. In @code{handle stop}
> mode @value{GDBN} displays the violation kind: "Upper" or "Lower", the
> memory address accessed and the bounds. In @code{handle nostop} no
> additional information is displayed.
>
>> +
>> +The usual output of a segfault is:
>> +@smallexample
>> +Program received signal SIGSEGV, Segmentation fault
>> +0x0000000000400d7c in upper (p=0x603010, a=0x603030, b=0x603050,
>> +c=0x603070, d=0x603090, len=7) at i386-mpx-sigsegv.c:68
>> +68 value = *(p + len);
>> +@end smallexample
>
> Remove arguments here too:
>
> The usual output of a segfault is:
>
> @smallexample
> Program received signal SIGSEGV, Segmentation fault
> 0x0000000000400d7c in upper () at i386-mpx-sigsegv.c:68
> 68 value = *(p + len);
> @end smallexample
>
>
>> +
>> +In case it is a bound violation it will be presented as:
>
> "While a bound violation is presented as:"
>
> Note: s/will/is/
>
>> +@smallexample
>> +Upper bound violation while accessing address 0x7fffffffc3b3
>> +Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3]
>> +0x0000000000400d7c in upper (p=0x603010, a=0x603030, b=0x603050,
>> +c=0x603070, d=0x603090, len=7) at i386-mpx-sigsegv.c:68
>> +68 value = *(p + len);
>> +@end smallexample
>
> "Program received ..." is missing. Drop arguments:
>
> @smallexample
> Program received signal SIGSEGV, Segmentation fault
> Upper bound violation while accessing address 0x7fffffffc3b3
> Bounds: [lower = 0x7fffffffc390, upper = 0x7fffffffc3a3]
> 0x0000000000400d7c in upper () at i386-mpx-sigsegv.c:68
> 68 value = *(p + len);
> @end smallexample
>
>> +
>> @node Thread Stops
>> @section Stopping and Starting Multi-thread Programs
>>
>> @@ -22267,6 +22291,7 @@ whose bounds are to be changed, @var{lbound} and @var{ubound} are new values
>> for lower and upper bounds respectively.
>> @end table
>>
>> +
>> @node Alpha
>
> Spurious empty lines?
>
>> @subsection Alpha
>>
>> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
>> index d45af1a..f7fef25 100644
>> --- a/gdb/gdbarch.c
>> +++ b/gdb/gdbarch.c
>> @@ -189,6 +189,7 @@ struct gdbarch
>> int num_pseudo_regs;
>> gdbarch_ax_pseudo_register_collect_ftype *ax_pseudo_register_collect;
>> gdbarch_ax_pseudo_register_push_stack_ftype *ax_pseudo_register_push_stack;
>> + gdbarch_bound_violation_handler_ftype *bound_violation_handler;
>> int sp_regnum;
>> int pc_regnum;
>> int ps_regnum;
>> @@ -531,6 +532,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>> /* Skip verify of num_pseudo_regs, invalid_p == 0 */
>> /* Skip verify of ax_pseudo_register_collect, has predicate. */
>> /* Skip verify of ax_pseudo_register_push_stack, has predicate. */
>> + /* Skip verify of bound_violation_handler, has predicate. */
>> /* Skip verify of sp_regnum, invalid_p == 0 */
>> /* Skip verify of pc_regnum, invalid_p == 0 */
>> /* Skip verify of ps_regnum, invalid_p == 0 */
>> @@ -773,6 +775,12 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>> "gdbarch_dump: bits_big_endian = %s\n",
>> plongest (gdbarch->bits_big_endian));
>> fprintf_unfiltered (file,
>> + "gdbarch_dump: gdbarch_bound_violation_handler_p() = %d\n",
>> + gdbarch_bound_violation_handler_p (gdbarch));
>> + fprintf_unfiltered (file,
>> + "gdbarch_dump: bound_violation_handler = <%s>\n",
>> + host_address_to_string (gdbarch->bound_violation_handler));
>> + fprintf_unfiltered (file,
>> "gdbarch_dump: breakpoint_from_pc = <%s>\n",
>> host_address_to_string (gdbarch->breakpoint_from_pc));
>> fprintf_unfiltered (file,
>> @@ -1986,6 +1994,30 @@ set_gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch,
>> }
>>
>> int
>> +gdbarch_bound_violation_handler_p (struct gdbarch *gdbarch)
>> +{
>> + gdb_assert (gdbarch != NULL);
>> + return gdbarch->bound_violation_handler != NULL;
>> +}
>> +
>> +void
>> +gdbarch_bound_violation_handler (struct gdbarch *gdbarch, struct ui_out *uiout)
>> +{
>> + gdb_assert (gdbarch != NULL);
>> + gdb_assert (gdbarch->bound_violation_handler != NULL);
>> + if (gdbarch_debug >= 2)
>> + fprintf_unfiltered (gdb_stdlog, "gdbarch_bound_violation_handler called\n");
>> + gdbarch->bound_violation_handler (gdbarch, uiout);
>> +}
>> +
>> +void
>> +set_gdbarch_bound_violation_handler (struct gdbarch *gdbarch,
>> + gdbarch_bound_violation_handler_ftype bound_violation_handler)
>> +{
>> + gdbarch->bound_violation_handler = bound_violation_handler;
>> +}
>> +
>> +int
>> gdbarch_sp_regnum (struct gdbarch *gdbarch)
>> {
>> gdb_assert (gdbarch != NULL);
>> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
>> index 3c16af2..eb5de0d 100644
>> --- a/gdb/gdbarch.h
>> +++ b/gdb/gdbarch.h
>> @@ -63,6 +63,7 @@ struct ravenscar_arch_ops;
>> struct elf_internal_linux_prpsinfo;
>> struct mem_range;
>> struct syscalls_info;
>> +struct ui_out;
>>
>> #include "regcache.h"
>>
>> @@ -299,6 +300,16 @@ typedef int (gdbarch_ax_pseudo_register_push_stack_ftype) (struct gdbarch *gdbar
>> extern int gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, struct agent_expr *ax, int reg);
>> extern void set_gdbarch_ax_pseudo_register_push_stack (struct gdbarch *gdbarch, gdbarch_ax_pseudo_register_push_stack_ftype *ax_pseudo_register_push_stack);
>>
>> +/* Function called when a segmentation fault signal is received by the inferior,
>> + having SIGCODE 3 (SIG_CODE_BOUNDARY_FAULT).
>> + UIOUT is the output stream where the handler will place information. */
>> +
>> +extern int gdbarch_bound_violation_handler_p (struct gdbarch *gdbarch);
>> +
>> +typedef void (gdbarch_bound_violation_handler_ftype) (struct gdbarch *gdbarch, struct ui_out *uiout);
>> +extern void gdbarch_bound_violation_handler (struct gdbarch *gdbarch, struct ui_out *uiout);
>> +extern void set_gdbarch_bound_violation_handler (struct gdbarch *gdbarch, gdbarch_bound_violation_handler_ftype *bound_violation_handler);
>> +
>> /* GDB's standard (or well known) register numbers. These can map onto
>> a real register or a pseudo (computed) register or not be defined at
>> all (-1).
>> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>> index f80cd51..edd155a 100755
>> --- a/gdb/gdbarch.sh
>> +++ b/gdb/gdbarch.sh
>> @@ -446,6 +446,11 @@ M:int:ax_pseudo_register_collect:struct agent_expr *ax, int reg:ax, reg
>> # Return -1 if something goes wrong, 0 otherwise.
>> M:int:ax_pseudo_register_push_stack:struct agent_expr *ax, int reg:ax, reg
>>
>> +# Function called when a segmentation fault signal is received by the inferior,
>> +# having SIGCODE 3 (SIG_CODE_BOUNDARY_FAULT).
>
> # Function called when a segmentation fault with
> # SIGCODE 3 (SIG_CODE_BOUNDARY_FAULT) is received by the inferior.
>
> But, see below.
>
>> +# UIOUT is the output stream where the handler will place information.
>> +M:void:bound_violation_handler:struct ui_out *uiout:uiout
>> +
>> # GDB's standard (or well known) register numbers. These can map onto
>> # a real register or a pseudo (computed) register or not be defined at
>> # all (-1).
>> @@ -1247,6 +1252,7 @@ struct ravenscar_arch_ops;
>> struct elf_internal_linux_prpsinfo;
>> struct mem_range;
>> struct syscalls_info;
>> +struct ui_out;
>>
>> #include "regcache.h"
>>
>> diff --git a/gdb/i386-linux-tdep.c b/gdb/i386-linux-tdep.c
>> index af39e78..45212e0 100644
>> --- a/gdb/i386-linux-tdep.c
>> +++ b/gdb/i386-linux-tdep.c
>> @@ -30,6 +30,7 @@
>> #include "i386-tdep.h"
>> #include "i386-linux-tdep.h"
>> #include "linux-tdep.h"
>> +#include "utils.h"
>> #include "glibc-tdep.h"
>> #include "solib-svr4.h"
>> #include "symtab.h"
>> @@ -384,6 +385,49 @@ i386_canonicalize_syscall (int syscall)
>> return gdb_sys_no_syscall;
>> }
>>
>> +void
>> +i386_mpx_bound_violation_handler (struct gdbarch *gdbarch, struct ui_out *uiout)
>
> Missing intro comment. Something like "Implement ... hook." should be enough.
> Look for preexisting examples.
>
>> +{
>> + CORE_ADDR lower_bound, upper_bound, access;
>> + int is_upper;
>> +
>> + if (!i386_mpx_enabled ())
>> + return;
>> + TRY
>> + {
>> + lower_bound
>> + = parse_and_eval_long ("$_siginfo._sifields._sigfault._addr_bnd._lower");
>> + upper_bound
>> + = parse_and_eval_long ("$_siginfo._sifields._sigfault._addr_bnd._upper");
>> + access
>> + = parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr");
>> + }
>> + CATCH (exception, RETURN_MASK_ALL)
>> + {
>> + return;
>> + }
>> + END_CATCH
>> +
>> + is_upper = (access > upper_bound ? 1 : 0);
>> +
>> + ui_out_text (uiout, "\n");
>> + if (is_upper)
>> + ui_out_field_string (uiout, "sigcode-meaning", "Upper bound violation");
>> + else
>> + ui_out_field_string (uiout, "sigcode-meaning", "Lower bound violation");
>> +
>> + ui_out_text (uiout, " while accessing address ");
>> + ui_out_field_fmt (uiout,"bound-access", "%s", paddress (gdbarch, access));
>> + ui_out_text (uiout, "\nBounds: [lower = ");
>> + ui_out_field_fmt (uiout,"lower-bound", "%s", paddress (gdbarch, lower_bound));
>> + ui_out_text (uiout, ", upper = ");
>> + ui_out_field_fmt (uiout,"upper-bound", "%s", paddress (gdbarch, upper_bound));
>> + ui_out_text (uiout, "]");
>
> Missing empty space after a few "uiout," above. Watch out for line-too-long after
> fixing that.
>
>> +
>> +
>> + return;
>
> Remove these two empty lines and the unnecessary "return".
>
>> +}
>> +
>> /* Parse the arguments of current system call instruction and record
>> the values of the registers and memory that will be changed into
>> "record_arch_list". This instruction is "int 0x80" (Linux
>> @@ -1002,6 +1046,8 @@ i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>> i386_linux_get_syscall_number);
>>
>> set_gdbarch_get_siginfo_type (gdbarch, x86_linux_get_siginfo_type);
>> + set_gdbarch_bound_violation_handler(gdbarch,
>> + i386_mpx_bound_violation_handler);
>> }
>>
>> /* Provide a prototype to silence -Wmissing-prototypes. */
>> diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h
>> index b32f6d1..6ffacd4 100644
>> --- a/gdb/i386-linux-tdep.h
>> +++ b/gdb/i386-linux-tdep.h
>> @@ -37,6 +37,11 @@
>> /* Get XSAVE extended state xcr0 from core dump. */
>> extern uint64_t i386_linux_core_read_xcr0 (bfd *abfd);
>>
>> +/* Handles and displays information related to the MPX bound violation
>> + to the user. */
>> +void
>> +i386_mpx_bound_violation_handler (struct gdbarch *gdbarch, struct ui_out *uiout);
>
> Only function definitions should have the function name at collumn 0. All other
> declarations have explicit "extern". Thus, write:
>
> extern void i386_mpx_bound_violation_handler (struct gdbarch *gdbarch,
> struct ui_out *uiout);
>
>> +
>> /* Linux target description. */
>> extern struct target_desc *tdesc_i386_linux;
>> extern struct target_desc *tdesc_i386_mmx_linux;
>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>> index b706463..b5d0d14 100644
>> --- a/gdb/i386-tdep.c
>> +++ b/gdb/i386-tdep.c
>> @@ -8651,9 +8651,7 @@ i386_mpx_bd_base (void)
>> return ret & MPX_BASE_MASK;
>> }
>>
>> -/* Check if the current target is MPX enabled. */
>> -
>> -static int
>> +int
>> i386_mpx_enabled (void)
>> {
>> const struct gdbarch_tdep *tdep = gdbarch_tdep (get_current_arch ());
>> diff --git a/gdb/i386-tdep.h b/gdb/i386-tdep.h
>> index 10d2772..26933f2 100644
>> --- a/gdb/i386-tdep.h
>> +++ b/gdb/i386-tdep.h
>> @@ -420,6 +420,8 @@ extern int i386_process_record (struct gdbarch *gdbarch,
>> struct regcache *regcache, CORE_ADDR addr);
>> extern const struct target_desc *i386_target_description (uint64_t xcr0);
>>
>> +/* Verify if target is MPX enabled. */
>
> Lost "current"? If changing the comment, I'd suggest:
>
> /* Return true iff the current target is MPX enabled. */
>
>> +extern int i386_mpx_enabled (void);
>>
>>
>> /* Functions and variables exported from i386bsd-tdep.c. */
>> diff --git a/gdb/infrun.c b/gdb/infrun.c
>> index 64c729e..87b930e 100644
>> --- a/gdb/infrun.c
>> +++ b/gdb/infrun.c
>> @@ -7893,6 +7893,36 @@ print_exited_reason (struct ui_out *uiout, int exitstatus)
>> }
>> }
>>
>> +/* Value of the sigcode in case of a boundary fault. */
>
> Spurious double-space before "Value".
>
>> +
>> +#define SIG_CODE_BONDARY_FAULT 3
>> +
>> +/* Verifies if a received segmentation fault is a boundary fault.
>> + In the case it is it calls the architecture dependent function
>> + to handle the boundary fault. */
>
>
>> +
>> +static void
>> +handle_segmentation_faults (struct ui_out *uiout)
>
> "handle_segmentation_fault", singular.
>
>> +{
>> + long sig_code = 0;
>> + struct regcache *regcache = get_current_regcache ();
>> + struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +
>> + TRY
>> + {
>> + sig_code = parse_and_eval_long ("$_siginfo.si_code\n");
>
> It's useless to do this on architectures that don't install
> the gdbarch hook. I think this should be pushed down to the
> gdbarch hook. You'll end up with a single try/catch.
> And then the gdbarch hook can be renamed to a more generic
> gdbarch_handle_segmentation_fault. The comments thoughout should be
> updated then, like, for this function:
>
> /* Some targets/architectures can do extra processing/display of
> segmentation faults. E.g., Intel MPX boundary faults.
> Call the architecture dependent function to handle the fault. */
>
> static void
> handle_segmentation_fault (struct ui_out *uiout)
> {
>
>
>> + }
>> + CATCH (exception, RETURN_MASK_ALL)
>> + {
>> + return;
>> + }
>> + END_CATCH
>> +
>> + if (sig_code == SIG_CODE_BONDARY_FAULT
>> + && gdbarch_bound_violation_handler_p (gdbarch))
>> + gdbarch_bound_violation_handler (gdbarch, uiout);
>> +}
>
>
>> +
>> void
>> print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
>> {
>> @@ -7922,6 +7952,10 @@ print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
>> annotate_signal_string ();
>> ui_out_field_string (uiout, "signal-meaning",
>> gdb_signal_to_string (siggnal));
>> +
>> + if (siggnal == GDB_SIGNAL_SEGV)
>> + handle_segmentation_faults (uiout);
>> +
>> annotate_signal_string_end ();
>> }
>> ui_out_text (uiout, ".\n");
>> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c
>> new file mode 100644
>> index 0000000..7500352
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c
>> @@ -0,0 +1,120 @@
>> +/* Copyright (C) 2015 Free Software Foundation, Inc.
>
> 2015-2016
>
>> +++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
>> @@ -0,0 +1,88 @@
>> +# Copyright (C) 2015 Free Software Foundation, Inc.
>
> Ditto.
>
>
>> +gdb_test_multiple "print have_mpx ()" "have mpx" {
>> + -re ".. = 1\r\n$gdb_prompt " {
>> + pass "check whether processor supports MPX"
>
> set message "check whether processor supports MPX"
> gdb_test_multiple "print have_mpx ()" $message {
> -re ".. = 1\r\n$gdb_prompt " {
> pass $message
>
>> + }
>
> Alignment of } looks odd, here and throughout the file.
>
>> +set common_ubound_violation ".*Program received signal SIGSEGV,\
>> + Segmentation fault\r\nUpper bound violation while accessing\
>> + address $hex\r\nBounds: \\\[lower = $hex, upper = $hex\\\]"
>> +
>> +set common_lbound_violation ".*Program received signal SIGSEGV,\
>> + Segmentation fault\r\nLower bound violation while accessing\
>> + address $hex\r\nBounds: \\\[lower = $hex, upper = $hex\\\]"
>
> The initial ".*" is useless.
>
> Use [multi_line ...].
>
>
>> +
>> +set segv_upper_bound "$common_ubound_violation.*$gdb_prompt $"
>> +
>> +set segv_lower_bound "$common_lbound_violation.*$gdb_prompt $"
>> +
>> +for {set i 0} {$i < 15} {incr i} {
>> + set message "MPX signal segv Upper: ${i}"
>> + gdb_test_multiple "continue" "$message ${i}" {
>> + -re $segv_upper_bound {
>> + pass "$message"
>> + }
>> + -re ".*$inferior_exited_re normally.*$gdb_prompt $" {
>> + fail "$message"
>
> The pass/fail calls are missing ${i}. Please make sure test
> messages are unique in gdb.sum:
In fail and pass i think we do not need the ${i} it is added at the
message level already see:
set message "MPX signal segv Upper: ${i}"
>
> https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
>
>> + break
>> + }
>> + }
>> + gdb_test "where" ".*#0 0x\[0-9a-fA-F\]+ in upper.*"\
>> + "$message: should be in upper"
>
> Ditto.
>
> Useless ".*".
>
> Use $hex.
>
>
>> +}
>> +
>> +for {set i 0} {$i < 15} {incr i} {
>> + set message "MPX signal segv Lower: ${i}"
>> + gdb_test_multiple "continue" "$message ${i}" {
>> + -re $segv_lower_bound {
>> + pass "$message ${i}"
>> + }
>> + -re ".*$inferior_exited_re normally.*$gdb_prompt $" {
>> + fail "$message ${i}"
>> + break
>> + }
>> + }
>> + gdb_test "where" ".*#0 0x\[0-9a-fA-F\]+ in lower.*"\
>> + "$message: should be in lower"
>> +}
>
> Likewise.
>
>> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c
>> new file mode 100644
>> index 0000000..5317369
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c
>> @@ -0,0 +1,66 @@
>> +/* Copyright (C) 2015 Free Software Foundation, Inc.
>
> 2015-2016
>
>> +
>
>> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
>
> Largely the same comments apply to this file. I'll only mention other
> extra things I spotted below.
>
>> +# Using the handler for SIGSEGV as "print pass stop"
>> +set parameters "print pass stop"
>> +runto_main
>
> Check result. Wrap with_test_prefix instead of appending
> $parameters, 0, 1, etc. in some test messages. That way,
> any fail within runto_main, etc. is covered as well, as well
> as being easier.
>
>> +send_gdb "handle SIGSEGV $parameters\n"
>> +send_gdb "continue\n"
>> +
>> +gdb_expect {
>> + -re $segv_bound_with_prompt {
>> + pass $parameters
>> + }
>> +}
>
> Use gdb_test/gdb_test_multiple instead of send_gdb/gdb_expect.
>
>> +gdb_test "handle SIGSEGV $parameters" "" "Setting\
>> +the handler for segfault 0"
>
> Odd line break. Break it before the string, not in the middle.
>
>> +
>> +gdb_test_multiple "continue" "test 0" {
>> + -re $segv_with_exit {
>> + pass $parameters
>> + }
>> + -re "$gdb_prompt $" {
>> + fail $parameters
>> + }
>> +}
>> +
>> +gdb_test "where" "No stack." "no inferior $parameters"
>> +
>> +# Using the handler for SIGSEGV as "print nopass stop"
>> +set parameters "print nopass stop"
>> +
>> +runto_main
>> +gdb_test "handle SIGSEGV $parameters" "" "Setting\
>> +the handler for segfault 1"
>
>
> Thanks,
> Pedro Alves
>
About your question on x32 and MPX.
MPX does not support 32bit pointers in 64bit mode.
Changes were just sent.
Thanks a lot for your review!
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
More information about the Gdb-patches
mailing list