[PATCH obv] Changing compiler flags for MPX tests.

[PATCH obv] Changing compiler flags for MPX tests.

[Walfred Tedeschi]

Adapts tests to use actual GCC flags, previous used flags were
related to an internal GCC release.

2015-06-18  Walfred Tedeschi  <walfred.tedeschi@intel.com>

gdb/testsuite:

	* gdb.arch/i386-mpx-map.exp (comp_flags): Use released GCC flags.
	* gdb.arch/i386-mpx.exp (comp_flags): Use released GCC flags.

---
 gdb/testsuite/gdb.arch/i386-mpx-map.exp | 2 +-
 gdb/testsuite/gdb.arch/i386-mpx.exp     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/i386-mpx-map.exp b/gdb/testsuite/gdb.arch/i386-mpx-map.exp
index 47efb16..a3488c0 100644
--- a/gdb/testsuite/gdb.arch/i386-mpx-map.exp
+++ b/gdb/testsuite/gdb.arch/i386-mpx-map.exp
@@ -23,7 +23,7 @@ if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
 
 standard_testfile
 
-set comp_flags "-fmpx -I${srcdir}/../nat/"
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
 
 if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
  [list debug nowarnings additional_flags=${comp_flags}]] } {
diff --git a/gdb/testsuite/gdb.arch/i386-mpx.exp b/gdb/testsuite/gdb.arch/i386-mpx.exp
index 2db6401..a4166ba 100644
--- a/gdb/testsuite/gdb.arch/i386-mpx.exp
+++ b/gdb/testsuite/gdb.arch/i386-mpx.exp
@@ -27,7 +27,7 @@ if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
     return
 }
 
-set comp_flags "-fmpx -I${srcdir}/../nat/"
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
 
 if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
      [list debug nowarnings additional_flags=${comp_flags}]] } {
-- 
2.1.4

[PATCH v1] Intel(R) MPX registers to the DWARF enumeration.

[Walfred Tedeschi]

Add registers as defined in the ABI adapted for MPX.
As presented at:
https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI

2013-05-06  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* amd64-tdep.c (amd64_dwarf_regmap): Add mpx registers.
	* amd64-tdep.h (amd64_regnum): Add mpx registers.

---
 gdb/amd64-tdep.c | 12 +++++++++++-
 gdb/amd64-tdep.h |  3 ++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index f0720c8..0fa4d54 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -233,7 +233,17 @@ static int amd64_dwarf_regmap[] =
   /* Floating Point Control Registers.  */
   AMD64_MXCSR_REGNUM,
   AMD64_FCTRL_REGNUM,
-  AMD64_FSTAT_REGNUM
+  AMD64_FSTAT_REGNUM,
+  -1, -1, -1, -1,				/* 67 ... 70.  */
+  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,	/* 70 ... 80.  */
+  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,	/* 80 ... 90.  */
+  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,	/* 90 ... 100.  */
+  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,	/* 100 ... 110.  */
+  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,	/* 110 ... 120.  */
+  -1, -1, -1, -1, -1,				/*120  ... 125.  */
+
+  AMD64_BND0R_REGNUM, AMD64_BND0R_REGNUM + 1,
+  AMD64_BND0R_REGNUM + 2, AMD64_BND0R_REGNUM + 3
 };
 
 static const int amd64_dwarf_regmap_len =
diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
index 704225e..76a89b9 100644
--- a/gdb/amd64-tdep.h
+++ b/gdb/amd64-tdep.h
@@ -66,7 +66,8 @@ enum amd64_regnum
   AMD64_YMM0H_REGNUM,		/* %ymm0h */
   AMD64_YMM15H_REGNUM = AMD64_YMM0H_REGNUM + 15,
   AMD64_BND0R_REGNUM = AMD64_YMM15H_REGNUM + 1,
-  AMD64_BND3R_REGNUM = AMD64_BND0R_REGNUM + 3,
+  AMD64_BND1R_REGNUM, AMD64_BND2R_REGNUM,
+  AMD64_BND3R_REGNUM,
   AMD64_BNDCFGU_REGNUM,
   AMD64_BNDSTATUS_REGNUM,
   AMD64_XMM16_REGNUM,
-- 
2.1.4

Re: [PATCH v1] Intel(R) MPX registers to the DWARF enumeration.

[Joel Brobecker]

> Add registers as defined in the ABI adapted for MPX.
> As presented at:
> https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
> 
> 2013-05-06  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
>         * amd64-tdep.c (amd64_dwarf_regmap): Add mpx registers.
>         * amd64-tdep.h (amd64_regnum): Add mpx registers.

Small nit: should we spell "MPX"?

BTW - the ABI document reference above seem to only indicate
registers 126-129 as "reserved" rather than bound registers 0-4.
Is that normal?

> ---
>  gdb/amd64-tdep.c | 12 +++++++++++-
>  gdb/amd64-tdep.h |  3 ++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index f0720c8..0fa4d54 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -233,7 +233,17 @@ static int amd64_dwarf_regmap[] =
>    /* Floating Point Control Registers.  */
>    AMD64_MXCSR_REGNUM,
>    AMD64_FCTRL_REGNUM,
> -  AMD64_FSTAT_REGNUM
> +  AMD64_FSTAT_REGNUM,
> +  -1, -1, -1, -1,                              /* 67 ... 70.  */
> +  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,      /* 70 ... 80.  */
> +  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,      /* 80 ... 90.  */
> +  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,      /* 90 ... 100.  */
> +  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,      /* 100 ... 110.  */
> +  -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,      /* 110 ... 120.  */
> +  -1, -1, -1, -1, -1,                          /*120  ... 125.  */
> +
> +  AMD64_BND0R_REGNUM, AMD64_BND0R_REGNUM + 1,
> +  AMD64_BND0R_REGNUM + 2, AMD64_BND0R_REGNUM + 3

I'll admit this is a bit of a nitpicking, but I think it would
be useful to continue doing what we've been doing before,
which is document what the various range of register numbers
are for. And let's try to have them organized in banks of 8,
rather than 10.

Eg:

  /* MMX Registers 16 - 31 (67 - 82).  */
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,

  /* Reserved (83 - 117).  */
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1,

  /* Vector Mask Register 0 - 7 (118 - 125).  */
  -1, -1, -1, -1, -1, -1, -1, -1,

  /* Bound Registers 0 - 3 (126 - 129).  */
  AMD64_BND0R_REGNUM, AMD64_BND0R_REGNUM + 1,
  AMD64_BND0R_REGNUM + 2, AMD64_BND0R_REGNUM + 3,

(note that I added a comma at the end of the last register;
that way, if we add more later on, we don't have to modify
that line of code)

>  };
>  
>  static const int amd64_dwarf_regmap_len =
> diff --git a/gdb/amd64-tdep.h b/gdb/amd64-tdep.h
> index 704225e..76a89b9 100644
> --- a/gdb/amd64-tdep.h
> +++ b/gdb/amd64-tdep.h
> @@ -66,7 +66,8 @@ enum amd64_regnum
>    AMD64_YMM0H_REGNUM,          /* %ymm0h */
>    AMD64_YMM15H_REGNUM = AMD64_YMM0H_REGNUM + 15,
>    AMD64_BND0R_REGNUM = AMD64_YMM15H_REGNUM + 1,
> -  AMD64_BND3R_REGNUM = AMD64_BND0R_REGNUM + 3,
> +  AMD64_BND1R_REGNUM, AMD64_BND2R_REGNUM,
> +  AMD64_BND3R_REGNUM,
>    AMD64_BNDCFGU_REGNUM,
>    AMD64_BNDSTATUS_REGNUM,
>    AMD64_XMM16_REGNUM,
> -- 
> 2.1.4

-- 
Joel

Re: [PATCH v1] Intel(R) MPX registers to the DWARF enumeration.

[H.J. Lu]

On Sun, Dec 6, 2015 at 8:35 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Add registers as defined in the ABI adapted for MPX.
>> As presented at:
>> https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
>>
>> 2013-05-06  Walfred Tedeschi  <walfred.tedeschi@intel.com>
>>
>>         * amd64-tdep.c (amd64_dwarf_regmap): Add mpx registers.
>>         * amd64-tdep.h (amd64_regnum): Add mpx registers.
>
> Small nit: should we spell "MPX"?
>
> BTW - the ABI document reference above seem to only indicate
> registers 126-129 as "reserved" rather than bound registers 0-4.
> Is that normal?
>

We used to have DWARF register map for bound registers.  But
it was determined that it isn't needed.  If it isn't true, we need to
revisit it.


-- 
H.J.

RE: [PATCH v1] Intel(R) MPX registers to the DWARF enumeration.

[Tedeschi, Walfred]

HJ and Joel,

I suppose those registers were in on an older version of the ABI. However, I see no need for the DWARF registers as well. 
Will implement as in the new document; they will be out.

Thanks and regards,
-Fred

-----Original Message-----
From: H.J. Lu [mailto:hjl.tools@gmail.com] 
Sent: Sunday, December 06, 2015 6:42 PM
To: Joel Brobecker
Cc: Tedeschi, Walfred; GDB
Subject: Re: [PATCH v1] Intel(R) MPX registers to the DWARF enumeration.

On Sun, Dec 6, 2015 at 8:35 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Add registers as defined in the ABI adapted for MPX.
>> As presented at:
>> https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
>>
>> 2013-05-06  Walfred Tedeschi  <walfred.tedeschi@intel.com>
>>
>>         * amd64-tdep.c (amd64_dwarf_regmap): Add mpx registers.
>>         * amd64-tdep.h (amd64_regnum): Add mpx registers.
>
> Small nit: should we spell "MPX"?
>
> BTW - the ABI document reference above seem to only indicate registers 
> 126-129 as "reserved" rather than bound registers 0-4.
> Is that normal?
>

We used to have DWARF register map for bound registers.  But it was determined that it isn't needed.  If it isn't true, we need to revisit it.


--
H.J.
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

[PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Walfred Tedeschi]

New kernel and glibc patches have introduced fields in the
segmentation fault fields.

Kernel patch:
http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=ee1b58d36aa1b5a79eaba11f5c3633c88231da83

Glibc patch:
http://repo.or.cz/w/glibc.git/commit/d4358b51c26a634eb885955aea06cad26af6f696

2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* linux-tdep.c (linux_get_siginfo_type): Add the _addr_bnd
	structure to the siginfo.

---
 gdb/linux-tdep.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 7c24eaa..de773ae 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -248,10 +248,10 @@ static struct type *
 linux_get_siginfo_type (struct gdbarch *gdbarch)
 {
   struct linux_gdbarch_data *linux_gdbarch_data;
-  struct type *int_type, *uint_type, *long_type, *void_ptr_type;
+  struct type *int_type, *uint_type, *long_type, *void_ptr_type, *short_type;
   struct type *uid_type, *pid_type;
   struct type *sigval_type, *clock_type;
-  struct type *siginfo_type, *sifields_type;
+  struct type *siginfo_type, *sifields_type, *sigfault_bnd_fields;
   struct type *type;
 
   linux_gdbarch_data = get_linux_gdbarch_data (gdbarch);
@@ -264,6 +264,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
 				 1, "unsigned int");
   long_type = arch_integer_type (gdbarch, gdbarch_long_bit (gdbarch),
 				 0, "long");
+  short_type = arch_integer_type (gdbarch, gdbarch_long_bit (gdbarch),
+				 0, "short");
   void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
 
   /* sival_t */
@@ -339,6 +341,12 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
   /* _sigfault */
   type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
   append_composite_type_field (type, "si_addr", void_ptr_type);
+  append_composite_type_field (type, "_addr_lsb", short_type);
+
+  sigfault_bnd_fields = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
+  append_composite_type_field (sigfault_bnd_fields, "_lower", void_ptr_type);
+  append_composite_type_field (sigfault_bnd_fields, "_upper", void_ptr_type);
+  append_composite_type_field (type, "_addr_bnd", sigfault_bnd_fields);
   append_composite_type_field (sifields_type, "_sigfault", type);
 
   /* _sigpoll */
-- 
2.1.4

Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Joel Brobecker]

> New kernel and glibc patches have introduced fields in the
> segmentation fault fields.
> 
> Kernel patch:
> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=ee1b58d36aa1b5a79eaba11f5c3633c88231da83
> 
> Glibc patch:
> http://repo.or.cz/w/glibc.git/commit/d4358b51c26a634eb885955aea06cad26af6f696
> 
> 2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* linux-tdep.c (linux_get_siginfo_type): Add the _addr_bnd
> 	structure to the siginfo.

I would really like someone like Pedro to review this patch, as
I am really uncertain about the idea of extending our view of
the structure without a way of checking that you have kernel and
glibc support for it. As far as I can tell, this sets things up
so as to ready undefined data, thus leading to undefined behavior.

> 
> ---
>  gdb/linux-tdep.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 7c24eaa..de773ae 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -248,10 +248,10 @@ static struct type *
>  linux_get_siginfo_type (struct gdbarch *gdbarch)
>  {
>    struct linux_gdbarch_data *linux_gdbarch_data;
> -  struct type *int_type, *uint_type, *long_type, *void_ptr_type;
> +  struct type *int_type, *uint_type, *long_type, *void_ptr_type, *short_type;
>    struct type *uid_type, *pid_type;
>    struct type *sigval_type, *clock_type;
> -  struct type *siginfo_type, *sifields_type;
> +  struct type *siginfo_type, *sifields_type, *sigfault_bnd_fields;
>    struct type *type;
>  
>    linux_gdbarch_data = get_linux_gdbarch_data (gdbarch);
> @@ -264,6 +264,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>  				 1, "unsigned int");
>    long_type = arch_integer_type (gdbarch, gdbarch_long_bit (gdbarch),
>  				 0, "long");
> +  short_type = arch_integer_type (gdbarch, gdbarch_long_bit (gdbarch),
> +				 0, "short");
>    void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
>  
>    /* sival_t */
> @@ -339,6 +341,12 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>    /* _sigfault */
>    type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>    append_composite_type_field (type, "si_addr", void_ptr_type);
> +  append_composite_type_field (type, "_addr_lsb", short_type);
> +
> +  sigfault_bnd_fields = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
> +  append_composite_type_field (sigfault_bnd_fields, "_lower", void_ptr_type);
> +  append_composite_type_field (sigfault_bnd_fields, "_upper", void_ptr_type);
> +  append_composite_type_field (type, "_addr_bnd", sigfault_bnd_fields);
>    append_composite_type_field (sifields_type, "_sigfault", type);
>  
>    /* _sigpoll */
> -- 
> 2.1.4

-- 
Joel

RE: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Tedeschi, Walfred]

Joel and all,

First of all, Thanks a lot for your review, on both patches!

I had a discussion on the GDB list about this topic.
Observation from Yao was that we could use this structure on new and old kernels.
https://sourceware.org/ml/gdb/2015-07/msg00029.html

This was the reason why I wanted to keep it simple and avoid doing any king of check.

I understood from the kernel and glibc patches that the kernel is generic (all architectures) and the 
glibc is added for Intel only.
 
Finally I think it is also better to describe a bit what happens with the new fields in the kernel side.

In case we have a bound violation, the kernel does a disassemble of the current instruction,
Reads the values of the operands of the compare bound instructions and set the _low and _upper bound.
At the same time it sets the sig_code to 3. I.e. Those fields are only valid if the sigcode is 3.

1. Make a mechanism to determine which siginfo version to be used depending from the system being debugged.
I am not sure if there is such a mechanism in GDB already or how error prune it would be to be
able to detect pre presence of these fields from the kernel side and glibc (in the case of
the glibc also taking into account the architecture). 

2. Set the field _low and _upper to 0 in case the sigcode is not 3.

Any ideas or comments on that.

Thanks and regards,
-Fred

-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Thursday, November 19, 2015 12:02 AM
To: Tedeschi, Walfred
Cc: palves@redhat.com; gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

> New kernel and glibc patches have introduced fields in the 
> segmentation fault fields.
> 
> Kernel patch:
> http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?id=ee1
> b58d36aa1b5a79eaba11f5c3633c88231da83
> 
> Glibc patch:
> http://repo.or.cz/w/glibc.git/commit/d4358b51c26a634eb885955aea06cad26
> af6f696
> 
> 2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* linux-tdep.c (linux_get_siginfo_type): Add the _addr_bnd
> 	structure to the siginfo.

I would really like someone like Pedro to review this patch, as I am really uncertain about the idea of extending our view of the structure without a way of checking that you have kernel and glibc support for it. As far as I can tell, this sets things up so as to ready undefined data, thus leading to undefined behavior.

> 
> ---
>  gdb/linux-tdep.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c index 
> 7c24eaa..de773ae 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -248,10 +248,10 @@ static struct type *  linux_get_siginfo_type 
> (struct gdbarch *gdbarch)  {
>    struct linux_gdbarch_data *linux_gdbarch_data;
> -  struct type *int_type, *uint_type, *long_type, *void_ptr_type;
> +  struct type *int_type, *uint_type, *long_type, *void_ptr_type, 
> + *short_type;
>    struct type *uid_type, *pid_type;
>    struct type *sigval_type, *clock_type;
> -  struct type *siginfo_type, *sifields_type;
> +  struct type *siginfo_type, *sifields_type, *sigfault_bnd_fields;
>    struct type *type;
>  
>    linux_gdbarch_data = get_linux_gdbarch_data (gdbarch); @@ -264,6 
> +264,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>  				 1, "unsigned int");
>    long_type = arch_integer_type (gdbarch, gdbarch_long_bit (gdbarch),
>  				 0, "long");
> +  short_type = arch_integer_type (gdbarch, gdbarch_long_bit (gdbarch),
> +				 0, "short");
>    void_ptr_type = lookup_pointer_type (builtin_type 
> (gdbarch)->builtin_void);
>  
>    /* sival_t */
> @@ -339,6 +341,12 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
>    /* _sigfault */
>    type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
>    append_composite_type_field (type, "si_addr", void_ptr_type);
> +  append_composite_type_field (type, "_addr_lsb", short_type);
> +
> +  sigfault_bnd_fields = arch_composite_type (gdbarch, NULL, 
> + TYPE_CODE_STRUCT);  append_composite_type_field 
> + (sigfault_bnd_fields, "_lower", void_ptr_type);  
> + append_composite_type_field (sigfault_bnd_fields, "_upper", 
> + void_ptr_type);  append_composite_type_field (type, "_addr_bnd", 
> + sigfault_bnd_fields);
>    append_composite_type_field (sifields_type, "_sigfault", type);
>  
>    /* _sigpoll */
> --
> 2.1.4

--
Joel
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

Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Pedro Alves]

On 11/19/2015 09:51 AM, Tedeschi, Walfred wrote:

> I understood from the kernel and glibc patches that the kernel is generic (all architectures) and the 
> glibc is added for Intel only.

IIRC, not all architectures in the kernel use /include/uapi/asm-generic/siginfo.h, and
some have their own custom siginfo.h.  Those should have a custom gdbarch_get_siginfo_type
hook in gdb as well, but nobody has ever bothered to fix that this far, supposedly
because the siginfo tests in the testsuite don't depend in bits of the siginfo objects
that might differ on such architectures.

It does look a bit odd to me to expose these new fields on all architectures, given that
this is Intel/x86-only.  But it doesn't _really_ hurt, other than potentially being
a little bit confusing.

>  
> Finally I think it is also better to describe a bit what happens with the new fields in the kernel side.
> 
> In case we have a bound violation, the kernel does a disassemble of the current instruction,
> Reads the values of the operands of the compare bound instructions and set the _low and _upper bound.
> At the same time it sets the sig_code to 3. I.e. Those fields are only valid if the sigcode is 3.
> 
> 1. Make a mechanism to determine which siginfo version to be used depending from the system being debugged.
> I am not sure if there is such a mechanism in GDB already or how error prune it would be to be
> able to detect pre presence of these fields from the kernel side and glibc (in the case of
> the glibc also taking into account the architecture). 

As alluded to above, the 'get_siginfo_type' method can be set different for each gdbarch.
So x86, arm, etc. each can tailor the siginfo type to their needs by installing
a different hook.  In fact, it's what we did prior to 5cd867b414fe, except that
before that commit, all archs were actually installing the same hook.

Checking the kernel version would be more complicated, considering remote debugging.  We
could have gdbserver probe for support for the feature, and then report it in qSupported
at connection time, for example, or even have the server fully describe the layout of the
object (similar to how it can describe register types, which you used in
gdb/features/i386/64bit-mpx.xml), though I don't think either would be warranted here.
The siginfo object's layout and size is ABI, the kernel can't change it.  Even though new fields
have been added to the end of struct _sigfault, the _sigfault field itself is part of a
union, which is padded to SI_PAD_SIZE bytes.  That is, this change did not change the
siginfo object's size.  Since we're only supposed to interpret the contents of the new
elements when si_code is 3 and the kernel never set si_code to 3 before, the change is
also fully backwards compatible.

> 
> 2. Set the field _low and _upper to 0 in case the sigcode is not 3.

I'd just leave them be.  You always have to know how to interpret the
siginfo object's elements, and know which enum field to look at between
siginfo._sifields.{_kill|_timer|_sigchld|etc.}, depending on si_code
and si_signo.  Doesn't the kernel of glibc already always memset the
whole object before filling in the valid bits, BTW?


Lastly, I think you missed tweaking amd64_linux_siginfo_fixup & co in order to
correctly convert these fields when debugging 32-bit and x32 programs on
a 64-bit kernel.  Likewise the gdbserver equivalents.


Thanks,
Pedro Alves

RE: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Tedeschi, Walfred]

Pedro,

If I understood you right you are in favour of keeping the siginfo as global one?
In case this is correct, I will start working on the code you pointed out.

What I could see from the glibc code the structure is reset via memset.

Thanks a lot for your feedback! 
Muito obrigado!

Best regards,
-Fred

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Thursday, November 19, 2015 2:28 PM
To: Tedeschi, Walfred; Joel Brobecker; Yao Qi <yao@codesourcery.com> (yao@codesourcery.com)
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

On 11/19/2015 09:51 AM, Tedeschi, Walfred wrote:

> I understood from the kernel and glibc patches that the kernel is 
> generic (all architectures) and the glibc is added for Intel only.

IIRC, not all architectures in the kernel use /include/uapi/asm-generic/siginfo.h, and some have their own custom siginfo.h.  Those should have a custom gdbarch_get_siginfo_type hook in gdb as well, but nobody has ever bothered to fix that this far, supposedly because the siginfo tests in the testsuite don't depend in bits of the siginfo objects that might differ on such architectures.

It does look a bit odd to me to expose these new fields on all architectures, given that this is Intel/x86-only.  But it doesn't _really_ hurt, other than potentially being a little bit confusing.

>  
> Finally I think it is also better to describe a bit what happens with the new fields in the kernel side.
> 
> In case we have a bound violation, the kernel does a disassemble of 
> the current instruction, Reads the values of the operands of the compare bound instructions and set the _low and _upper bound.
> At the same time it sets the sig_code to 3. I.e. Those fields are only valid if the sigcode is 3.
> 
> 1. Make a mechanism to determine which siginfo version to be used depending from the system being debugged.
> I am not sure if there is such a mechanism in GDB already or how error 
> prune it would be to be able to detect pre presence of these fields 
> from the kernel side and glibc (in the case of the glibc also taking into account the architecture).

As alluded to above, the 'get_siginfo_type' method can be set different for each gdbarch.
So x86, arm, etc. each can tailor the siginfo type to their needs by installing a different hook.  In fact, it's what we did prior to 5cd867b414fe, except that before that commit, all archs were actually installing the same hook.

Checking the kernel version would be more complicated, considering remote debugging.  We could have gdbserver probe for support for the feature, and then report it in qSupported at connection time, for example, or even have the server fully describe the layout of the object (similar to how it can describe register types, which you used in gdb/features/i386/64bit-mpx.xml), though I don't think either would be warranted here.
The siginfo object's layout and size is ABI, the kernel can't change it.  Even though new fields have been added to the end of struct _sigfault, the _sigfault field itself is part of a union, which is padded to SI_PAD_SIZE bytes.  That is, this change did not change the siginfo object's size.  Since we're only supposed to interpret the contents of the new elements when si_code is 3 and the kernel never set si_code to 3 before, the change is also fully backwards compatible.

> 
> 2. Set the field _low and _upper to 0 in case the sigcode is not 3.

I'd just leave them be.  You always have to know how to interpret the siginfo object's elements, and know which enum field to look at between siginfo._sifields.{_kill|_timer|_sigchld|etc.}, depending on si_code and si_signo.  Doesn't the kernel of glibc already always memset the whole object before filling in the valid bits, BTW?


Lastly, I think you missed tweaking amd64_linux_siginfo_fixup & co in order to correctly convert these fields when debugging 32-bit and x32 programs on a 64-bit kernel.  Likewise the gdbserver equivalents.


Thanks,
Pedro Alves

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

Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Pedro Alves]

On 11/19/2015 04:40 PM, Tedeschi, Walfred wrote:
> Pedro,
> 
> If I understood you right you are in favour of keeping the siginfo as global one?
> In case this is correct, I will start working on the code you pointed out.

I favor not querying whether the running x86 kernel supports the extra field.

But I wouldn't say I'm in favor of exposing the field in all targets -- more
like, if it's hard and we really want to be lazy, I see no real big harm in
making the change affect all archs, though I'd mildly prefer making the change
be x86-specific, like glibc did.  I actually think it'd be simple.  IIRC, Yao
already suggested how to do it without much changes.  Simply rename the existing
linux_get_siginfo_type function, and add a new parameter that allows
saying "I want field X".  Then the x86 version can install a siginfo type with
the field, while other ports not.

e.g.,
enum linux_siginfo_extra_field_values
{
  LINUX_SIGINFO_FIELD_ADDR_BND = 1,
  ...
};
DEF_ENUM_FLAGS_TYPE (enum linux_siginfo_extra_field_values, linux_siginfo_extra_fields);

static struct type *
linux_get_siginfo_type_with_fields (struct gdbarch *gdbarch,
                                    linux_siginfo_extra_fields extra_fields)
{
 ...

  if ((extra_fields & LINUX_SIGINFO_FIELD_ADDR_BND) != 0)
    {
       ...
    }
}

static struct type *
linux_get_siginfo_type (struct gdbarch *gdbarch)
{
   return linux_get_siginfo_type_with_fields (gdbarch, 0);
}

x86:

x86_linux_get_siginfo_type_with_fields (struct gdbarch *gdbarch,
                                        linux_siginfo_extra_fields extra_fields)
{
   return linux_get_siginfo_type_with_fields (gdbarch,
                                              LINUX_SIGINFO_FIELD_ADDR_BND);
}

...
set_gdbarch_get_siginfo_type (gdbarch, x86_linux_get_siginfo_type_with_fields);
...

> 
> What I could see from the glibc code the structure is reset via memset.

Actually, gdb reads the siginfo object out of the kernel directly, so whatever
glibc does has no bearing here.  The question is whether the kernel really
shows garbage in those fields when si_code is not 3, or whether it always
pre-fills the siginfo objects (including the padding) with zeroes.

> 
> Thanks a lot for your feedback! 
> Muito obrigado!

You're welcome.  De nada.  ;-)

Thanks,
Pedro Alves

RE: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Tedeschi, Walfred]

Hello Pedro,

Thanks a lot for your feedback!

Moving forward on this patch I observed that there is fixup for 32 and x32 on gdbserver and gdb side.
I  suppose we should join both implementation so we patch only once every time! :)

To do that I was wondering where should be the appropriated place, the "nat" or the "common" folder.


Thanks and regards,

-Fred



-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Thursday, November 19, 2015 6:07 PM
To: Tedeschi, Walfred; Joel Brobecker; Yao Qi <yao@codesourcery.com> (yao@codesourcery.com)
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

On 11/19/2015 04:40 PM, Tedeschi, Walfred wrote:
> Pedro,
> 
> If I understood you right you are in favour of keeping the siginfo as global one?
> In case this is correct, I will start working on the code you pointed out.

I favor not querying whether the running x86 kernel supports the extra field.

But I wouldn't say I'm in favor of exposing the field in all targets -- more like, if it's hard and we really want to be lazy, I see no real big harm in making the change affect all archs, though I'd mildly prefer making the change be x86-specific, like glibc did.  I actually think it'd be simple.  IIRC, Yao already suggested how to do it without much changes.  Simply rename the existing linux_get_siginfo_type function, and add a new parameter that allows saying "I want field X".  Then the x86 version can install a siginfo type with the field, while other ports not.

e.g.,
enum linux_siginfo_extra_field_values
{
  LINUX_SIGINFO_FIELD_ADDR_BND = 1,
  ...
};
DEF_ENUM_FLAGS_TYPE (enum linux_siginfo_extra_field_values, linux_siginfo_extra_fields);

static struct type *
linux_get_siginfo_type_with_fields (struct gdbarch *gdbarch,
                                    linux_siginfo_extra_fields extra_fields) {  ...

  if ((extra_fields & LINUX_SIGINFO_FIELD_ADDR_BND) != 0)
    {
       ...
    }
}

static struct type *
linux_get_siginfo_type (struct gdbarch *gdbarch) {
   return linux_get_siginfo_type_with_fields (gdbarch, 0); }

x86:

x86_linux_get_siginfo_type_with_fields (struct gdbarch *gdbarch,
                                        linux_siginfo_extra_fields extra_fields) {
   return linux_get_siginfo_type_with_fields (gdbarch,
                                              LINUX_SIGINFO_FIELD_ADDR_BND); }

...
set_gdbarch_get_siginfo_type (gdbarch, x86_linux_get_siginfo_type_with_fields);
...

> 
> What I could see from the glibc code the structure is reset via memset.

Actually, gdb reads the siginfo object out of the kernel directly, so whatever glibc does has no bearing here.  The question is whether the kernel really shows garbage in those fields when si_code is not 3, or whether it always pre-fills the siginfo objects (including the padding) with zeroes.

> 
> Thanks a lot for your feedback! 
> Muito obrigado!

You're welcome.  De nada.  ;-)

Thanks,
Pedro Alves
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

Re: [PATCH v1] Synchronize siginfo type described in GDB with the kernel and glibc ones.

[Pedro Alves]

On 12/01/2015 10:08 AM, Tedeschi, Walfred wrote:
> Hello Pedro,
> 
> Thanks a lot for your feedback!
> 
> Moving forward on this patch I observed that there is fixup for 32 and x32 on gdbserver and gdb side.
> I  suppose we should join both implementation so we patch only once every time! :)

That'd be nice. :-)

> 
> To do that I was wondering where should be the appropriated place, the "nat" or the "common" folder.

Since this is code used by the (native) target_ops implementations, nat/ is the
appropriate place.

Thanks,
Pedro Alves

[PATCH v1] ABI changes for Intel(R) MPX.

[Walfred Tedeschi]

Code reflects what is presented in the ABI document:
https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI

Here new class POINTER was added.  GDB code is modified to mirror
this new class.

New set and show command was added as option for initializing bounds when
returning from inferior calls or not.

2015-08-25  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* amd64-tdep.c (amd64_reg_class): Add new class AMD64_POINTER.
	(amd64_merge_classes): Add AMD64_POINTER to merging classes rules.
	(amd64_classify): Add AMD64_POINTER.
	(mpx_bnd_init_on_return): New.
	(amd64_return_value): Add bound register.
	(amd64_return_value): Set bound values for returning.
	(amd64_push_arguments): Add new AMD64_POINTER class.
	(amd64_push_dummy_call): Initialize bound registers before call.
	(show_mpx_init_on_return): New funtion.
	(amd64_init_abi): Add new commands set/show mpx-bnd-init-on-return.

doc:
	gdb.texinfo: (Intel(R) Memory Protection Extensions): Add
	documentation on performing program function calls.

testsuite:
	amd64-mpx-call.c: New.
	amd64-mpx-call.exp: New.

---
 gdb/amd64-tdep.c                          |  97 +++++++++++++++++++++--
 gdb/doc/gdb.texinfo                       |  17 ++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 124 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 ++++++++++++++++++++++
 4 files changed, 321 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
 create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 0fa4d54..2fa555d 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -473,6 +473,7 @@ amd64_pseudo_register_write (struct gdbarch *gdbarch,
 
 enum amd64_reg_class
 {
+  AMD64_POINTER,
   AMD64_INTEGER,
   AMD64_SSE,
   AMD64_SSEUP,
@@ -504,18 +505,22 @@ amd64_merge_classes (enum amd64_reg_class class1, enum amd64_reg_class class2)
   if (class1 == AMD64_MEMORY || class2 == AMD64_MEMORY)
     return AMD64_MEMORY;
 
-  /* Rule (d): If one of the classes is INTEGER, the result is INTEGER.  */
+  /* Rule (d): If one of the classes is POINTER, the result is POINTER.  */
+  if (class1 == AMD64_POINTER || class2 == AMD64_POINTER)
+    return AMD64_POINTER;
+
+  /* Rule (e): If one of the classes is INTEGER, the result is INTEGER.  */
   if (class1 == AMD64_INTEGER || class2 == AMD64_INTEGER)
     return AMD64_INTEGER;
 
-  /* Rule (e): If one of the classes is X87, X87UP, COMPLEX_X87 class,
+  /* Rule (f): If one of the classes is X87, X87UP, COMPLEX_X87 class,
      MEMORY is used as class.  */
   if (class1 == AMD64_X87 || class1 == AMD64_X87UP
       || class1 == AMD64_COMPLEX_X87 || class2 == AMD64_X87
       || class2 == AMD64_X87UP || class2 == AMD64_COMPLEX_X87)
     return AMD64_MEMORY;
 
-  /* Rule (f): Otherwise class SSE is used.  */
+  /* Rule (g): Otherwise class SSE is used.  */
   return AMD64_SSE;
 }
 
@@ -648,14 +653,17 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
 
   theclass[0] = theclass[1] = AMD64_NO_CLASS;
 
+  /* Arguments of types (pointer and reference) are of the class pointer.  */
+  if (code == TYPE_CODE_PTR || code == TYPE_CODE_REF)
+    theclass[0] = AMD64_POINTER;
+
   /* Arguments of types (signed and unsigned) _Bool, char, short, int,
      long, long long, and pointers are in the INTEGER class.  Similarly,
      range types, used by languages such as Ada, are also in the INTEGER
      class.  */
-  if ((code == TYPE_CODE_INT || code == TYPE_CODE_ENUM
+  else if ((code == TYPE_CODE_INT || code == TYPE_CODE_ENUM
        || code == TYPE_CODE_BOOL || code == TYPE_CODE_RANGE
-       || code == TYPE_CODE_CHAR
-       || code == TYPE_CODE_PTR || code == TYPE_CODE_REF)
+       || code == TYPE_CODE_CHAR)
       && (len == 1 || len == 2 || len == 4 || len == 8))
     theclass[0] = AMD64_INTEGER;
 
@@ -705,16 +713,22 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
     amd64_classify_aggregate (type, theclass);
 }
 
+static int mpx_bnd_init_on_return = 1;
+
 static enum return_value_convention
 amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 		    struct type *type, struct regcache *regcache,
 		    gdb_byte *readbuf, const gdb_byte *writebuf)
 {
   enum amd64_reg_class theclass[2];
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   int len = TYPE_LENGTH (type);
   static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
   static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
+  static int bnd_regnum[] = { AMD64_BND0R_REGNUM, AMD64_BND0R_REGNUM,
+      AMD64_BND2R_REGNUM, AMD64_BND3R_REGNUM };
   int integer_reg = 0;
+  int bnd_reg = 0;
   int sse_reg = 0;
   int i;
 
@@ -742,6 +756,20 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 
 	  regcache_raw_read_unsigned (regcache, AMD64_RAX_REGNUM, &addr);
 	  read_memory (addr, readbuf, TYPE_LENGTH (type));
+
+	  /*  If the class is memory, Boundary has to be stored in the bnd0 in
+	      the case the application is MPX enabled.
+	      In order to be return a boundary value mpx_bnd_init_on_return
+	      has to be 0.  Otherwise the actual value present in the register
+	      will be returned.  */
+
+	  if (writebuf && mpx_bnd_init_on_return && AMD64_BND0R_REGNUM > 0)
+	    {
+	      gdb_byte bnd_buf[16];
+
+	      memset (bnd_buf, 0, 16);
+	      regcache_raw_write (regcache, AMD64_BND0R_REGNUM, bnd_buf);
+	    }
 	}
 
       return RETURN_VALUE_ABI_RETURNS_ADDRESS;
@@ -777,6 +805,7 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
   for (i = 0; len > 0; i++, len -= 8)
     {
       int regnum = -1;
+      int bnd_reg_count = -1;
       int offset = 0;
 
       switch (theclass[i])
@@ -787,6 +816,13 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 	  regnum = integer_regnum[integer_reg++];
 	  break;
 
+	case AMD64_POINTER:
+	  /* 3. If the class is POINTER, same rules of integer applies.  */
+	  regnum = integer_regnum[integer_reg++];
+	  /* But we need to allocate a bnd reg.  */
+	  bnd_reg_count = bnd_regnum[bnd_reg++];
+	  break;
+
 	case AMD64_SSE:
 	  /* 4. If the class is SSE, the next available SSE register
              of the sequence %xmm0, %xmm1 is used.  */
@@ -835,6 +871,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
 				 writebuf + i * 8);
     }
 
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+      int i, init_bnd;
+
+      memset (bnd_buf, 0, 16);
+      if (writebuf && mpx_bnd_init_on_return)
+	for (i = 0; i < I387_NUM_BND_REGS; i++)
+	  regcache_raw_write (regcache, AMD64_BND0R_REGNUM + i, bnd_buf);
+    }
+
   return RETURN_VALUE_REGISTER_CONVENTION;
 }
 \f
@@ -888,7 +935,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
          this argument.  */
       for (j = 0; j < 2; j++)
 	{
-	  if (theclass[j] == AMD64_INTEGER)
+	  if (theclass[j] == AMD64_INTEGER || theclass[j] == AMD64_POINTER)
 	    needed_integer_regs++;
 	  else if (theclass[j] == AMD64_SSE)
 	    needed_sse_regs++;
@@ -919,6 +966,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
 
 	      switch (theclass[j])
 		{
+		case AMD64_POINTER:
 		case AMD64_INTEGER:
 		  regnum = integer_regnum[integer_reg++];
 		  break;
@@ -978,8 +1026,23 @@ amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		       int struct_return, CORE_ADDR struct_addr)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   gdb_byte buf[8];
 
+  /* When MPX is enabled all bnd registers have to be Initialized before the
+     call.  This avoids undesired bound violations while executing the call.
+     At this point in time we don need to worry about the   */
+
+  if (I387_BND0R_REGNUM (tdep) > 0)
+    {
+      gdb_byte bnd_buf[16];
+      int i;
+
+      memset (bnd_buf, 0, 16);
+      for (i = 0; i < I387_NUM_BND_REGS; i++)
+	regcache_raw_write (regcache, AMD64_BND0R_REGNUM + i, bnd_buf);
+    }
+
   /* Pass arguments.  */
   sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
 
@@ -2944,6 +3007,18 @@ static const int amd64_record_regmap[] =
   AMD64_DS_REGNUM, AMD64_ES_REGNUM, AMD64_FS_REGNUM, AMD64_GS_REGNUM
 };
 
+static void
+show_mpx_init_on_return (struct ui_file *file, int from_tty,
+			  struct cmd_list_element *c, const char *value)
+{
+  if (mpx_bnd_init_on_return > 0)
+    fprintf_filtered (file,
+		    _("BND registers will be initialized on return.\n"));
+  else
+    fprintf_filtered (file,
+		    _("BND registers will not be initialized on return.\n"));
+}
+
 void
 amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -2994,6 +3069,14 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.mpx") != NULL)
     {
+      add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
+				&mpx_bnd_init_on_return, _("\
+Set the bnd registers to INIT state when returning from a call."), _("\
+Show the state of the mpx-bnd-init-on-return."),
+			    NULL,
+			    NULL,
+			    show_mpx_init_on_return,
+			    &setlist, &showlist);
       tdep->mpx_register_names = amd64_mpx_names;
       tdep->bndcfgu_regnum = AMD64_BNDCFGU_REGNUM;
       tdep->bnd0r_regnum = AMD64_BND0R_REGNUM;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 3c1f785..1d8e667 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -22042,6 +22042,23 @@ whose bounds are to be changed, @var{lbound} and @var{ubound} are new values
 for lower and upper bounds respectively.
 @end table
 
+While calling functions of an MPX enabled program boundary registers have
+to be initialized before performing the call. Intended to avoid unexpected
+side effects, as receiving a bound violation signal while performing
+the operation.  Nevertheless is possible to change the boundary values if
+desired in placing a breakpoint at the end of the prologue and setting
+bound registers as wished.
+After the call is performed bound register might be keept or not for
+further investigations.  The behaviour of initializing bounds on returning
+from a program function calls can be controlled and vizualized via the commands
+@table @code
+@kindex set mpx-bnd-init-on-return
+When set to 1 bound registers will be initialized when returning from a
+calling a program function
+@kindex show mpx-bnd-init-on-return
+Show the state of mpx-bnd-init-on-return.
+@end table
+
 @node Alpha
 @subsection Alpha
 
diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.c b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
new file mode 100644
index 0000000..726bc76
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
@@ -0,0 +1,124 @@
+/*
+* Copyright 2012 Free Software Foundation, Inc.
+*
+* Contributed by Intel Corp. <christian.himpel@intel.com>,
+*                            <walfred.tedeschi@intel.com>
+*
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of the GNU General Public License as published by
+* the Free Software Foundation; either version 3 of the License, or
+* (at your option) any later version.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU General Public License for more details.
+*
+* You should have received a copy of the GNU General Public License
+* along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*
+* This test was converted from idb/test/td/runreg/Biendian/bi.c_bitfield
+*
+*/
+#include <stdio.h>
+#include "x86-cpuid.h"
+
+#define MYTYPE   int
+#define OUR_SIZE    5
+
+MYTYPE gx[OUR_SIZE];
+MYTYPE ga[OUR_SIZE];
+MYTYPE gb[OUR_SIZE];
+MYTYPE gc[OUR_SIZE];
+MYTYPE gd[OUR_SIZE];
+
+unsigned int
+have_mpx (void)
+{
+  unsigned int eax, ebx, ecx, edx;
+
+  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
+    {
+      if (__get_cpuid_max (0, NULL) < 7)
+	return 0;
+
+      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+
+      if ((ebx & bit_MPX) == bit_MPX)
+	return 1;
+      else
+	return 0;
+    }
+  return 0;
+}
+
+int
+bp1 (MYTYPE value)
+{
+  return 1;
+}
+
+int
+bp2 (MYTYPE value)
+{
+  return 1;
+}
+
+void
+upper (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
+{
+  MYTYPE value;
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);
+}
+
+MYTYPE *
+upper_ptr (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
+{
+  MYTYPE value;
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);  /* bkpt 2.  */
+  free (p);
+  p = calloc (OUR_SIZE * 2, sizeof (MYTYPE));
+  return ++p;
+}
+
+
+int
+main (void)
+{
+  if (have_mpx ())
+    {
+      MYTYPE sx[OUR_SIZE];
+      MYTYPE sa[OUR_SIZE];
+      MYTYPE sb[OUR_SIZE];
+      MYTYPE sc[OUR_SIZE];
+      MYTYPE sd[OUR_SIZE];
+      MYTYPE *x, *a, *b, *c, *d;
+
+      x = calloc (OUR_SIZE, sizeof (MYTYPE));
+      a = calloc (OUR_SIZE, sizeof (MYTYPE));
+      b = calloc (OUR_SIZE, sizeof (MYTYPE));
+      c = calloc (OUR_SIZE, sizeof (MYTYPE));
+      d = calloc (OUR_SIZE, sizeof (MYTYPE));
+
+      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
+      x = upper_ptr (sx, sa, sb, sc, sd, 0);
+
+      free (x); /* bkpt 3.  */
+      free (a);
+      free (b);
+      free (c);
+      free (d);
+    }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.exp b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
new file mode 100644
index 0000000..d293778
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
@@ -0,0 +1,90 @@
+# Copyright 2012 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+ [list debug nowarnings additional_flags=${comp_flags}]] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+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
+    }
+    -re ".*$gdb_prompt $" {
+        fail "check whether processor supports MPX"
+    }
+    timeout {
+        fail "check whether processor supports MPX (timeout)"
+    }
+}
+
+gdb_test_no_output "set confirm off"
+
+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)" "" "test the call of a function"
+gdb_test "p upper_ptr (x, a, b, c, d, 0)" "" "test the call of a function"
+
+set break "bkpt 2."
+gdb_breakpoint [ gdb_get_line_number "${break}" ]
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+set break "bkpt 3."
+gdb_breakpoint [ gdb_get_line_number "${break}" ]
+gdb_test "p \$bound0 = \$bnd0" "" "nm"
+gdb_test "return a"
+gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\ && \$bnd0\.lbound ==\
+\$bound0\.lbound\ \)" "0" "after return with initialization off"
+
+runto_main
+gdb_test_no_output "set mpx-bnd-init-on-return off" "Turn off initialization on\
+return"
+
+set break "bkpt 2."
+gdb_breakpoint [ gdb_get_line_number "${break}" ]
+gdb_continue_to_breakpoint "${break}" ".*${break}.*"
+set break "bkpt 3."
+gdb_breakpoint [ gdb_get_line_number "${break}" ]
+gdb_test "p \$bound0 = \$bnd0" "" "nm"
+gdb_test "return a"
+gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\ && \$bnd0\.lbound ==\
+\$bound0\.lbound\ \)" "1" "after return with initialization on"
+
+gdb_test "show mpx-bnd-init-on-return" "BND registers will not be initialized\
+on return." "double check of initialization on return"
+
+send_gdb "quit\n"
-- 
2.1.4

Re: [PATCH v1] ABI changes for Intel(R) MPX.

[Eli Zaretskii]

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Cc: gdb-patches@sourceware.org, Walfred Tedeschi <walfred.tedeschi@intel.com>
> Date: Mon, 26 Oct 2015 13:54:46 +0100
> 
> doc:
> 	gdb.texinfo: (Intel(R) Memory Protection Extensions): Add
> 	documentation on performing program function calls.

Is it possible to lose the "(R)" part here?  It gets in the way of
Emacs syntax highlighting of ChangeLog entries.

> +While calling functions of an MPX enabled program boundary registers have
> +to be initialized before performing the call. Intended to avoid unexpected
> +side effects, as receiving a bound violation signal while performing
> +the operation.  Nevertheless is possible to change the boundary values if
> +desired in placing a breakpoint at the end of the prologue and setting
> +bound registers as wished.
> +After the call is performed bound register might be keept or not for
> +further investigations.  The behaviour of initializing bounds on returning
> +from a program function calls can be controlled and vizualized via the commands
> +@table @code
> +@kindex set mpx-bnd-init-on-return
> +When set to 1 bound registers will be initialized when returning from a
> +calling a program function
> +@kindex show mpx-bnd-init-on-return
> +Show the state of mpx-bnd-init-on-return.
> +@end table

There are a few English and Texinfo issues here, so I suggest the
following minor rewording (please make sure I didn't change the
meaning of what you intended to say):

  While calling functions of an MPX enabled program, boundary registers have
  to be initialized before performing the call, to avoid unexpected side
  effects, such as a bound violation exception, while performing the
  operation.  Nevertheless, is possible to change the boundary values, if
  desired, by placing a breakpoint at the end of the prologue and setting
  bound registers as wished in the commands of that breakpoint.
  After the call is performed, a bound register might or might not be keept for
  further investigations.  The behaviour of initializing bounds on returning
  from a program function calls can be controlled and displayed using the
  following commands:

  @table @code
  @kindex set mpx-bnd-init-on-return
  @item set mpx-bnd-init-on-return
  When set to 1, bound registers will be initialized when returning from a
  calling a program function.
  @kindex show mpx-bnd-init-on-return
  @item show mpx-bnd-init-on-return
  Show the state of @code{mpx-bnd-init-on-return}.
  @end table

Thanks.

RE: [PATCH v1] ABI changes for Intel(R) MPX.

[Tedeschi, Walfred]

Eli,

Thanks a lot for your review!

I will use your version and remove also the (R).

Thanks again,
-Fred

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Eli Zaretskii
Sent: Monday, October 26, 2015 5:12 PM
To: Tedeschi, Walfred
Cc: palves@redhat.com; brobecker@adacore.com; gdb-patches@sourceware.org
Subject: Re: [PATCH v1] ABI changes for Intel(R) MPX.

> From: Walfred Tedeschi <walfred.tedeschi@intel.com>
> Cc: gdb-patches@sourceware.org, Walfred Tedeschi 
> <walfred.tedeschi@intel.com>
> Date: Mon, 26 Oct 2015 13:54:46 +0100
> 
> doc:
> 	gdb.texinfo: (Intel(R) Memory Protection Extensions): Add
> 	documentation on performing program function calls.

Is it possible to lose the "(R)" part here?  It gets in the way of Emacs syntax highlighting of ChangeLog entries.

> +While calling functions of an MPX enabled program boundary registers 
> +have to be initialized before performing the call. Intended to avoid 
> +unexpected side effects, as receiving a bound violation signal while 
> +performing the operation.  Nevertheless is possible to change the 
> +boundary values if desired in placing a breakpoint at the end of the 
> +prologue and setting bound registers as wished.
> +After the call is performed bound register might be keept or not for 
> +further investigations.  The behaviour of initializing bounds on 
> +returning from a program function calls can be controlled and 
> +vizualized via the commands @table @code @kindex set 
> +mpx-bnd-init-on-return When set to 1 bound registers will be 
> +initialized when returning from a calling a program function @kindex 
> +show mpx-bnd-init-on-return Show the state of mpx-bnd-init-on-return.
> +@end table

There are a few English and Texinfo issues here, so I suggest the following minor rewording (please make sure I didn't change the meaning of what you intended to say):

  While calling functions of an MPX enabled program, boundary registers have
  to be initialized before performing the call, to avoid unexpected side
  effects, such as a bound violation exception, while performing the
  operation.  Nevertheless, is possible to change the boundary values, if
  desired, by placing a breakpoint at the end of the prologue and setting
  bound registers as wished in the commands of that breakpoint.
  After the call is performed, a bound register might or might not be keept for
  further investigations.  The behaviour of initializing bounds on returning
  from a program function calls can be controlled and displayed using the
  following commands:

  @table @code
  @kindex set mpx-bnd-init-on-return
  @item set mpx-bnd-init-on-return
  When set to 1, bound registers will be initialized when returning from a
  calling a program function.
  @kindex show mpx-bnd-init-on-return
  @item show mpx-bnd-init-on-return
  Show the state of @code{mpx-bnd-init-on-return}.
  @end table

Thanks.
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, Prof. Dr. Hermann Eul
Chairperson of the Supervisory Board: Tiffany Doon Silva
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v1] ABI changes for Intel(R) MPX.

[Joel Brobecker]

> Code reflects what is presented in the ABI document:
> https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI

Can you say which document contains the info in question.
I looked at all 4 documents there, and in particular at
"x86-64 psABI revision 249", but couldn't find this new
"pointer" class. I may have missed it, given that "pointer"
is a fairly common term (understatement).

Regardless, I started taking a look at the patch, at least for
superficial stuff... I'm sorry, it's a bit of a lengthy email,
but it's mostly trivial stuff. Please don't feel like I am
picking on you, it's just that (a) I don't know the first thing
about MPX, and b) GDB's coding style is fairly detailed and
particular. You should get the hang of it fairly quickly.

> Here new class POINTER was added.  GDB code is modified to mirror
> this new class.
> 
> New set and show command was added as option for initializing bounds when
> returning from inferior calls or not.
> 
> 2015-08-25  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
>         * amd64-tdep.c (amd64_reg_class): Add new class AMD64_POINTER.
>         (amd64_merge_classes): Add AMD64_POINTER to merging classes rules.
>         (amd64_classify): Add AMD64_POINTER.
>         (mpx_bnd_init_on_return): New.
>         (amd64_return_value): Add bound register.
>         (amd64_return_value): Set bound values for returning.
>         (amd64_push_arguments): Add new AMD64_POINTER class.
>         (amd64_push_dummy_call): Initialize bound registers before call.
>         (show_mpx_init_on_return): New funtion.
>         (amd64_init_abi): Add new commands set/show mpx-bnd-init-on-return.
> 
> doc:
>         gdb.texinfo: (Intel(R) Memory Protection Extensions): Add
>         documentation on performing program function calls.
> 
> testsuite:
>         amd64-mpx-call.c: New.
>         amd64-mpx-call.exp: New.
> 
> ---
>  gdb/amd64-tdep.c                          |  97 +++++++++++++++++++++--
>  gdb/doc/gdb.texinfo                       |  17 ++++
>  gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 124 ++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  90 ++++++++++++++++++++++
>  4 files changed, 321 insertions(+), 7 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.c
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-mpx-call.exp
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 0fa4d54..2fa555d 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -473,6 +473,7 @@ amd64_pseudo_register_write (struct gdbarch *gdbarch,
>  
>  enum amd64_reg_class
>  {
> +  AMD64_POINTER,
>    AMD64_INTEGER,
>    AMD64_SSE,
>    AMD64_SSEUP,
> @@ -504,18 +505,22 @@ amd64_merge_classes (enum amd64_reg_class class1, enum amd64_reg_class class2)
>    if (class1 == AMD64_MEMORY || class2 == AMD64_MEMORY)
>      return AMD64_MEMORY;
>  
> -  /* Rule (d): If one of the classes is INTEGER, the result is INTEGER.  */
> +  /* Rule (d): If one of the classes is POINTER, the result is POINTER.  */
> +  if (class1 == AMD64_POINTER || class2 == AMD64_POINTER)
> +    return AMD64_POINTER;
> +
> +  /* Rule (e): If one of the classes is INTEGER, the result is INTEGER.  */
>    if (class1 == AMD64_INTEGER || class2 == AMD64_INTEGER)
>      return AMD64_INTEGER;
>  
> -  /* Rule (e): If one of the classes is X87, X87UP, COMPLEX_X87 class,
> +  /* Rule (f): If one of the classes is X87, X87UP, COMPLEX_X87 class,
>       MEMORY is used as class.  */
>    if (class1 == AMD64_X87 || class1 == AMD64_X87UP
>        || class1 == AMD64_COMPLEX_X87 || class2 == AMD64_X87
>        || class2 == AMD64_X87UP || class2 == AMD64_COMPLEX_X87)
>      return AMD64_MEMORY;
>  
> -  /* Rule (f): Otherwise class SSE is used.  */
> +  /* Rule (g): Otherwise class SSE is used.  */
>    return AMD64_SSE;
>  }
>  
> @@ -648,14 +653,17 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
>  
>    theclass[0] = theclass[1] = AMD64_NO_CLASS;
>  
> +  /* Arguments of types (pointer and reference) are of the class pointer.  */
> +  if (code == TYPE_CODE_PTR || code == TYPE_CODE_REF)
> +    theclass[0] = AMD64_POINTER;
> +
>    /* Arguments of types (signed and unsigned) _Bool, char, short, int,
>       long, long long, and pointers are in the INTEGER class.  Similarly,
>       range types, used by languages such as Ada, are also in the INTEGER
>       class.  */
> -  if ((code == TYPE_CODE_INT || code == TYPE_CODE_ENUM
> +  else if ((code == TYPE_CODE_INT || code == TYPE_CODE_ENUM
>         || code == TYPE_CODE_BOOL || code == TYPE_CODE_RANGE
> -       || code == TYPE_CODE_CHAR
> -       || code == TYPE_CODE_PTR || code == TYPE_CODE_REF)
> +       || code == TYPE_CODE_CHAR)
>        && (len == 1 || len == 2 || len == 4 || len == 8))
>      theclass[0] = AMD64_INTEGER;
>  
> @@ -705,16 +713,22 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
>      amd64_classify_aggregate (type, theclass);
>  }
>  
> +static int mpx_bnd_init_on_return = 1;

New globals and new function must be documented via an introductory
comment.

> +
>  static enum return_value_convention
>  amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>                     struct type *type, struct regcache *regcache,
>                     gdb_byte *readbuf, const gdb_byte *writebuf)
>  {
>    enum amd64_reg_class theclass[2];
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    int len = TYPE_LENGTH (type);
>    static int integer_regnum[] = { AMD64_RAX_REGNUM, AMD64_RDX_REGNUM };
>    static int sse_regnum[] = { AMD64_XMM0_REGNUM, AMD64_XMM1_REGNUM };
> +  static int bnd_regnum[] = { AMD64_BND0R_REGNUM, AMD64_BND0R_REGNUM,
> +      AMD64_BND2R_REGNUM, AMD64_BND3R_REGNUM };

I think we would want to format the following as follow:

  static int bnd_regnum[]
    = {AMD64_BND0R_REGNUM, AMD64_BND0R_REGNUM, AMD64_BND2R_REGNUM,
       AMD64_BND3R_REGNUM};

(no space before '}', even though we made that mistake on the previous
static variables.

>    int integer_reg = 0;
> +  int bnd_reg = 0;
>    int sse_reg = 0;
>    int i;
>  
> @@ -742,6 +756,20 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>  
>           regcache_raw_read_unsigned (regcache, AMD64_RAX_REGNUM, &addr);
>           read_memory (addr, readbuf, TYPE_LENGTH (type));
> +
> +         /*  If the class is memory, Boundary has to be stored in the bnd0 in
> +             the case the application is MPX enabled.
> +             In order to be return a boundary value mpx_bnd_init_on_return
> +             has to be 0.  Otherwise the actual value present in the register
> +             will be returned.  */
> +
> +         if (writebuf && mpx_bnd_init_on_return && AMD64_BND0R_REGNUM > 0)
> +           {
> +             gdb_byte bnd_buf[16];
> +
> +             memset (bnd_buf, 0, 16);
> +             regcache_raw_write (regcache, AMD64_BND0R_REGNUM, bnd_buf);
> +           }

I'm sorry, but I am completely confused by the comment. After reading
Eli's suggested rephrasing of your documentation update, I think
I understand the code, but actually not quite.

First, this only takes effect when WRITEBUF is not NULL, which only
happens when using the "return" command. That was unclear to me when
reading the proposed documentation and this commit's revision log.

For the revision log, it would be useful to say when that new class
is used, what the current behavior is (before applying your patch),
why it is wrong, and what the behavior is afterwards - usually,
adding copies of GDB output helps making things clearer. Also,
please be more explicit in the name of the options, and how they
influence the behavior. I would say, provide a copy of the behavior
when the new setting is on, and when it is off.

In terms of the new setting's documentation in the GDB manual,
I got confused by "calling functions", which to me meant GDB
making an inferior function call, whereas I think now, that this meant
a regular function call in a program. Also, the next sentence say
"have to be initialized", which to me implies something either
the user or GDB has to do before it can make the call. This is
further re-inforced by the fact that the following sentence clearly
discusses what a user can do to change what the program does.
Did you actually mean to say something like the following?

   In MPX-enabled programs, boundary registers are always initialized
   before calling functions, to avoid unexpected side-effects during
   the function call (Eg. bound violation exceptions).

   (what does "while performing the operation" mean? Does it mean
   "during the function call"?)

   Are bound registers actually _always_ initialized? I have
   a feeling that this is more a "sometimes". Perhaps a better
   phrasing for the above is "boundary registers are expected to
   be set before calling functions". Is it "set" or really
   "initialized" (as in set to zero)?

   Also, can there really be any side-effect other than a bound
   violation when it comes to setting the bound registers.

I think another part that got me in that description is that I think
this reference to the bound registers being set and the fact that
there is an easy way to force them to another value is only loosely
related to what the patch is trying to do, and what the new
mpx-bnd-init-on-return setting allows you to do if set. I think
it would be clearer if made that text (up to "as wished") as its
own paragraph, and then start another paragraph about how you
can change the value of those bound registers in the "return" command.

And lastly, the command and the documentation say "initialize",
but it's not clear what value is used. I suggest we say either
"initialize to zero", or "reset". I think the command name would
also gain from using the term "reset" instead of "init" as well.

>         }
>  
>        return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> @@ -777,6 +805,7 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>    for (i = 0; len > 0; i++, len -= 8)
>      {
>        int regnum = -1;
> +      int bnd_reg_count = -1;
>        int offset = 0;
>  
>        switch (theclass[i])
> @@ -787,6 +816,13 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>           regnum = integer_regnum[integer_reg++];
>           break;
>  
> +       case AMD64_POINTER:
> +         /* 3. If the class is POINTER, same rules of integer applies.  */

same rules _as_ integer _apply_


> +         regnum = integer_regnum[integer_reg++];
> +         /* But we need to allocate a bnd reg.  */
> +         bnd_reg_count = bnd_regnum[bnd_reg++];
> +         break;
> +
>         case AMD64_SSE:
>           /* 4. If the class is SSE, the next available SSE register
>               of the sequence %xmm0, %xmm1 is used.  */
> @@ -835,6 +871,17 @@ amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>                                  writebuf + i * 8);
>      }
>  
> +  if (I387_BND0R_REGNUM (tdep) > 0)
> +    {
> +      gdb_byte bnd_buf[16];
> +      int i, init_bnd;
> +
> +      memset (bnd_buf, 0, 16);
> +      if (writebuf && mpx_bnd_init_on_return)
> +       for (i = 0; i < I387_NUM_BND_REGS; i++)
> +         regcache_raw_write (regcache, AMD64_BND0R_REGNUM + i, bnd_buf);
> +    }
> +
>    return RETURN_VALUE_REGISTER_CONVENTION;
>  }
>  \f
> @@ -888,7 +935,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
>           this argument.  */
>        for (j = 0; j < 2; j++)
>         {
> -         if (theclass[j] == AMD64_INTEGER)
> +         if (theclass[j] == AMD64_INTEGER || theclass[j] == AMD64_POINTER)
>             needed_integer_regs++;
>           else if (theclass[j] == AMD64_SSE)
>             needed_sse_regs++;
> @@ -919,6 +966,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
>  
>               switch (theclass[j])
>                 {
> +               case AMD64_POINTER:
>                 case AMD64_INTEGER:
>                   regnum = integer_regnum[integer_reg++];
>                   break;
> @@ -978,8 +1026,23 @@ amd64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>                        int struct_return, CORE_ADDR struct_addr)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    gdb_byte buf[8];
>  
> +  /* When MPX is enabled all bnd registers have to be Initialized before the
> +     call.  This avoids undesired bound violations while executing the call.
> +     At this point in time we don need to worry about the   */

Some typos, unexpected capital letter, missing comma, and the end of
the last sentence was truncated. As a starter, I suggest:

  /* When MPX is enabled, all bnd registers must be initialized before
     the call.  This avoids undesired bound violations during the
     function's execution.  At this point in time we do not need to
     worry about the


> +
> +  if (I387_BND0R_REGNUM (tdep) > 0)
> +    {
> +      gdb_byte bnd_buf[16];
> +      int i;
> +
> +      memset (bnd_buf, 0, 16);
> +      for (i = 0; i < I387_NUM_BND_REGS; i++)
> +       regcache_raw_write (regcache, AMD64_BND0R_REGNUM + i, bnd_buf);
> +    }
> +
>    /* Pass arguments.  */
>    sp = amd64_push_arguments (regcache, nargs, args, sp, struct_return);
>  
> @@ -2944,6 +3007,18 @@ static const int amd64_record_regmap[] =
>    AMD64_DS_REGNUM, AMD64_ES_REGNUM, AMD64_FS_REGNUM, AMD64_GS_REGNUM
>  };
>  
> +static void
> +show_mpx_init_on_return (struct ui_file *file, int from_tty,
> +                         struct cmd_list_element *c, const char *value)

Make sure you documeant each and every new function you add.
In this particular case, only a trivial intro comment is necessary.
For instance:

/* Implement the "show mpx-bnd-init-on-return" command.  */

It would also be nice if your function name matched the name
of the command, eg show_mpx_bnd_init_on_return.

> +  if (mpx_bnd_init_on_return > 0)
> +    fprintf_filtered (file,
> +                   _("BND registers will be initialized on return.\n"));
> +  else
> +    fprintf_filtered (file,
> +                   _("BND registers will not be initialized on return.\n"));

The " > 0" is a little odd for a boolean setting?!?

>    if (tdesc_find_feature (tdesc, "org.gnu.gdb.i386.mpx") != NULL)
>      {
> +      add_setshow_boolean_cmd ("mpx-bnd-init-on-return", no_class,
> +                               &mpx_bnd_init_on_return, _("\
> +Set the bnd registers to INIT state when returning from a call."), _("\
> +Show the state of the mpx-bnd-init-on-return."),
> +                           NULL,
> +                           NULL,
> +                           show_mpx_init_on_return,
> +                           &setlist, &showlist);

Is "INIT state" something that is defined in the reference manual
to mean something specific (such as "set to zero")? You might want
to expand a bit the documentation for people not so versed in MPX.

>        tdep->mpx_register_names = amd64_mpx_names;
>        tdep->bndcfgu_regnum = AMD64_BNDCFGU_REGNUM;
>        tdep->bnd0r_regnum = AMD64_BND0R_REGNUM;
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 3c1f785..1d8e667 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -22042,6 +22042,23 @@ whose bounds are to be changed, @var{lbound} and @var{ubound} are new values
>  for lower and upper bounds respectively.
>  @end table
>  
> +While calling functions of an MPX enabled program boundary registers have
> +to be initialized before performing the call. Intended to avoid unexpected
> +side effects, as receiving a bound violation signal while performing
> +the operation.  Nevertheless is possible to change the boundary values if
> +desired in placing a breakpoint at the end of the prologue and setting
> +bound registers as wished.
> +After the call is performed bound register might be keept or not for
> +further investigations.  The behaviour of initializing bounds on returning
> +from a program function calls can be controlled and vizualized via the commands
> +@table @code
> +@kindex set mpx-bnd-init-on-return
> +When set to 1 bound registers will be initialized when returning from a
> +calling a program function
> +@kindex show mpx-bnd-init-on-return
> +Show the state of mpx-bnd-init-on-return.
> +@end table
> +
>  @node Alpha
>  @subsection Alpha
>  
> diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.c b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
> new file mode 100644
> index 0000000..726bc76
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
> @@ -0,0 +1,124 @@
> +/*
> +* Copyright 2012 Free Software Foundation, Inc.
> +*
> +* Contributed by Intel Corp. <christian.himpel@intel.com>,
> +*                            <walfred.tedeschi@intel.com>
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the GNU General Public License as published by
> +* the Free Software Foundation; either version 3 of the License, or
> +* (at your option) any later version.
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*
> +* This test was converted from idb/test/td/runreg/Biendian/bi.c_bitfield
> +*
> +*/

Please remove the leading '*' in the command (GNU Coding Standards).
The date in the copyright should include 2015, but in practice should
say 2012-2015.

Can you add a (C) after Copyright and before 2015? (also GCS).

The reference to "idb/[etc]" should probably be removed, as I am
guessing this is something that's not public. Or else, provided
more info, assuming that this is useful info to understand this
testcase.

> +#include <stdio.h>
> +#include "x86-cpuid.h"
> +
> +#define MYTYPE   int

Is there really any value in using MYTYPE rather than just "int"
directly? The only thing that this inspires me is that it obscures
the rest of the code, because you have to remember that MYTYPE is
just an int.

> +#define OUR_SIZE    5
> +
> +MYTYPE gx[OUR_SIZE];
> +MYTYPE ga[OUR_SIZE];
> +MYTYPE gb[OUR_SIZE];
> +MYTYPE gc[OUR_SIZE];
> +MYTYPE gd[OUR_SIZE];
> +
> +unsigned int
> +have_mpx (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +       return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ebx & bit_MPX) == bit_MPX)
> +       return 1;
> +      else
> +       return 0;
> +    }
> +  return 0;
> +}
> +
> +int
> +bp1 (MYTYPE value)
> +{
> +  return 1;
> +}
> +
> +int
> +bp2 (MYTYPE value)
> +{
> +  return 1;
> +}
> +
> +void
> +upper (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)

No space after '*' when it means a pointer (as opposed to '*' as the
binary operator). There are identical issues in the rest of the file,
so can you fix those without me pointing out each one of them?

> +{
> +  MYTYPE value;
> +  value = *(p + len);

We try to make our testcases follow the same style as GDB's code,
so can you add an empty line after local variable declarations,
please? Same for all the other functions later in that file.

> +  value = *(a + len);
> +  value = *(b + len);
> +  value = *(c + len);
> +  value = *(d + len);
> +}
> +
> +MYTYPE *
> +upper_ptr (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
> +{
> +  MYTYPE value;
> +  value = *(p + len);
> +  value = *(a + len);
> +  value = *(b + len);
> +  value = *(c + len);
> +  value = *(d + len);  /* bkpt 2.  */
> +  free (p);
> +  p = calloc (OUR_SIZE * 2, sizeof (MYTYPE));
> +  return ++p;
> +}
> +
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      MYTYPE sx[OUR_SIZE];
> +      MYTYPE sa[OUR_SIZE];
> +      MYTYPE sb[OUR_SIZE];
> +      MYTYPE sc[OUR_SIZE];
> +      MYTYPE sd[OUR_SIZE];
> +      MYTYPE *x, *a, *b, *c, *d;
> +
> +      x = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      a = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      b = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      c = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      d = calloc (OUR_SIZE, sizeof (MYTYPE));
> +
> +      upper (sx, sa, sb, sc, sd, 0);  /* bkpt 1.  */
> +      x = upper_ptr (sx, sa, sb, sc, sd, 0);
> +
> +      free (x); /* bkpt 3.  */
> +      free (a);
> +      free (b);
> +      free (c);
> +      free (d);
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-mpx-call.exp b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
> new file mode 100644
> index 0000000..d293778
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
> @@ -0,0 +1,90 @@
> +# Copyright 2012 Free Software Foundation, Inc.

Same as above for the copyright date.

> +#
> +# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> + [list debug nowarnings additional_flags=${comp_flags}]] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +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
> +    }
> +    -re ".*$gdb_prompt $" {
> +        fail "check whether processor supports MPX"
> +    }
> +    timeout {
> +        fail "check whether processor supports MPX (timeout)"
> +    }
> +}

We do not use send_gdb and gdb_expect in testcases unless there is
absolutely no other way. In your case, you can do the same using
gdb_test_multiple.
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Performing_a_Test

> +gdb_test_no_output "set confirm off"

Why do you need this?

> +set break "bkpt 1."
> +gdb_breakpoint [ gdb_get_line_number "${break}" ]

no space after the '[' or before the ']' in function calls.
It should be:

        gdb_breakpoint [gdb_get_line_number "${break}"]

(I will let you fix the rest of the testcase up)

> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +gdb_test "p upper (x, a, b, c, d, 0)" "" "test the call of a function"

If the test should generate no output, use "gdb_test_no_output".

> +gdb_test "p upper_ptr (x, a, b, c, d, 0)" "" "test the call of a function"

Ditto (I will let you find any other test that should be changed
as well).

Also, test names should be unique. You're using the same string
as the test before.
https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_executables_are_unique

It doesn't look like you're going to call
those two functions more than once, so I'd just remove the explicit
test name, and let gdb_test use the default one, which is the
command that was used to perform the test.

> +set break "bkpt 2."
> +gdb_breakpoint [ gdb_get_line_number "${break}" ]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +set break "bkpt 3."
> +gdb_breakpoint [ gdb_get_line_number "${break}" ]
> +gdb_test "p \$bound0 = \$bnd0" "" "nm"

"nm"?
(again, I'll let you fix other instances of this in the rest of
the testcase)

> +gdb_test "return a"
> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\ && \$bnd0\.lbound ==\
> +\$bound0\.lbound\ \)" "0" "after return with initialization off"

I would split the test. This is going to help two ways:
  (1) make the expression a little shorter, thus fitting in 80 chars
  (2) if something goes wrong, you'll know which side(s) of the &&
      expression actually fail.

Also, when checking the output of a print command, make sure to include
a match of the ' = ' before the number you want to match.  Otherwise,
anything that ends in '0' will return a success.

"after return with initialization off" on its own is a little obscure.
Suggest using "with_test_prefix" as suggested in the "make sure
tests are unique" doco referenced above.

> +runto_main
> +gdb_test_no_output "set mpx-bnd-init-on-return off" "Turn off initialization on\
> +return"

I would split the line of code a little differently. The following
is fairly typical:

gdb_test_no_output "set mpx-bnd-init-on-return off" \
    Turn off initialization on return"

(small gotcha - no space before '\' meaning you actually wrote
"onreturnr" rather than "on return")

> +set break "bkpt 2."
> +gdb_breakpoint [ gdb_get_line_number "${break}" ]
> +gdb_continue_to_breakpoint "${break}" ".*${break}.*"
> +set break "bkpt 3."
> +gdb_breakpoint [ gdb_get_line_number "${break}" ]
> +gdb_test "p \$bound0 = \$bnd0" "" "nm"
> +gdb_test "return a"
> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\ && \$bnd0\.lbound ==\
> +\$bound0\.lbound\ \)" "1" "after return with initialization on"

Ditto as above, I would split the condition.

> +gdb_test "show mpx-bnd-init-on-return" "BND registers will not be initialized\
> +on return." "double check of initialization on return"
> +
> +send_gdb "quit\n"

You don't need to send "quit", so you should just remove it.

-- 
Joel

[PATCH obv] Fix non stopping breakpoint on newer compilers.

[Walfred Tedeschi]

The breakpoint presented in the return statement was not activated while
compiling the test with gcc 4.9.2.  Added a dummy statement to allow the
breakpoint again.

2015-10-14  Walfred Tedeschi  <walfred.tedeschi@intel.com>

gdb/testsuite:

	* i386-mpx-map.c (foo): Add dummy statement to trigger breakpoint.

---
 gdb/testsuite/gdb.arch/i386-mpx-map.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/i386-mpx-map.c b/gdb/testsuite/gdb.arch/i386-mpx-map.c
index 8a9094c..ee7f11b 100644
--- a/gdb/testsuite/gdb.arch/i386-mpx-map.c
+++ b/gdb/testsuite/gdb.arch/i386-mpx-map.c
@@ -70,8 +70,9 @@ foo (T *p)
 #if defined  __GNUC__ && !defined __INTEL_COMPILER
   __bnd_store_ptr_bounds (x, &x);
 #endif
-
-  return;			/* after-assign */
+  /* Dummy assign.  */
+  x = x + 1;			/* after-assign */
+  return;
 }
 
 int
-- 
2.1.4

Re: [PATCH obv] Fix non stopping breakpoint on newer compilers.

[Joel Brobecker]

> The breakpoint presented in the return statement was not activated while
> compiling the test with gcc 4.9.2.  Added a dummy statement to allow the
> breakpoint again.
> 
> 2015-10-14  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> gdb/testsuite:
> 
> 	* i386-mpx-map.c (foo): Add dummy statement to trigger breakpoint.

Looks good to me.

-- 
Joel

[PATCH v1] Intel(R) MPX - Bound violation handling.

[Walfred Tedeschi]

With Intel(R) Memory Protection Extensions it was introduced the concept of
boundary violation.  A boundary violations is presented to the inferior as
a segmentation fault having as sigcode the value 3.  This patch adds a
handler for a boundary violation extending the information displayed
when a bound violation is presented to the inferior (segfault with code 3).
In this case the debugger will also display the kind of violation upper or
lower the violated bounds and the address accessed.

There some open points to be discussed though before asking permission to
commit.
They are:
1. infrun.c (process_segmentation_faults): The inferior is stopped to
allow doing some evaluations. I have seen no side effect on doing that,
on the other hand it does not look so natural to me.
2. i386-linux-tdep.c (i386_mpx_bound_violation_handler): I was wondering if
the right place for it wouldn't be the linux-tdep.c as it is done for the
siginfo things.  This is a new structure in the kernel. Doing at the
linux-tdep.c documentation should be simplified and all architectures
providing that functionality in a different way only have to set the right
function pointer.
3. Still need to add documentation, expected mi changes and siginfo changes

Thanks a lot for your support!

Changelog:

2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>

	* amd64-linux-tdep.c (amd64_linux_init_abi_common):
	Add handler for bound violation signal.
	* gdbarch.sh (bound_violation_handler): New.
	* 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 (process_segmentation_faults): New.
	(print_signal_received_reason): Use process_segmentation_faults.

testsuite/gdb.arch
	* i386-mpx-sigsegv.c: New.
	* i386-mpx-sigsegv.exp: New.
	* i386-mpx-simple_segv.c: New.
	* i386-mpx-simple_segv.exp: New.

---
 gdb/amd64-linux-tdep.c                          |   3 +
 gdb/gdbarch.c                                   |  32 ++++++
 gdb/gdbarch.h                                   |  11 ++
 gdb/gdbarch.sh                                  |   7 ++
 gdb/i386-linux-tdep.c                           |  66 ++++++++++++
 gdb/i386-linux-tdep.h                           |   5 +
 gdb/i386-tdep.c                                 |   2 +-
 gdb/i386-tdep.h                                 |   3 +
 gdb/infrun.c                                    |  18 ++++
 gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c       | 128 +++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp     |  95 +++++++++++++++++
 gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c   |  70 +++++++++++++
 gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp | 130 ++++++++++++++++++++++++
 13 files changed, 569 insertions(+), 1 deletion(-)
 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/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
index 021dca6..12e9e01 100644
--- a/gdb/amd64-linux-tdep.c
+++ b/gdb/amd64-linux-tdep.c
@@ -1838,6 +1838,9 @@ amd64_linux_init_abi_common(struct gdbarch_info info, struct gdbarch *gdbarch)
 
   set_gdbarch_process_record (gdbarch, i386_process_record);
   set_gdbarch_process_record_signal (gdbarch, amd64_linux_record_signal);
+
+  set_gdbarch_bound_violation_handler(gdbarch,
+                                      i386_mpx_bound_violation_handler);
 }
 
 static void
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index f04eef9..e63f141 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, long si_code)
+{
+  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 (uiout, si_code);
+}
+
+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 2e4ed3e..c2dd767 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -64,6 +64,7 @@ struct ravenscar_arch_ops;
 struct elf_internal_linux_prpsinfo;
 struct mem_range;
 struct syscalls_info;
+struct ui_out;
 
 #include "regcache.h"
 
@@ -300,6 +301,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);
 
+/* Bound violation can be handled differently accross architectures.
+   UIOUT is the output stream where the handler will place information.
+   si_code is the value of the field with the same name in the siginfo structure. */
+
+extern int gdbarch_bound_violation_handler_p (struct gdbarch *gdbarch);
+
+typedef void (gdbarch_bound_violation_handler_ftype) (struct ui_out *uiout, long si_code);
+extern void gdbarch_bound_violation_handler (struct gdbarch *gdbarch, struct ui_out *uiout, long si_code);
+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 a13d9b9..b4d231c 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
 
+# Bound violation can be handled differently accross architectures.
+# UIOUT is the output stream where the handler will place information.
+# si_code is the value of the field with the same name in the siginfo structure.
+F:void:bound_violation_handler:struct ui_out *uiout, long si_code:uiout, si_code
+
 # 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).
@@ -696,6 +701,7 @@ M:void:iterate_over_regset_sections:iterate_over_regset_sections_cb *cb, void *c
 # Create core file notes
 M:char *:make_corefile_notes:bfd *obfd, int *note_size:obfd, note_size
 
+
 # The elfcore writer hook to use to write Linux prpsinfo notes to core
 # files.  Most Linux architectures use the same prpsinfo32 or
 # prpsinfo64 layouts, and so won't need to provide this hook, as we
@@ -1247,6 +1253,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 d02c527..fe3b52f 100644
--- a/gdb/i386-linux-tdep.c
+++ b/gdb/i386-linux-tdep.c
@@ -384,6 +384,68 @@ i386_canonicalize_syscall (int syscall)
     return gdb_sys_no_syscall;
 }
 
+/* Code for faulty boundary has to be introduced.  */
+#define SIG_CODE_BONDARY_FAULT 3
+
+void
+i386_mpx_bound_violation_handler (struct ui_out *uiout, long si_code)
+{
+  CORE_ADDR lower_bound, upper_bound, access;
+  int is_upper;
+
+  if (!i386_mpx_enabled () || si_code != SIG_CODE_BONDARY_FAULT)
+    return;
+
+  lower_bound = parse_and_eval_long (
+      "$_siginfo._sifields._sigfault._addr_bnd._lower\n");
+  upper_bound = parse_and_eval_long (
+      "$_siginfo._sifields._sigfault._addr_bnd._upper\n");
+  access = parse_and_eval_long (
+      "$_siginfo._sifields._sigfault.si_addr\n");
+  is_upper = (access > upper_bound ? 1 : 0);
+
+  if (ui_out_is_mi_like_p (uiout))
+    {
+      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_field_fmt (uiout,"lower-bound",
+			"0x%lx",
+			lower_bound);
+      ui_out_field_fmt (uiout,"upper-bound",
+			"0x%lx",
+			upper_bound);
+      ui_out_field_fmt (uiout,"bound-access",
+			"0x%lx",
+			access);
+    }
+  else
+    {
+      if (is_upper)
+	ui_out_field_string (uiout, "sigcode-meaning",
+			     "\nupper bound violation");
+      else
+	ui_out_field_string (uiout, "sigcode-meaning",
+			     "\nlower bound violation");
+
+      ui_out_field_string (uiout, "bounds",
+			   " - bounds");
+      ui_out_field_fmt (uiout,"bound-values",
+			" {lbound = 0x%lx, ubound = 0x%lx}",
+			lower_bound,
+			upper_bound);
+      ui_out_field_fmt (uiout,"bound-access",
+			" accessing 0x%lx",
+			access);
+    }
+
+  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
@@ -995,6 +1057,10 @@ i386_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_xml_syscall_file_name (gdbarch, XML_SYSCALL_FILENAME_I386);
   set_gdbarch_get_syscall_number (gdbarch,
                                   i386_linux_get_syscall_number);
+
+  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 5ac08d3..5655081 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 ui_out *uiout, long si_code);
+
 /* 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 b8edff6..6349389 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -8641,7 +8641,7 @@ i386_mpx_bd_base (void)
 
 /* 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 95288ba..4e81288 100644
--- a/gdb/i386-tdep.h
+++ b/gdb/i386-tdep.h
@@ -420,6 +420,9 @@ 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.  */
+extern int i386_mpx_enabled (void);
+
 \f
 
 /* Functions and variables exported from i386bsd-tdep.c.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 0c268ff..3a323ae 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7744,6 +7744,20 @@ print_exited_reason (struct ui_out *uiout, int exitstatus)
     }
 }
 
+
+static void
+process_segmentation_faults (struct ui_out * uiout)
+{
+  long si_code;
+  struct regcache *regcache = get_current_regcache ();
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
+
+  set_running (user_visible_resume_ptid (1), 0);
+  si_code = parse_and_eval_long ("$_siginfo.si_code\n");
+  if (gdbarch_bound_violation_handler_p (gdbarch))
+    gdbarch_bound_violation_handler (gdbarch, uiout, si_code);
+}
+
 void
 print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
 {
@@ -7773,6 +7787,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)
+	process_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..5f20aa2
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c
@@ -0,0 +1,128 @@
+/*
+* Copyright 2012 Free Software Foundation, Inc.
+*
+* Contributed by Intel Corp. <christian.himpel@intel.com>,
+*                            <walfred.tedeschi@intel.com>
+*
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of the GNU General Public License as published by
+* the Free Software Foundation; either version 3 of the License, or
+* (at your option) any later version.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU General Public License for more details.
+*
+* You should have received a copy of the GNU General Public License
+* along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*
+* This test was converted from idb/test/td/runreg/Biendian/bi.c_bitfield
+*
+*/
+#include <stdio.h>
+#include "x86-cpuid.h"
+
+
+#define MYTYPE   int
+#define OUR_SIZE    5
+
+MYTYPE gx[OUR_SIZE];
+MYTYPE ga[OUR_SIZE];
+MYTYPE gb[OUR_SIZE];
+MYTYPE gc[OUR_SIZE];
+MYTYPE gd[OUR_SIZE];
+
+unsigned int
+have_mpx (void)
+{
+  unsigned int eax, ebx, ecx, edx;
+
+  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
+    {
+      if (__get_cpuid_max (0, NULL) < 7)
+	return 0;
+
+      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+
+      if ((ebx & bit_MPX) == bit_MPX)
+	return 1;
+      else
+	return 0;
+    }
+  return 0;
+}
+
+int
+bp1 (MYTYPE value)
+{
+  return 1;
+}
+
+int
+bp2 (MYTYPE value)
+{
+  return 1;
+}
+
+void
+upper (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
+{
+  MYTYPE value;
+  value = *(p + len);
+  value = *(a + len);
+  value = *(b + len);
+  value = *(c + len);
+  value = *(d + len);
+}
+
+void
+lower (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
+{
+  MYTYPE value;
+  value = *(p - len);
+  value = *(a - len);
+  value = *(b - len);
+  value = *(c - len);
+  bp2 (value);
+  value = *(d - len);
+}
+
+
+int
+main (void)
+{
+  if (have_mpx ())
+    {
+      MYTYPE sx[OUR_SIZE];
+      MYTYPE sa[OUR_SIZE];
+      MYTYPE sb[OUR_SIZE];
+      MYTYPE sc[OUR_SIZE];
+      MYTYPE sd[OUR_SIZE];
+      MYTYPE *x, *a, *b, *c, *d;
+
+      x = calloc (OUR_SIZE, sizeof (MYTYPE));
+      a = calloc (OUR_SIZE, sizeof (MYTYPE));
+      b = calloc (OUR_SIZE, sizeof (MYTYPE));
+      c = calloc (OUR_SIZE, sizeof (MYTYPE));
+      d = calloc (OUR_SIZE, sizeof (MYTYPE));
+
+      upper (x, a, b, c, d, OUR_SIZE + 2);
+      upper (sx, sa, sb, sc, sd, OUR_SIZE + 2);
+      upper (gx, ga, gb, gc, gd, OUR_SIZE + 2);
+      lower (x, a, b, c, d, 1);
+      lower (sx, sa, sb, sc, sd, 1);
+      bp1 (*x);
+      lower (gx, ga, gb, gc, gd, 1);
+
+      free (x);
+      free (a);
+      free (b);
+      free (c);
+      free (d);
+    }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
new file mode 100644
index 0000000..f689018
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
@@ -0,0 +1,95 @@
+# Copyright 2012 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+ [list debug nowarnings additional_flags=${comp_flags}]] } {
+    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 " {
+        verbose "processor does not support MPX; skipping MPX tests"
+        return
+    }
+    -re ".*$gdb_prompt $" {
+        fail "check whether processor supports MPX"
+    }
+    timeout {
+        fail "check whether processor supports MPX (timeout)"
+    }
+}
+
+set segv_lower_bound ".*Program received signal SIGSEGV,\
+        Segmentation fault\r\nlower bound violation - bounds \\\{lbound\
+        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
+        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
+
+set segv_upper_bound ".*Program received signal SIGSEGV,\
+        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
+        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
+        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
+
+for {set i 0} {$i < 15} {incr i} {
+    set message "MPX signal segv Upper: ${i}"
+    send_gdb "continue\n"
+    gdb_expect {
+         -re $segv_upper_bound
+        { pass "$message" }
+        -re ".*$inferior_exited_re normally.*$gdb_prompt $"
+        {
+          fail "$message" 
+          break
+        }
+    }
+    gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
+             "$message: should be in upper"
+}
+
+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"
+}
+
+send_gdb "quit\n"
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..407086b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c
@@ -0,0 +1,70 @@
+/*
+* Copyright 2012 Free Software Foundation, Inc.
+*
+* Contributed by Intel Corp. <walfred.tedeschi@intel.com>
+*
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of the GNU General Public License as published by
+* the Free Software Foundation; either version 3 of the License, or
+* (at your option) any later version.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU General Public License for more details.
+*
+* You should have received a copy of the GNU General Public License
+* along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*
+*/
+
+#include <stdio.h>
+#include "x86-cpuid.h"
+
+#define MYTYPE      int
+#define OUR_SIZE    5
+
+unsigned int
+have_mpx (void)
+{
+  unsigned int eax, ebx, ecx, edx;
+
+  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
+    {
+      if (__get_cpuid_max (0, NULL) < 7)
+	return 0;
+
+      __cpuid_count (7, 0, eax, ebx, ecx, edx);
+
+      if ((ebx & bit_MPX) == bit_MPX)
+	return 1;
+      else
+	return 0;
+    }
+  return 0;
+}
+
+void
+upper (MYTYPE * p, int len)
+{
+  MYTYPE value;
+  len++;			/* b0-size-test.  */
+  value = *(p + len);
+}
+
+int
+main (void)
+{
+  if (have_mpx ())
+    {
+      int a = 0;			/* Dummy variable for debugging purposes.  */
+      MYTYPE sx[OUR_SIZE];
+      a++;				/* register-eval.  */
+      upper (sx, OUR_SIZE + 2);
+      return sx[1];
+    }
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
new file mode 100644
index 0000000..988a8b4
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
@@ -0,0 +1,130 @@
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# Contributed by Intel Corp.  <walfred.tedeschi@intel.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Testing handle setup together with boundary violation signals.
+#
+# Some states are not allowed as reported on the manual, as noprint
+# implies nostop, but nostop might print.
+#
+# Caveat: Setting the handle to nopass, ends up in a endless loop.
+
+
+if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
+    verbose "Skipping x86 MPX tests."
+    return
+}
+
+standard_testfile
+
+set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
+ [list debug nowarnings additional_flags=${comp_flags}]] } {
+    return -1
+}
+
+if ![runto_main] {
+    untested "could not run to main"
+    return -1
+}
+
+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
+    }
+    -re ".*$gdb_prompt $" {
+        fail "check whether processor supports MPX"
+    }
+    timeout {
+        fail "check whether processor supports MPX (timeout)"
+    }
+}
+
+set segv_bound_with_prompt ".*Program received signal SIGSEGV,\
+        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
+        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
+        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
+
+set segv_bound_with_exit ".*Program received signal SIGSEGV,\
+        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
+        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
+        0x\[0-9a-fA-F\]+.*$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"
+
+gdb_expect {
+     -re $segv_bound_with_prompt
+    { pass $parameters}
+    timeout { fail $parameters }
+}
+gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
+         "should be in upper; $parameters"
+
+# Using the handler for SIGSEGV as "print pass nostop"
+set parameters "print pass nostop"
+runto main
+gdb_test "handle SIGSEGV $parameters" "" "Setting the handler for segfault 0"
+
+gdb_test_multiple "continue" "test 0" {
+    -re $segv_bound_with_exit
+    { pass $parameters}
+    -re "$gdb_prompt $"
+    { fail $parameters }
+    timeout { 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"
+
+gdb_test_multiple "continue" "test 1" {
+     -re $segv_bound_with_prompt
+    { pass $parameters}
+    timeout
+    { fail $parameters }
+}
+
+gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
+         "should be in upper $parameters"
+
+# print nopass stop
+set parameters "noprint pass nostop"
+runto main
+gdb_test "handle SIGSEGV $parameters" "" "Setting the handler for segfault 2"
+
+gdb_test_multiple "continue" "test 2" {
+    -re "Continuing\..*$inferior_exited_re.*"
+    { pass $parameters}
+    timeout { fail $parameters }
+}
+
+gdb_test "where" "No stack." "no inferior $parameters"
+
+send_gdb "quit\n"
+
-- 
2.1.4

Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Joel Brobecker]

> 2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-linux-tdep.c (amd64_linux_init_abi_common):
> 	Add handler for bound violation signal.
> 	* gdbarch.sh (bound_violation_handler): New.
> 	* 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 (process_segmentation_faults): New.
> 	(print_signal_received_reason): Use process_segmentation_faults.
> 
> testsuite/gdb.arch
> 	* i386-mpx-sigsegv.c: New.
> 	* i386-mpx-sigsegv.exp: New.
> 	* i386-mpx-simple_segv.c: New.
> 	* i386-mpx-simple_segv.exp: New.

This is not a full review (haven't had the time), but one question
is nagging at me: How to do handle the case of older kernels/libc-s,
where the info is not there? Does it look like you are just reading
undefined memory?

-- 
Joel

RE: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Tedeschi, Walfred]

Hello Joel,

Thanks a lot for your support and feedback! :)

The new fields are on the bottom of the structure and yes we are reading junk memory.
On the other hand those fields have meaning when the  sig_code is 3, and meaningless otherwise.

Also see that reading the glibc version will not help a lot. Architecture and availability of fields might vary.

Possible solution is to zero the fields if value of sig_code is different than 3.

Or let the interpretation for the user, what is also an option the siginfo is already a set of unions. They have to be interpreted by the user anyhow.

Thanks and regards,
-Fred

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Joel Brobecker
Sent: Wednesday, November 04, 2015 3:55 PM
To: Tedeschi, Walfred
Cc: palves@redhat.com; gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

> 2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-linux-tdep.c (amd64_linux_init_abi_common):
> 	Add handler for bound violation signal.
> 	* gdbarch.sh (bound_violation_handler): New.
> 	* 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 (process_segmentation_faults): New.
> 	(print_signal_received_reason): Use process_segmentation_faults.
> 
> testsuite/gdb.arch
> 	* i386-mpx-sigsegv.c: New.
> 	* i386-mpx-sigsegv.exp: New.
> 	* i386-mpx-simple_segv.c: New.
> 	* i386-mpx-simple_segv.exp: New.

This is not a full review (haven't had the time), but one question is nagging at me: How to do handle the case of older kernels/libc-s, where the info is not there? Does it look like you are just reading undefined memory?

--
Joel
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

Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Joel Brobecker]

> With Intel(R) Memory Protection Extensions it was introduced the concept of
> boundary violation.  A boundary violations is presented to the inferior as
> a segmentation fault having as sigcode the value 3.  This patch adds a
> handler for a boundary violation extending the information displayed
> when a bound violation is presented to the inferior (segfault with code 3).
> In this case the debugger will also display the kind of violation upper or
> lower the violated bounds and the address accessed.
> 
> 
> Thanks a lot for your support!
> 
> Changelog:
> 
> 2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-linux-tdep.c (amd64_linux_init_abi_common):
> 	Add handler for bound violation signal.
> 	* gdbarch.sh (bound_violation_handler): New.
> 	* 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 (process_segmentation_faults): New.
> 	(print_signal_received_reason): Use process_segmentation_faults.
> 
> testsuite/gdb.arch
> 	* i386-mpx-sigsegv.c: New.
> 	* i386-mpx-sigsegv.exp: New.
> 	* i386-mpx-simple_segv.c: New.
> 	* i386-mpx-simple_segv.exp: New.

Same here, I would really like to have Pedro's perspective on this
patch. Here are my comments.

First, it would be nice to have an example of the patch as work.
What was the output looking like before, and what does it look like
with your patch applied (assuming kernel & glibc support).

Answering some of your questions...

> There some open points to be discussed though before asking permission to
> commit.
> They are:
> 1. infrun.c (process_segmentation_faults): The inferior is stopped to
> allow doing some evaluations. I have seen no side effect on doing that,
> on the other hand it does not look so natural to me.

Hmmmm... Isn't the inferior already stopped due to the signal anyway?

> 2. i386-linux-tdep.c (i386_mpx_bound_violation_handler): I was wondering if
> the right place for it wouldn't be the linux-tdep.c as it is done for the
> siginfo things.  This is a new structure in the kernel. Doing at the
> linux-tdep.c documentation should be simplified and all architectures
> providing that functionality in a different way only have to set the right
> function pointer.

Possibly. Probably. Do you know if this applies to arm-linux as well,
for instance?

> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index a13d9b9..b4d231c 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
>  
> +# Bound violation can be handled differently accross architectures.
> +# UIOUT is the output stream where the handler will place information.
> +# si_code is the value of the field with the same name in the siginfo structure.
> +F:void:bound_violation_handler:struct ui_out *uiout, long si_code:uiout, si_code

For this function's documention, it would be better to describe what
this function is supposed to do. If it helps, I would explain when
the function is called.

/* Called when a bla bla bla due to a bound violation bla bla.
   Print bla bla bla on UIOUT. SI_CODE is ...  */

But the way this is currently implemented seems to indicate it is
called for any SEGV... Is that what "bound violation" means? I don't
think so, right? As far as I can tell, it's only when SI_CODE is
SIG_CODE_BONDARY_FAULT...

So the function seems to be misnamed.

>  M:char *:make_corefile_notes:bfd *obfd, int *note_size:obfd, note_size
>  
> +

Seems like an unnecessary change.

> +/* Code for faulty boundary has to be introduced.  */
> +#define SIG_CODE_BONDARY_FAULT 3
> +
> +void
> +i386_mpx_bound_violation_handler (struct ui_out *uiout, long si_code)

Every new function must be documented via a small introductory comment.
In this case, since it implements a gdbarch "method", a trivial comment
like below is sufficient:

/* Implement theh "bound_violation_handler" gdbarch method.  */

> +  lower_bound = parse_and_eval_long (
> +      "$_siginfo._sifields._sigfault._addr_bnd._lower\n");
> +  upper_bound = parse_and_eval_long (
> +      "$_siginfo._sifields._sigfault._addr_bnd._upper\n");
> +  access = parse_and_eval_long (
> +      "$_siginfo._sifields._sigfault.si_addr\n");

The formatting needs to be adjusted. And I don't think you need
the \n at the end. Thus:

  lower_bound
    = parse_and_eval_long ("$_siginfo._sifields._sigfault._addr_bnd._lower");

But more importantly, I am not sure you really want to use
parse_and_eval_long, here. It's convenient, sure, but then
avoids questions like error handling. For instance, if an issue
happens during evaluation and causes an exception to be raised,
are you OK letting it propagate. I personally wouldn't.


> +  is_upper = (access > upper_bound ? 1 : 0);
> +
> +  if (ui_out_is_mi_like_p (uiout))
> +    {
> +      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_field_fmt (uiout,"lower-bound",
> +			"0x%lx",
> +			lower_bound);
> +      ui_out_field_fmt (uiout,"upper-bound",
> +			"0x%lx",
> +			upper_bound);
> +      ui_out_field_fmt (uiout,"bound-access",
> +			"0x%lx",
> +			access);
> +    }
> +  else
> +    {
> +      if (is_upper)
> +	ui_out_field_string (uiout, "sigcode-meaning",
> +			     "\nupper bound violation");
> +      else
> +	ui_out_field_string (uiout, "sigcode-meaning",
> +			     "\nlower bound violation");
> +
> +      ui_out_field_string (uiout, "bounds",
> +			   " - bounds");
> +      ui_out_field_fmt (uiout,"bound-values",
> +			" {lbound = 0x%lx, ubound = 0x%lx}",
> +			lower_bound,
> +			upper_bound);
> +      ui_out_field_fmt (uiout,"bound-access",
> +			" accessing 0x%lx",
> +			access);
> +    }

I think you can do this more simply. Something like this:

   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, " - bounds {lbound = ");
   ui_out_field_fmt (uiout, "lower-bound", paddress (lower_bound));
   ui_out_text (uiout, ", ubound = ");
   ui_out_field_fmt (uiout, "upper-bound", paddress (upper_bound));

etc etc etc. To be verified, though, especially in the GDB/MI output,
to make sure the ui_out_text calls do not bleed text into the GDB/MI
output.

Note a couple of important little details:

  - There are a number of formatting issues (space after comma in
    function call, alignment issues on multi-line statements, etc).
    Our coding standard is documented at:
    https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

  - do not print a CORE_ADDR using %0xlx. Use paddress instead.
    This requires a gdbarch, but you can get that by changing
    the type of your gdbarch to a method (IIRC).

If I may say so, the new output might be a little heavy.  Wouldn't there
be a way to do this more on demand?

> +/* Handles and displays information related to the MPX bound violation
> +   to the user.  */
> +void
> +i386_mpx_bound_violation_handler (struct ui_out *uiout, long si_code);

Ah - I missed that one when I said all function should be documented.
There are two school of thoughts within the GDB group; some want the
documentation to be next to the implementation (in the .c file, some
think it's more natural to be in the .h file, which people can then
browse when they are looking for something). The consensus is that,
if you are going to document in the .h file, then make sure there is
a note of it in the .c file. Something like:

/* See "i386-linux-tdep.h".  */

More specifically for this function, though, because it implements
a hook that has already been documented, it's better to refer to
the gdbarch method's documentation.  That way, if we have to modify
the gdbarch method, we only have to change the documentation once.

> +/* Verify if target is MPX enabled.  */
> +extern int i386_mpx_enabled (void);

Already documented in the .c file, so unnecesarry here.

> +static void
> +process_segmentation_faults (struct ui_out * uiout)

Requires documentation, as said before.
No space after the "*" -> "struct ui_out *uiout".

This is nitpicking, so feel free to ignore, but I think we've been
using the term "handle" rather than "process" for this kinds of
things.

> +{
> +  long si_code;
> +  struct regcache *regcache = get_current_regcache ();
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +
> +  set_running (user_visible_resume_ptid (1), 0);

This is the part that _really_ concerns me, not necessary because
I think it's wrong (although, it is a big red flag for me), but
because I don't understand why it's needed, and how it will affect
things.

> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");

Same remarks as above.

> +  if (gdbarch_bound_violation_handler_p (gdbarch))
> +    gdbarch_bound_violation_handler (gdbarch, uiout, si_code);
> +}
> +
>  void
>  print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
>  {
> @@ -7773,6 +7787,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)
> +	process_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..5f20aa2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c
> @@ -0,0 +1,128 @@
> +/*
> +* Copyright 2012 Free Software Foundation, Inc.
> +*
> +* Contributed by Intel Corp. <christian.himpel@intel.com>,
> +*                            <walfred.tedeschi@intel.com>
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the GNU General Public License as published by
> +* the Free Software Foundation; either version 3 of the License, or
> +* (at your option) any later version.
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*
> +* This test was converted from idb/test/td/runreg/Biendian/bi.c_bitfield
> +*

No leading '*' in multi-line comments (Gnu Coding Standards).
Also, I would remove the reference to idb/[...]/bi.c_bitfield unless
it is something from the GDB project itself.

One tiny but important nit - the copyright year should at least
include 2015. If the file was first create in 2012, then it should
be 2012-2015.

> +#include <stdio.h>

Do you need stdio.h, for this test?

> +#include "x86-cpuid.h"
> +
> +
> +#define MYTYPE   int

Is that really useful?

> +#define OUR_SIZE    5
> +
> +MYTYPE gx[OUR_SIZE];
> +MYTYPE ga[OUR_SIZE];
> +MYTYPE gb[OUR_SIZE];
> +MYTYPE gc[OUR_SIZE];
> +MYTYPE gd[OUR_SIZE];
> +
> +unsigned int
> +have_mpx (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +	return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ebx & bit_MPX) == bit_MPX)
> +	return 1;
> +      else
> +	return 0;
> +    }
> +  return 0;
> +}
> +
> +int
> +bp1 (MYTYPE value)
> +{
> +  return 1;
> +}
> +
> +int
> +bp2 (MYTYPE value)
> +{
> +  return 1;
> +}
> +
> +void
> +upper (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
> +{
> +  MYTYPE value;
> +  value = *(p + len);
> +  value = *(a + len);
> +  value = *(b + len);
> +  value = *(c + len);
> +  value = *(d + len);
> +}
> +
> +void
> +lower (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, int len)
> +{
> +  MYTYPE value;
> +  value = *(p - len);
> +  value = *(a - len);
> +  value = *(b - len);
> +  value = *(c - len);
> +  bp2 (value);
> +  value = *(d - len);
> +}
> +
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      MYTYPE sx[OUR_SIZE];
> +      MYTYPE sa[OUR_SIZE];
> +      MYTYPE sb[OUR_SIZE];
> +      MYTYPE sc[OUR_SIZE];
> +      MYTYPE sd[OUR_SIZE];
> +      MYTYPE *x, *a, *b, *c, *d;
> +
> +      x = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      a = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      b = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      c = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      d = calloc (OUR_SIZE, sizeof (MYTYPE));
> +
> +      upper (x, a, b, c, d, OUR_SIZE + 2);
> +      upper (sx, sa, sb, sc, sd, OUR_SIZE + 2);
> +      upper (gx, ga, gb, gc, gd, OUR_SIZE + 2);
> +      lower (x, a, b, c, d, 1);
> +      lower (sx, sa, sb, sc, sd, 1);
> +      bp1 (*x);
> +      lower (gx, ga, gb, gc, gd, 1);
> +
> +      free (x);
> +      free (a);
> +      free (b);
> +      free (c);
> +      free (d);
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
> new file mode 100644
> index 0000000..f689018
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
> @@ -0,0 +1,95 @@
> +# Copyright 2012 Free Software Foundation, Inc.

Same comment about the copyright years.

> +# Contributed by Intel Corp. <walfred.tedeschi@intel.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> + [list debug nowarnings additional_flags=${comp_flags}]] } {
> +    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 " {
> +        verbose "processor does not support MPX; skipping MPX tests"
> +        return
> +    }
> +    -re ".*$gdb_prompt $" {
> +        fail "check whether processor supports MPX"
> +    }
> +    timeout {
> +        fail "check whether processor supports MPX (timeout)"
> +    }

You don't need the last two branches (already taken care of
by gdb_test_multiple).

> +set segv_lower_bound ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nlower bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
> +
> +set segv_upper_bound ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
> +
> +for {set i 0} {$i < 15} {incr i} {
> +    set message "MPX signal segv Upper: ${i}"
> +    send_gdb "continue\n"
> +    gdb_expect {
> +         -re $segv_upper_bound
> +        { pass "$message" }
> +        -re ".*$inferior_exited_re normally.*$gdb_prompt $"
> +        {
> +          fail "$message" 
> +          break
> +        }

I think you can use gdb_test_multiple, here, no? We avoid using
gdb_expect like the pest. For instance, you're missing a lot of
the stuff that gdb_test_multiple does for you.

The formatting and indentation are not what we usually do:

  gdb_test_multiple [...] {
     -re "something" {
         pass message
         }
     -re "something else" {
         fail message
         }
  }

> +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"
> +}
> +
> +send_gdb "quit\n"

Unecessary.

> 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..407086b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c
> @@ -0,0 +1,70 @@
> +/*
> +* Copyright 2012 Free Software Foundation, Inc.
> +*
> +* Contributed by Intel Corp. <walfred.tedeschi@intel.com>
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the GNU General Public License as published by
> +* the Free Software Foundation; either version 3 of the License, or
> +* (at your option) any later version.
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*
> +*/
> +
> +#include <stdio.h>
> +#include "x86-cpuid.h"
> +
> +#define MYTYPE      int
> +#define OUR_SIZE    5
> +
> +unsigned int
> +have_mpx (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +	return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ebx & bit_MPX) == bit_MPX)
> +	return 1;
> +      else
> +	return 0;
> +    }
> +  return 0;
> +}
> +
> +void
> +upper (MYTYPE * p, int len)
> +{
> +  MYTYPE value;
> +  len++;			/* b0-size-test.  */
> +  value = *(p + len);
> +}
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      int a = 0;			/* Dummy variable for debugging purposes.  */

Can you indent the comment a little less so as to avoid exceeding 80
chars?

> +      MYTYPE sx[OUR_SIZE];
> +      a++;				/* register-eval.  */
> +      upper (sx, OUR_SIZE + 2);
> +      return sx[1];
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
> new file mode 100644
> index 0000000..988a8b4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
> @@ -0,0 +1,130 @@
> +# Copyright 2013 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp.  <walfred.tedeschi@intel.com>
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Testing handle setup together with boundary violation signals.
> +#
> +# Some states are not allowed as reported on the manual, as noprint
> +# implies nostop, but nostop might print.
> +#
> +# Caveat: Setting the handle to nopass, ends up in a endless loop.
> +
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \
> + [list debug nowarnings additional_flags=${comp_flags}]] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +send_gdb "print have_mpx ()\r"
> +gdb_expect {

Same comment as above about 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
> +    }
> +    -re ".*$gdb_prompt $" {
> +        fail "check whether processor supports MPX"
> +    }
> +    timeout {
> +        fail "check whether processor supports MPX (timeout)"
> +    }
> +}
> +
> +set segv_bound_with_prompt ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
> +
> +set segv_bound_with_exit ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$inferior_exited_re.*"
> +
> +# Using the handler for SIGSEGV as "print pass stop"
> +set parameters "print pass stop"
> +runto main

Can you use runto_main instead?

[I'm running out of time fo now, but I think the rest is going to be
more of the same comments]

Bye!

> +send_gdb "handle SIGSEGV $parameters\n"
> +send_gdb "continue\n"
> +
> +gdb_expect {
> +     -re $segv_bound_with_prompt
> +    { pass $parameters}
> +    timeout { fail $parameters }
> +}
> +gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
> +         "should be in upper; $parameters"
> +
> +# Using the handler for SIGSEGV as "print pass nostop"
> +set parameters "print pass nostop"
> +runto main
> +gdb_test "handle SIGSEGV $parameters" "" "Setting the handler for segfault 0"
> +
> +gdb_test_multiple "continue" "test 0" {
> +    -re $segv_bound_with_exit
> +    { pass $parameters}
> +    -re "$gdb_prompt $"
> +    { fail $parameters }
> +    timeout { 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"
> +
> +gdb_test_multiple "continue" "test 1" {
> +     -re $segv_bound_with_prompt
> +    { pass $parameters}
> +    timeout
> +    { fail $parameters }
> +}
> +
> +gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
> +         "should be in upper $parameters"
> +
> +# print nopass stop
> +set parameters "noprint pass nostop"
> +runto main
> +gdb_test "handle SIGSEGV $parameters" "" "Setting the handler for segfault 2"
> +
> +gdb_test_multiple "continue" "test 2" {
> +    -re "Continuing\..*$inferior_exited_re.*"
> +    { pass $parameters}
> +    timeout { fail $parameters }
> +}
> +
> +gdb_test "where" "No stack." "no inferior $parameters"
> +
> +send_gdb "quit\n"
> +
> -- 
> 2.1.4

-- 
Joel

RE: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Tedeschi, Walfred]

Joel and Pedro,

Thanks a lot for your feedback!

I Could address most of the comments in here. 
An important one is still missing, namely this one:

> +{
> +  long si_code;
> +  struct regcache *regcache = get_current_regcache ();
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +
> +  set_running (user_visible_resume_ptid (1), 0);

This is the part that _really_ concerns me, not necessary because I think it's wrong (although, it is a big red flag for me), but because I don't understand why it's needed, and how it will affect things.
(From Joel)
> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");

During the debugging time I understood that inferior was stopped. Gdb is that was in the process to determine in which state the inferior was.
In this sense I set the flag at this point to allow for the evaluation.

I also looked in gdb for error handling while performing evaluations but did not find anything.
I am considering that the way to proceed is to use TRY and CATCH blocks. Would you recommend that?

Thanks and regards,
-Fred


-----Original Message-----
From: Joel Brobecker [mailto:brobecker@adacore.com] 
Sent: Thursday, November 19, 2015 1:02 AM
To: Tedeschi, Walfred
Cc: palves@redhat.com; gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

> With Intel(R) Memory Protection Extensions it was introduced the 
> concept of boundary violation.  A boundary violations is presented to 
> the inferior as a segmentation fault having as sigcode the value 3.  
> This patch adds a handler for a boundary violation extending the 
> information displayed when a bound violation is presented to the inferior (segfault with code 3).
> In this case the debugger will also display the kind of violation 
> upper or lower the violated bounds and the address accessed.
> 
> 
> Thanks a lot for your support!
> 
> Changelog:
> 
> 2015-07-21  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-linux-tdep.c (amd64_linux_init_abi_common):
> 	Add handler for bound violation signal.
> 	* gdbarch.sh (bound_violation_handler): New.
> 	* 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 (process_segmentation_faults): New.
> 	(print_signal_received_reason): Use process_segmentation_faults.
> 
> testsuite/gdb.arch
> 	* i386-mpx-sigsegv.c: New.
> 	* i386-mpx-sigsegv.exp: New.
> 	* i386-mpx-simple_segv.c: New.
> 	* i386-mpx-simple_segv.exp: New.

Same here, I would really like to have Pedro's perspective on this patch. Here are my comments.

First, it would be nice to have an example of the patch as work.
What was the output looking like before, and what does it look like with your patch applied (assuming kernel & glibc support).

Answering some of your questions...

> There some open points to be discussed though before asking permission 
> to commit.
> They are:
> 1. infrun.c (process_segmentation_faults): The inferior is stopped to 
> allow doing some evaluations. I have seen no side effect on doing 
> that, on the other hand it does not look so natural to me.

Hmmmm... Isn't the inferior already stopped due to the signal anyway?

> 2. i386-linux-tdep.c (i386_mpx_bound_violation_handler): I was 
> wondering if the right place for it wouldn't be the linux-tdep.c as it 
> is done for the siginfo things.  This is a new structure in the 
> kernel. Doing at the linux-tdep.c documentation should be simplified 
> and all architectures providing that functionality in a different way 
> only have to set the right function pointer.

Possibly. Probably. Do you know if this applies to arm-linux as well, for instance?

> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh index a13d9b9..b4d231c 
> 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
>  
> +# Bound violation can be handled differently accross architectures.
> +# UIOUT is the output stream where the handler will place information.
> +# si_code is the value of the field with the same name in the siginfo structure.
> +F:void:bound_violation_handler:struct ui_out *uiout, long 
> +si_code:uiout, si_code

For this function's documention, it would be better to describe what this function is supposed to do. If it helps, I would explain when the function is called.

/* Called when a bla bla bla due to a bound violation bla bla.
   Print bla bla bla on UIOUT. SI_CODE is ...  */

But the way this is currently implemented seems to indicate it is called for any SEGV... Is that what "bound violation" means? I don't think so, right? As far as I can tell, it's only when SI_CODE is SIG_CODE_BONDARY_FAULT...

So the function seems to be misnamed.

>  M:char *:make_corefile_notes:bfd *obfd, int *note_size:obfd, 
> note_size
>  
> +

Seems like an unnecessary change.

> +/* Code for faulty boundary has to be introduced.  */ #define 
> +SIG_CODE_BONDARY_FAULT 3
> +
> +void
> +i386_mpx_bound_violation_handler (struct ui_out *uiout, long si_code)

Every new function must be documented via a small introductory comment.
In this case, since it implements a gdbarch "method", a trivial comment like below is sufficient:

/* Implement theh "bound_violation_handler" gdbarch method.  */

> +  lower_bound = parse_and_eval_long (
> +      "$_siginfo._sifields._sigfault._addr_bnd._lower\n");
> +  upper_bound = parse_and_eval_long (
> +      "$_siginfo._sifields._sigfault._addr_bnd._upper\n");
> +  access = parse_and_eval_long (
> +      "$_siginfo._sifields._sigfault.si_addr\n");

The formatting needs to be adjusted. And I don't think you need the \n at the end. Thus:

  lower_bound
    = parse_and_eval_long ("$_siginfo._sifields._sigfault._addr_bnd._lower");

But more importantly, I am not sure you really want to use parse_and_eval_long, here. It's convenient, sure, but then avoids questions like error handling. For instance, if an issue happens during evaluation and causes an exception to be raised, are you OK letting it propagate. I personally wouldn't.


> +  is_upper = (access > upper_bound ? 1 : 0);
> +
> +  if (ui_out_is_mi_like_p (uiout))
> +    {
> +      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_field_fmt (uiout,"lower-bound",
> +			"0x%lx",
> +			lower_bound);
> +      ui_out_field_fmt (uiout,"upper-bound",
> +			"0x%lx",
> +			upper_bound);
> +      ui_out_field_fmt (uiout,"bound-access",
> +			"0x%lx",
> +			access);
> +    }
> +  else
> +    {
> +      if (is_upper)
> +	ui_out_field_string (uiout, "sigcode-meaning",
> +			     "\nupper bound violation");
> +      else
> +	ui_out_field_string (uiout, "sigcode-meaning",
> +			     "\nlower bound violation");
> +
> +      ui_out_field_string (uiout, "bounds",
> +			   " - bounds");
> +      ui_out_field_fmt (uiout,"bound-values",
> +			" {lbound = 0x%lx, ubound = 0x%lx}",
> +			lower_bound,
> +			upper_bound);
> +      ui_out_field_fmt (uiout,"bound-access",
> +			" accessing 0x%lx",
> +			access);
> +    }

I think you can do this more simply. Something like this:

   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, " - bounds {lbound = ");
   ui_out_field_fmt (uiout, "lower-bound", paddress (lower_bound));
   ui_out_text (uiout, ", ubound = ");
   ui_out_field_fmt (uiout, "upper-bound", paddress (upper_bound));

etc etc etc. To be verified, though, especially in the GDB/MI output, to make sure the ui_out_text calls do not bleed text into the GDB/MI output.

Note a couple of important little details:

  - There are a number of formatting issues (space after comma in
    function call, alignment issues on multi-line statements, etc).
    Our coding standard is documented at:
    https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

  - do not print a CORE_ADDR using %0xlx. Use paddress instead.
    This requires a gdbarch, but you can get that by changing
    the type of your gdbarch to a method (IIRC).

If I may say so, the new output might be a little heavy.  Wouldn't there be a way to do this more on demand?

> +/* Handles and displays information related to the MPX bound violation
> +   to the user.  */
> +void
> +i386_mpx_bound_violation_handler (struct ui_out *uiout, long 
> +si_code);

Ah - I missed that one when I said all function should be documented.
There are two school of thoughts within the GDB group; some want the documentation to be next to the implementation (in the .c file, some think it's more natural to be in the .h file, which people can then browse when they are looking for something). The consensus is that, if you are going to document in the .h file, then make sure there is a note of it in the .c file. Something like:

/* See "i386-linux-tdep.h".  */

More specifically for this function, though, because it implements a hook that has already been documented, it's better to refer to the gdbarch method's documentation.  That way, if we have to modify the gdbarch method, we only have to change the documentation once.

> +/* Verify if target is MPX enabled.  */ extern int i386_mpx_enabled 
> +(void);

Already documented in the .c file, so unnecesarry here.

> +static void
> +process_segmentation_faults (struct ui_out * uiout)

Requires documentation, as said before.
No space after the "*" -> "struct ui_out *uiout".

This is nitpicking, so feel free to ignore, but I think we've been using the term "handle" rather than "process" for this kinds of things.

> +{
> +  long si_code;
> +  struct regcache *regcache = get_current_regcache ();
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +
> +  set_running (user_visible_resume_ptid (1), 0);

This is the part that _really_ concerns me, not necessary because I think it's wrong (although, it is a big red flag for me), but because I don't understand why it's needed, and how it will affect things.

> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");

Same remarks as above.

> +  if (gdbarch_bound_violation_handler_p (gdbarch))
> +    gdbarch_bound_violation_handler (gdbarch, uiout, si_code); }
> +
>  void
>  print_signal_received_reason (struct ui_out *uiout, enum gdb_signal 
> siggnal)  { @@ -7773,6 +7787,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)
> +	process_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..5f20aa2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.c
> @@ -0,0 +1,128 @@
> +/*
> +* Copyright 2012 Free Software Foundation, Inc.
> +*
> +* Contributed by Intel Corp. <christian.himpel@intel.com>,
> +*                            <walfred.tedeschi@intel.com>
> +*
> +* This program is free software; you can redistribute it and/or 
> +modify
> +* it under the terms of the GNU General Public License as published 
> +by
> +* the Free Software Foundation; either version 3 of the License, or
> +* (at your option) any later version.
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*
> +* This test was converted from 
> +idb/test/td/runreg/Biendian/bi.c_bitfield
> +*

No leading '*' in multi-line comments (Gnu Coding Standards).
Also, I would remove the reference to idb/[...]/bi.c_bitfield unless it is something from the GDB project itself.

One tiny but important nit - the copyright year should at least include 2015. If the file was first create in 2012, then it should be 2012-2015.

> +#include <stdio.h>

Do you need stdio.h, for this test?

> +#include "x86-cpuid.h"
> +
> +
> +#define MYTYPE   int

Is that really useful?

> +#define OUR_SIZE    5
> +
> +MYTYPE gx[OUR_SIZE];
> +MYTYPE ga[OUR_SIZE];
> +MYTYPE gb[OUR_SIZE];
> +MYTYPE gc[OUR_SIZE];
> +MYTYPE gd[OUR_SIZE];
> +
> +unsigned int
> +have_mpx (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +	return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ebx & bit_MPX) == bit_MPX)
> +	return 1;
> +      else
> +	return 0;
> +    }
> +  return 0;
> +}
> +
> +int
> +bp1 (MYTYPE value)
> +{
> +  return 1;
> +}
> +
> +int
> +bp2 (MYTYPE value)
> +{
> +  return 1;
> +}
> +
> +void
> +upper (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, 
> +int len) {
> +  MYTYPE value;
> +  value = *(p + len);
> +  value = *(a + len);
> +  value = *(b + len);
> +  value = *(c + len);
> +  value = *(d + len);
> +}
> +
> +void
> +lower (MYTYPE * p, MYTYPE * a, MYTYPE * b, MYTYPE * c, MYTYPE * d, 
> +int len) {
> +  MYTYPE value;
> +  value = *(p - len);
> +  value = *(a - len);
> +  value = *(b - len);
> +  value = *(c - len);
> +  bp2 (value);
> +  value = *(d - len);
> +}
> +
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      MYTYPE sx[OUR_SIZE];
> +      MYTYPE sa[OUR_SIZE];
> +      MYTYPE sb[OUR_SIZE];
> +      MYTYPE sc[OUR_SIZE];
> +      MYTYPE sd[OUR_SIZE];
> +      MYTYPE *x, *a, *b, *c, *d;
> +
> +      x = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      a = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      b = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      c = calloc (OUR_SIZE, sizeof (MYTYPE));
> +      d = calloc (OUR_SIZE, sizeof (MYTYPE));
> +
> +      upper (x, a, b, c, d, OUR_SIZE + 2);
> +      upper (sx, sa, sb, sc, sd, OUR_SIZE + 2);
> +      upper (gx, ga, gb, gc, gd, OUR_SIZE + 2);
> +      lower (x, a, b, c, d, 1);
> +      lower (sx, sa, sb, sc, sd, 1);
> +      bp1 (*x);
> +      lower (gx, ga, gb, gc, gd, 1);
> +
> +      free (x);
> +      free (a);
> +      free (b);
> +      free (c);
> +      free (d);
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp 
> b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
> new file mode 100644
> index 0000000..f689018
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-sigsegv.exp
> @@ -0,0 +1,95 @@
> +# Copyright 2012 Free Software Foundation, Inc.

Same comment about the copyright years.

> +# Contributed by Intel Corp. <walfred.tedeschi@intel.com> # # This 
> +program is free software; you can redistribute it and/or modify # it 
> +under the terms of the GNU General Public License as published by # 
> +the Free Software Foundation; either version 3 of the License, or # 
> +(at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful, # 
> +but WITHOUT ANY WARRANTY; without even the implied warranty of # 
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the # GNU 
> +General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License # 
> +along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \  
> +[list debug nowarnings additional_flags=${comp_flags}]] } {
> +    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 " {
> +        verbose "processor does not support MPX; skipping MPX tests"
> +        return
> +    }
> +    -re ".*$gdb_prompt $" {
> +        fail "check whether processor supports MPX"
> +    }
> +    timeout {
> +        fail "check whether processor supports MPX (timeout)"
> +    }

You don't need the last two branches (already taken care of by gdb_test_multiple).

> +set segv_lower_bound ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nlower bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
> +
> +set segv_upper_bound ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
> +
> +for {set i 0} {$i < 15} {incr i} {
> +    set message "MPX signal segv Upper: ${i}"
> +    send_gdb "continue\n"
> +    gdb_expect {
> +         -re $segv_upper_bound
> +        { pass "$message" }
> +        -re ".*$inferior_exited_re normally.*$gdb_prompt $"
> +        {
> +          fail "$message" 
> +          break
> +        }

I think you can use gdb_test_multiple, here, no? We avoid using gdb_expect like the pest. For instance, you're missing a lot of the stuff that gdb_test_multiple does for you.

The formatting and indentation are not what we usually do:

  gdb_test_multiple [...] {
     -re "something" {
         pass message
         }
     -re "something else" {
         fail message
         }
  }

> +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"
> +}
> +
> +send_gdb "quit\n"

Unecessary.

> 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..407086b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.c
> @@ -0,0 +1,70 @@
> +/*
> +* Copyright 2012 Free Software Foundation, Inc.
> +*
> +* Contributed by Intel Corp. <walfred.tedeschi@intel.com>
> +*
> +* This program is free software; you can redistribute it and/or 
> +modify
> +* it under the terms of the GNU General Public License as published 
> +by
> +* the Free Software Foundation; either version 3 of the License, or
> +* (at your option) any later version.
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*
> +*/
> +
> +#include <stdio.h>
> +#include "x86-cpuid.h"
> +
> +#define MYTYPE      int
> +#define OUR_SIZE    5
> +
> +unsigned int
> +have_mpx (void)
> +{
> +  unsigned int eax, ebx, ecx, edx;
> +
> +  if (!__get_cpuid (1, &eax, &ebx, &ecx, &edx))
> +    return 0;
> +
> +  if ((ecx & bit_OSXSAVE) == bit_OSXSAVE)
> +    {
> +      if (__get_cpuid_max (0, NULL) < 7)
> +	return 0;
> +
> +      __cpuid_count (7, 0, eax, ebx, ecx, edx);
> +
> +      if ((ebx & bit_MPX) == bit_MPX)
> +	return 1;
> +      else
> +	return 0;
> +    }
> +  return 0;
> +}
> +
> +void
> +upper (MYTYPE * p, int len)
> +{
> +  MYTYPE value;
> +  len++;			/* b0-size-test.  */
> +  value = *(p + len);
> +}
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      int a = 0;			/* Dummy variable for debugging purposes.  */

Can you indent the comment a little less so as to avoid exceeding 80 chars?

> +      MYTYPE sx[OUR_SIZE];
> +      a++;				/* register-eval.  */
> +      upper (sx, OUR_SIZE + 2);
> +      return sx[1];
> +    }
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp 
> b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
> new file mode 100644
> index 0000000..988a8b4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/i386-mpx-simple_segv.exp
> @@ -0,0 +1,130 @@
> +# Copyright 2013 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp.  <walfred.tedeschi@intel.com> # # This 
> +program is free software; you can redistribute it and/or modify # it 
> +under the terms of the GNU General Public License as published by # 
> +the Free Software Foundation; either version 3 of the License, or # 
> +(at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful, # 
> +but WITHOUT ANY WARRANTY; without even the implied warranty of # 
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the # GNU 
> +General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License # 
> +along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Testing handle setup together with boundary violation signals.
> +#
> +# Some states are not allowed as reported on the manual, as noprint # 
> +implies nostop, but nostop might print.
> +#
> +# Caveat: Setting the handle to nopass, ends up in a endless loop.
> +
> +
> +if { ![istarget i?86-*-*] && ![istarget x86_64-*-* ] } {
> +    verbose "Skipping x86 MPX tests."
> +    return
> +}
> +
> +standard_testfile
> +
> +set comp_flags "-mmpx -fcheck-pointer-bounds -I${srcdir}/../nat/"
> +
> +if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} \  
> +[list debug nowarnings additional_flags=${comp_flags}]] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +send_gdb "print have_mpx ()\r"
> +gdb_expect {

Same comment as above about 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
> +    }
> +    -re ".*$gdb_prompt $" {
> +        fail "check whether processor supports MPX"
> +    }
> +    timeout {
> +        fail "check whether processor supports MPX (timeout)"
> +    }
> +}
> +
> +set segv_bound_with_prompt ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$gdb_prompt $"
> +
> +set segv_bound_with_exit ".*Program received signal SIGSEGV,\
> +        Segmentation fault\r\nupper bound violation - bounds \\\{lbound\
> +        = 0x\[0-9a-fA-F\]+, ubound = 0x\[0-9a-fA-F\]+\\\} accessing\
> +        0x\[0-9a-fA-F\]+.*$inferior_exited_re.*"
> +
> +# Using the handler for SIGSEGV as "print pass stop"
> +set parameters "print pass stop"
> +runto main

Can you use runto_main instead?

[I'm running out of time fo now, but I think the rest is going to be more of the same comments]

Bye!

> +send_gdb "handle SIGSEGV $parameters\n"
> +send_gdb "continue\n"
> +
> +gdb_expect {
> +     -re $segv_bound_with_prompt
> +    { pass $parameters}
> +    timeout { fail $parameters }
> +}
> +gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
> +         "should be in upper; $parameters"
> +
> +# Using the handler for SIGSEGV as "print pass nostop"
> +set parameters "print pass nostop"
> +runto main
> +gdb_test "handle SIGSEGV $parameters" "" "Setting the handler for segfault 0"
> +
> +gdb_test_multiple "continue" "test 0" {
> +    -re $segv_bound_with_exit
> +    { pass $parameters}
> +    -re "$gdb_prompt $"
> +    { fail $parameters }
> +    timeout { 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"
> +
> +gdb_test_multiple "continue" "test 1" {
> +     -re $segv_bound_with_prompt
> +    { pass $parameters}
> +    timeout
> +    { fail $parameters }
> +}
> +
> +gdb_test "where" ".*#0  0x\[0-9a-fA-F\]+ in upper.*"\
> +         "should be in upper $parameters"
> +
> +# print nopass stop
> +set parameters "noprint pass nostop"
> +runto main
> +gdb_test "handle SIGSEGV $parameters" "" "Setting the handler for segfault 2"
> +
> +gdb_test_multiple "continue" "test 2" {
> +    -re "Continuing\..*$inferior_exited_re.*"
> +    { pass $parameters}
> +    timeout { fail $parameters }
> +}
> +
> +gdb_test "where" "No stack." "no inferior $parameters"
> +
> +send_gdb "quit\n"
> +
> --
> 2.1.4

--
Joel
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

Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Pedro Alves]

On 12/14/2015 05:43 PM, Tedeschi, Walfred wrote:
> Joel and Pedro,
> 
> Thanks a lot for your feedback!
> 
> I Could address most of the comments in here. 
> An important one is still missing, namely this one:
> 
>> +{
>> +  long si_code;
>> +  struct regcache *regcache = get_current_regcache ();
>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +
>> +  set_running (user_visible_resume_ptid (1), 0);
> 
> This is the part that _really_ concerns me, not necessary because I think it's wrong (although, it is a big red flag for me), but because I don't understand why it's needed, and how it will affect things.
> (From Joel)
>> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");
> 
> During the debugging time I understood that inferior was stopped. Gdb is that was in the process to determine in which state the inferior was.
> In this sense I set the flag at this point to allow for the evaluation.

Where is the error thrown that required brute-forcing set_running away?
Can we try to find some other way to handle this?  E.g., use something a bit lower
level than parse_and_eval_long that bypasses the error?  E.g., start from
lookup_internalvar and then use type/value manipulation routines?

Thanks,
Pedro Alves

> 
> I also looked in gdb for error handling while performing evaluations but did not find anything.
> I am considering that the way to proceed is to use TRY and CATCH blocks. Would you recommend that?
> 
> Thanks and regards,

RE: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Tedeschi, Walfred]

Pedro,

It comes from the infrun.c (validate_siginfo_access) .
The requirement is not running is not fulfilled. Also in the case that we execute a lookup_interval and ask for value_contents we trigger the same code.

What would be the suggestion here:
Additional function to be used internally in infrun or add a flag.

Thanks a lot for your comments and insights!
Best regards,
-Fred




-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, December 14, 2015 7:45 PM
To: Tedeschi, Walfred; Joel Brobecker
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

On 12/14/2015 05:43 PM, Tedeschi, Walfred wrote:
> Joel and Pedro,
> 
> Thanks a lot for your feedback!
> 
> I Could address most of the comments in here. 
> An important one is still missing, namely this one:
> 
>> +{
>> +  long si_code;
>> +  struct regcache *regcache = get_current_regcache ();
>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +
>> +  set_running (user_visible_resume_ptid (1), 0);
> 
> This is the part that _really_ concerns me, not necessary because I think it's wrong (although, it is a big red flag for me), but because I don't understand why it's needed, and how it will affect things.
> (From Joel)
>> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");
> 
> During the debugging time I understood that inferior was stopped. Gdb is that was in the process to determine in which state the inferior was.
> In this sense I set the flag at this point to allow for the evaluation.

Where is the error thrown that required brute-forcing set_running away?
Can we try to find some other way to handle this?  E.g., use something a bit lower level than parse_and_eval_long that bypasses the error?  E.g., start from lookup_internalvar and then use type/value manipulation routines?

Thanks,
Pedro Alves

> 
> I also looked in gdb for error handling while performing evaluations but did not find anything.
> I am considering that the way to proceed is to use TRY and CATCH blocks. Would you recommend that?
> 
> Thanks and regards,
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

RE: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Tedeschi, Walfred]

Pedro,

We have found an interesting fact, changing the order the observer_notify_signal_recieved from about line 8170 to just before
Observer_notify_normal_stop.  Allows the evaluation of the siginfo without the stop.

Looking at the code I could not see anything could harm there.

What do you think? Is moving that code a possibility?

Thanks and regards,
-Fred 

-----Original Message-----
From: Tedeschi, Walfred 
Sent: Tuesday, December 15, 2015 11:59 AM
To: Pedro Alves; Joel Brobecker
Cc: gdb-patches@sourceware.org
Subject: RE: [PATCH v1] Intel(R) MPX - Bound violation handling.

Pedro,

It comes from the infrun.c (validate_siginfo_access) .
The requirement is not running is not fulfilled. Also in the case that we execute a lookup_interval and ask for value_contents we trigger the same code.

What would be the suggestion here:
Additional function to be used internally in infrun or add a flag.

Thanks a lot for your comments and insights!
Best regards,
-Fred




-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Monday, December 14, 2015 7:45 PM
To: Tedeschi, Walfred; Joel Brobecker
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

On 12/14/2015 05:43 PM, Tedeschi, Walfred wrote:
> Joel and Pedro,
> 
> Thanks a lot for your feedback!
> 
> I Could address most of the comments in here. 
> An important one is still missing, namely this one:
> 
>> +{
>> +  long si_code;
>> +  struct regcache *regcache = get_current_regcache ();
>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> +
>> +  set_running (user_visible_resume_ptid (1), 0);
> 
> This is the part that _really_ concerns me, not necessary because I think it's wrong (although, it is a big red flag for me), but because I don't understand why it's needed, and how it will affect things.
> (From Joel)
>> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");
> 
> During the debugging time I understood that inferior was stopped. Gdb is that was in the process to determine in which state the inferior was.
> In this sense I set the flag at this point to allow for the evaluation.

Where is the error thrown that required brute-forcing set_running away?
Can we try to find some other way to handle this?  E.g., use something a bit lower level than parse_and_eval_long that bypasses the error?  E.g., start from lookup_internalvar and then use type/value manipulation routines?

Thanks,
Pedro Alves

> 
> I also looked in gdb for error handling while performing evaluations but did not find anything.
> I am considering that the way to proceed is to use TRY and CATCH blocks. Would you recommend that?
> 
> Thanks and regards,
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

Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Pedro Alves]

On 12/16/2015 03:19 PM, Tedeschi, Walfred wrote:
> Pedro,
> 
> We have found an interesting fact, changing the order the observer_notify_signal_recieved from about line 8170 to just before
> Observer_notify_normal_stop.  Allows the evaluation of the siginfo without the stop.

That's because we're about to present a stop to the user, so we
mark the threads as stopped in between:

  /* Let the user/frontend see the threads as stopped.  */
  do_cleanups (old_chain);

But there's another observer_notify_signal_received
call that is done while threads are still marked running.  Here
when we print the signal, but don't stop:

      /* Notify observers the signal has "handle print" set.  Note we
	 returned early above if stopping; normal_stop handles the
	 printing in that case.  */
      if (signal_print[ecs->event_thread->suspend.stop_signal])
	{
	  /* The signal table tells us to print about this signal.  */
	  target_terminal_ours_for_output ();
	  observer_notify_signal_received (ecs->event_thread->suspend.stop_signal);
	  target_terminal_inferior ();
	}

Should gdb print the extra info in that case?


In any case, for the normal_stop case, I think you'd want to move
the observers call to just after the do_cleanups call shown above.
At least, put it before the stop hook handling.  If something sets the
target running in the stop hook, then you'd lose stopped_by_random_signal.

> Looking at the code I could not see anything could harm there.

This may change output order, both CLI and MI.  Please actually
try using the resulting gdb to intercept a signal, and compare it
with an unpatched gdb.  Also, running the testsuite wouldn't be
a bad idea...

> 
> What do you think? Is moving that code a possibility?

Thanks,
Pedro Alves

RE: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Tedeschi, Walfred]

Pedro,

We have lost the print when not stopping, by doing that.
Idea was also to print in that case.
Any idea how we could also handle the other case?

For the stopping case all seems fine. Will run tests again to be sure.

Thanks and regards,
-Fred


-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Pedro Alves
Sent: Wednesday, December 16, 2015 5:53 PM
To: Tedeschi, Walfred; 'Joel Brobecker'
Cc: 'gdb-patches@sourceware.org'
Subject: Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

On 12/16/2015 03:19 PM, Tedeschi, Walfred wrote:
> Pedro,
> 
> We have found an interesting fact, changing the order the 
> observer_notify_signal_recieved from about line 8170 to just before Observer_notify_normal_stop.  Allows the evaluation of the siginfo without the stop.

That's because we're about to present a stop to the user, so we mark the threads as stopped in between:

  /* Let the user/frontend see the threads as stopped.  */
  do_cleanups (old_chain);

But there's another observer_notify_signal_received call that is done while threads are still marked running.  Here when we print the signal, but don't stop:

      /* Notify observers the signal has "handle print" set.  Note we
	 returned early above if stopping; normal_stop handles the
	 printing in that case.  */
      if (signal_print[ecs->event_thread->suspend.stop_signal])
	{
	  /* The signal table tells us to print about this signal.  */
	  target_terminal_ours_for_output ();
	  observer_notify_signal_received (ecs->event_thread->suspend.stop_signal);
	  target_terminal_inferior ();
	}

Should gdb print the extra info in that case?


In any case, for the normal_stop case, I think you'd want to move the observers call to just after the do_cleanups call shown above.
At least, put it before the stop hook handling.  If something sets the target running in the stop hook, then you'd lose stopped_by_random_signal.

> Looking at the code I could not see anything could harm there.

This may change output order, both CLI and MI.  Please actually try using the resulting gdb to intercept a signal, and compare it with an unpatched gdb.  Also, running the testsuite wouldn't be a bad idea...

> 
> What do you think? Is moving that code a possibility?

Thanks,
Pedro Alves

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

Re: [PATCH v1] Intel(R) MPX - Bound violation handling.

[Pedro Alves]

Hi Walfred,

(Please don't top post.)

On 12/15/2015 10:58 AM, Tedeschi, Walfred wrote:
> From: Pedro Alves [mailto:palves@redhat.com] 
> On 12/14/2015 05:43 PM, Tedeschi, Walfred wrote:

>>> >> +  set_running (user_visible_resume_ptid (1), 0);
>> > 
>> > This is the part that _really_ concerns me, not necessary because I think it's wrong (although, it is a big red flag for me), but because I don't understand why it's needed, and how it will affect things.
>> > (From Joel)
>>> >> +  si_code = parse_and_eval_long ("$_siginfo.si_code\n");
>> > 
>> > During the debugging time I understood that inferior was stopped. Gdb is that was in the process to determine in which state the inferior was.
>> > In this sense I set the flag at this point to allow for the evaluation.
> Where is the error thrown that required brute-forcing set_running away?
> Can we try to find some other way to handle this?  E.g., use something a bit lower level than parse_and_eval_long that bypasses the error?  E.g., start from lookup_internalvar and then use type/value manipulation routines?
>
> It comes from the infrun.c (validate_siginfo_access) .
> The requirement is not running is not fulfilled. Also in the case that we execute a lookup_interval and ask for value_contents we trigger the same code.
>
> What would be the suggestion here:
> Additional function to be used internally in infrun or add a flag.

I gave this some thought, and ended up filling 2 PRs and a proposal forward:

  [Bug breakpoints/19388] Access $_siginfo in breakpoint (catch signal) condition
  https://sourceware.org/bugzilla/show_bug.cgi?id=19388

  [Bug gdb/19389] GDB sometimes mistakenly allows accessing registers of running threads
  https://sourceware.org/bugzilla/show_bug.cgi?id=19389

I think that to move forward we should change validate_siginfo_access to
check is_executing instead of is_running for now.  I think it'll
fix your case (please give it a try).  It's the same check that we do to
prevent accessing registers from a running thread.  $_siginfo is
conceptually really no different from registers -- we could think of it as
just another register.

I sent a patch that does that here, along with testcase that justifies
it on its own, independently of MPX:

  [PATCH] Fix PR19388: Can't access $_siginfo in breakpoint (catch signal) condition
  https://sourceware.org/ml/gdb-patches/2015-12/msg00443.html

By making this change, $_siginfo will become exposed to PR19389 too,
just like registers, and we should definitely fix it, but that seems
like a lesser evil than not being able to get at the info at all.

Thanks,
Pedro Alves

Re: [PATCH obv] Changing compiler flags for MPX tests.

[Joel Brobecker]

> Adapts tests to use actual GCC flags, previous used flags were
> related to an internal GCC release.
> 
> 2015-06-18  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> gdb/testsuite:
> 
> 	* gdb.arch/i386-mpx-map.exp (comp_flags): Use released GCC flags.
> 	* gdb.arch/i386-mpx.exp (comp_flags): Use released GCC flags.

You labeled it as "obvious", but I'll approve anyway :)

-- 
Joel