From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30685 invoked by alias); 28 Jan 2016 13:49:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30673 invoked by uid 89); 28 Jan 2016 13:49:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:10.7.209, H*r:ip*10.7.209.18, Spurious, 1012 X-HELO: mga01.intel.com Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Jan 2016 13:49:07 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 28 Jan 2016 05:49:06 -0800 X-ExtLoop1: 1 Received: from wtedesch-mobl2.ger.corp.intel.com (HELO [172.28.205.68]) ([172.28.205.68]) by orsmga001.jf.intel.com with ESMTP; 28 Jan 2016 05:49:04 -0800 Subject: Re: [PATCH V5 5/5] ntel MPX bound violation handling To: Pedro Alves , eliz@gnu.org, brobecker@adacore.com References: <1453474456-13169-1-git-send-email-walfred.tedeschi@intel.com> <1453474456-13169-6-git-send-email-walfred.tedeschi@intel.com> <56AA1ADD.3080206@redhat.com> Cc: gdb-patches@sourceware.org From: Walfred Tedeschi Message-ID: <56AA1C4F.80807@intel.com> Date: Thu, 28 Jan 2016 13:49:00 -0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56AA1ADD.3080206@redhat.com> Content-Type: text/plain; charset="windows-1252"; format="flowed" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00700.txt.bz2 Am 1/28/2016 um 2:42 PM schrieb Pedro Alves: > "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=3D0x603010, a=3D0x603030, b=3D0x603050, >> c=3D0x603070, d=3D0x603090, len=3D7) at i386-mpx-sigsegv.c:68 >> 68 value =3D *(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 =3D 0x7fffffffc390, upper =3D 0x7fffffffc3a3] >> 0x0000000000400d7c in upper (p=3D0x603010, a=3D0x603030, b=3D0x603050, >> c=3D0x603070, d=3D0x603090, len=3D7) at i386-mpx-sigsegv.c:68 >> 68 value =3D *(p + len); >> >> In mi mode the output of a segfault is: >> *stopped,reason=3D"signal-received",signal-name=3D"SIGSEGV", >> signal-meaning=3D"Segmentation fault", frame=3D{addr=3D"0x0000000000400d= 7c", >> func=3D"upper",args=3D[{name=3D"p", value=3D"0x603010"},{name=3D"a",valu= e=3D"0x603030"} >> ,{name=3D"b",value=3D"0x603050"}, {name=3D"c",value=3D"0x603070"}, >> {name=3D"d",value=3D"0x603090"},{name=3D"len",value=3D"7"}], >> file=3D"i386-mpx-sigsegv.c",fullname=3D"i386-mpx-sigsegv.c",line=3D"68"}, >> thread-id=3D"1",stopped-threads=3D"all",core=3D"6" >> >> in the case of a bound violation: >> *stopped,reason=3D"signal-received",signal-name=3D"SIGSEGV", >> signal-meaning=3D"Segmentation fault", >> sigcode-meaning=3D"Upper bound violation", >> lower-bound=3D"0x603010",upper-bound=3D"0x603023",bound-access=3D"0x6030= 2f", >> frame=3D{addr=3D"0x0000000000400d7c",func=3D"upper",args=3D[{name=3D"p", >> value=3D"0x603010"},{name=3D"a",value=3D"0x603030"},{name=3D"b",value=3D= "0x603050"}, >> {name=3D"c",value=3D"0x603070"},{name=3D"d",value=3D"0x603090"}, >> {name=3D"len",value=3D"7"}],file=3D"i386-mpx-sigsegv.c", >> fullname=3D"i386-mpx-sigsegv.c",line=3D"68"},thread-id=3D"1", >> stopped-threads=3D"all",core=3D"6" >> >> 2016-01-15 Walfred Tedeschi >> >> 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 =3D 0x7fffffffc390, upper =3D 0x7fffffffc3a3]. >> + 0x0000000000400d7c in upper (p=3D0x603010, a=3D0x603030, b=3D0x60305= 0, >> + c=3D0x603070, d=3D0x603090, len=3D7) 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 =3D 0x7fffffffc390, upper =3D 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 =3D 0x7fffffffc390, upper =3D 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 in= fo, struct gdbarch *gdbarch) >> set_gdbarch_process_record_signal (gdbarch, amd64_linux_record_signa= l); >> >> 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 =3D (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 s= top} > 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=3D0x603010, a=3D0x603030, b=3D0x603050, >> +c=3D0x603070, d=3D0x603090, len=3D7) at i386-mpx-sigsegv.c:68 >> +68 value =3D *(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 =3D *(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 =3D 0x7fffffffc390, upper =3D 0x7fffffffc3a3] >> +0x0000000000400d7c in upper (p=3D0x603010, a=3D0x603030, b=3D0x603050, >> +c=3D0x603070, d=3D0x603090, len=3D7) at i386-mpx-sigsegv.c:68 >> +68 value =3D *(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 =3D 0x7fffffffc390, upper =3D 0x7fffffffc3a3] > 0x0000000000400d7c in upper () at i386-mpx-sigsegv.c:68 > 68 value =3D *(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 =3D=3D 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 =3D=3D 0 */ >> /* Skip verify of pc_regnum, invalid_p =3D=3D 0 */ >> /* Skip verify of ps_regnum, invalid_p =3D=3D 0 */ >> @@ -773,6 +775,12 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_fi= le *file) >> "gdbarch_dump: bits_big_endian =3D %s\n", >> plongest (gdbarch->bits_big_endian)); >> fprintf_unfiltered (file, >> + "gdbarch_dump: gdbarch_bound_violation_handler_p(= ) =3D %d\n", >> + gdbarch_bound_violation_handler_p (gdbarch)); >> + fprintf_unfiltered (file, >> + "gdbarch_dump: bound_violation_handler =3D <%s>\n= ", >> + host_address_to_string (gdbarch->bound_violation_= handler)); >> + fprintf_unfiltered (file, >> "gdbarch_dump: breakpoint_from_pc =3D <%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 !=3D NULL); >> + return gdbarch->bound_violation_handler !=3D NULL; >> +} >> + >> +void >> +gdbarch_bound_violation_handler (struct gdbarch *gdbarch, struct ui_out= *uiout) >> +{ >> + gdb_assert (gdbarch !=3D NULL); >> + gdb_assert (gdbarch->bound_violation_handler !=3D NULL); >> + if (gdbarch_debug >=3D 2) >> + fprintf_unfiltered (gdb_stdlog, "gdbarch_bound_violation_handler ca= lled\n"); >> + gdbarch->bound_violation_handler (gdbarch, uiout); >> +} >> + >> +void >> +set_gdbarch_bound_violation_handler (struct gdbarch *gdbarch, >> + gdbarch_bound_violation_handler_ft= ype bound_violation_handler) >> +{ >> + gdbarch->bound_violation_handler =3D bound_violation_handler; >> +} >> + >> +int >> gdbarch_sp_regnum (struct gdbarch *gdbarch) >> { >> gdb_assert (gdbarch !=3D 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 *gdba= rch, 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_p= ush_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 *g= dbarch, struct ui_out *uiout); >> +extern void gdbarch_bound_violation_handler (struct gdbarch *gdbarch, s= truct ui_out *uiout); >> +extern void set_gdbarch_bound_violation_handler (struct gdbarch *gdbarc= h, 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 i= nferior, >> +# 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_ou= t *uiout) > > Missing intro comment. Something like "Implement ... hook." should be en= ough. > Look for preexisting examples. > >> +{ >> + CORE_ADDR lower_bound, upper_bound, access; >> + int is_upper; >> + >> + if (!i386_mpx_enabled ()) >> + return; >> + TRY >> + { >> + lower_bound >> + =3D parse_and_eval_long ("$_siginfo._sifields._sigfault._addr_b= nd._lower"); >> + upper_bound >> + =3D parse_and_eval_long ("$_siginfo._sifields._sigfault._addr_b= nd._upper"); >> + access >> + =3D parse_and_eval_long ("$_siginfo._sifields._sigfault.si_addr= "); >> + } >> + CATCH (exception, RETURN_MASK_ALL) >> + { >> + return; >> + } >> + END_CATCH >> + >> + is_upper =3D (access > upper_bound ? 1 : 0); >> + >> + ui_out_text (uiout, "\n"); >> + if (is_upper) >> + ui_out_field_string (uiout, "sigcode-meaning", "Upper bound violati= on"); >> + else >> + ui_out_field_string (uiout, "sigcode-meaning", "Lower bound violati= on"); >> + >> + ui_out_text (uiout, " while accessing address "); >> + ui_out_field_fmt (uiout,"bound-access", "%s", paddress (gdbarch, acce= ss)); >> + ui_out_text (uiout, "\nBounds: [lower =3D "); >> + ui_out_field_fmt (uiout,"lower-bound", "%s", paddress (gdbarch, lower= _bound)); >> + ui_out_text (uiout, ", upper =3D "); >> + 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-l= ong 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, str= uct 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_ou= t *uiout); > > Only function definitions should have the function name at collumn 0. Al= l 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 =3D 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 *gdba= rch, >> struct regcache *regcache, CORE_ADDR a= ddr); >> extern const struct target_desc *i386_target_description (uint64_t xcr= 0); >> >> +/* 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); >> =0C >> >> /* 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 ex= itstatus) >> } >> } >> >> +/* 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 =3D 0; >> + struct regcache *regcache =3D get_current_regcache (); >> + struct gdbarch *gdbarch =3D get_regcache_arch (regcache); >> + >> + TRY >> + { >> + sig_code =3D 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 =3D=3D 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 si= ggnal) >> { >> @@ -7922,6 +7952,10 @@ print_signal_received_reason (struct ui_out *uiou= t, enum gdb_signal siggnal) >> annotate_signal_string (); >> ui_out_field_string (uiout, "signal-meaning", >> gdb_signal_to_string (siggnal)); >> + >> + if (siggnal =3D=3D 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/g= db.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 ".. =3D 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 ".. =3D 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 =3D $hex, upper =3D $hex\\\]" >> + >> +set common_lbound_violation ".*Program received signal SIGSEGV,\ >> + Segmentation fault\r\nLower bound violation while accessing\ >> + address $hex\r\nBounds: \\\[lower =3D $hex, upper =3D $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: > > https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_mess= ages_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/testsui= te/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/tests= uite/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 > Thanks a lot for your review! Will address all comments and answer your question as well! For the ones that are ok with the proposed changes should we commit=20 before the 7.11 branch is taken or rather waiting new branch is taken? (Respecting the release process i was waiting already with the previous=20 patch) Thanks again and 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