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

[Joel Brobecker <brobecker at adacore dot com>]

> 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;
>  }
>  
> @@ -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