Re: [PATCH V7 1/1] Intel MPX bound violation handling

[Pedro Alves <palves at redhat dot com>]

On 02/02/2016 01:42 PM, Walfred Tedeschi wrote:

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 7da31c8..521f542 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -5819,6 +5819,32 @@ $1 = (void *) 0x7ffff7ff7000
>  
>  Depending on target support, @code{$_siginfo} may also be writable.
>  
> +@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.

"mode" is missing after @code{handle nostop}.  Actually, reading this back,
avoiding "mode" may be better even, like:

"... has been told to handle the signal.  With @code{handle stop SIGSEGV},
@value{GDBN} displays the violation kind: "Upper" or "Lower", the memory
address accessed and the bounds, while with @code{handle nostop SIGSEGV}
no additional information is displayed."


> +
> +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:

Previously, I suggested changing this string to:

 "While a bound violation is presented as:"

That avoids the multiple ambiguous "it"s.



> +  TRY
> +    {
> +      long sig_code = 0;
> +
> +      /* Verify if the current segfault is a boundary violation.  */
> +      sig_code = parse_and_eval_long ("$_siginfo.si_code\n");
> +      if (sig_code != SIG_CODE_BONDARY_FAULT)
> +	return;

You can't "return" from a TRY block, you have to do that outside.

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

Wrap the string value strings in _().

> +
> +  ui_out_text (uiout, " while accessing address ");

Ditto.

> +  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, "]");
> +  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 +1066,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_handle_segmentation_fault (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 ecc9e31..f89efbc 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);
>  
> +/* Handle and display information related to the MPX bound violation
> +   to the user.  */
> +extern void i386_mpx_bound_violation_handler (struct gdbarch *gdbarch,
> +					      struct ui_out *uiout);

Add "linux", and rename it according to the gdbarch hook's name.  Tweak
the comment, like:

/* i386 GNU/Linux implementation of the handle_segmentation_fault
   gdbarch hook.  Displays information related to MPX bound
   violations.  */
extern void i386_linux_handle_segmentation_fault (struct gdbarch *gdbarch,
						  struct ui_out *uiout);

> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp

> +}
> +
> +set u_fault [multi_line "Program received signal SIGSEGV, Segmentation fault" \
> +                        "Upper bound violation while accessing address $hex" \
> +                        "Bounds: \\\[lower = $hex, upper = $hex\\\]"]
> +
> +set l_fault [multi_line "Program received signal SIGSEGV, Segmentation fault" \
> +                        "Lower bound violation while accessing address $hex" \
> +                        "Bounds: \\\[lower = $hex, upper = $hex\\\]"]
> +
> +set segv_upper_bound "$u_fault.*$gdb_prompt $"
> +
> +set segv_lower_bound "$l_fault.*$gdb_prompt $"
> +
> +for {set i 0} {$i < 15} {incr i} {
> +    set message "MPX signal segv Upper: ${i}"
> +    gdb_test_multiple "continue" "$message" {
> +        -re $segv_upper_bound {
> +            pass "$message"
> +        }
> +        -re ".*$inferior_exited_re normally.*$gdb_prompt $" {
> +            fail "$message"
> +            break

I hadn't noticed this break before -- if you want to skip the
rest of the loop, then do this outside the gdb_test_multiple,
because then you'll also catch the fails issued from inside
gdb_test_multiple.  Like:

    set ok 0
    gdb_test_multiple "continue" "$message" {
       -re $segv_upper_bound {
            pass "$message"
            set ok 1
        }
        -re ".*$inferior_exited_re normally.*$gdb_prompt $" {
            fail "$message"
        }
    }

    if {!$ok} {
      break
    }

But then, do we really need that "fail" regex?  Why not just:

    gdb_test_multiple "continue" "$message" {
       -re $segv_upper_bound {
            pass "$message"
            set ok 1
        }
    }

which could be further simplified with gdb_test:

    if {[gdb_test "continue" "$u_fault.* $message] != 0} {
       break
    }

> +
> +send_gdb "print have_mpx ()\r"
> +gdb_expect {
> +    -re ".. = 1\r\n$gdb_prompt " {
> +        pass "check whether processor supports MPX"
> +    }
> +    -re ".. = 0\r\n$gdb_prompt " {
> +        verbose "processor does not support MPX; skipping MPX tests"
> +        return
> +    }
> +}

Please use gdb_test/gdb_test_multiple instead of send_gdb/gdb_expect.

> +
> +set violation [multi_line "Program received signal SIGSEGV, Segmentation fault" \
> +                          "Upper bound violation while accessing address $hex" \
> +                          "Bounds: \\\[lower = $hex, upper = $hex\\\]"]
> +
> +set segv_bound_with_prompt "$violation.*$gdb_prompt $"
> +set segv_bound_with_exit "$violation.*$inferior_exited_re.*"
> +
> +set segv_with_exit "Program received signal SIGSEGV,\
> +        Segmentation fault.*$inferior_exited_re.*"
> +
> +# Using the handler for SIGSEGV as "print pass stop"
> +set parameters "print pass stop"
> +runto_main
> +send_gdb "handle SIGSEGV $parameters\n"
> +send_gdb "continue\n"

Likewise.

Check results of runto_main.

Actually, looks like you missed applying previous review
comments to this file.

Please cross check again the previous review comments to v5.

Thanks,
Pedro Alves