This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH V2] ABI changes for MPX.


Walfred,

I still had lots of comments, but generally speaking, we're making
progress, so please do not feel discouraged. We're getting there.

On Thu, Dec 17, 2015 at 02:23:44PM +0100, Walfred Tedeschi wrote:
> 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. (page 134)
> 
> When using the return command, execution of a function is aborted
> and present values are returned from that point.  That can cause
> bound violations in the MPX context.  To avoid such side-effects
> a new set variable was added "mpx-bnd-init-on-return" which controls
> the initialization of bound register when using the return command.
> 
> As bound initialization it is understood the set of the BND register
> to zero allowing the associated pointer to access the whole memory.
> 
> As default the value of "mpx-bnd-init-on-return" is set to 1.  So
> bound register are initilized when using the "return" command.
> 
> 
> 2015-12-15  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-tdep.h (amd64_regnum): Add MPX registers.
> 	* 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.

Small nit: You you need to start each new file with a '*'. And
there is no colon (':') between the file name and the context
in between '(' and ')'. Therefore:

 	* 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.

Same remarks as above. Also, those files are in a subdirectory
of the testsuite directory, so you need to provide that info
in the ChangeLog entry, since the entries are relative to
the ChangeLog file and that ChangeLog is in gdb/testsuite/.
Therefore:

gdb/testsuite/ChangeLog:

        * gdb.arch/amd64-mpx-call.c: New.
        * gdb.arch/amd64-mpx-call.exp: New.

Have you validated this patch against a compiler using the regular
amd64 ABI (Eg. x86_64-linux)?

A small note: As I am slowly understanding the real purpose
behind the patch, and how it is implemented, I think now that
this patch should have been split into 2 smaller patches, with:
  - first, a patch which adds implementation of the POINTER class;
  - second, a patch which adds the implementation of your new
    setting to reset/initialize the bound registers upon use of
    the "return" command.

I forgot it last time, but because this patch introduces a new
set of commands, it needs a NEWS entry (also reviewed by Eli).

> ---
>  gdb/amd64-tdep.c                          | 109 ++++++++++++++++++++++++--
>  gdb/amd64-tdep.h                          |   3 +-
>  gdb/doc/gdb.texinfo                       |  23 ++++++
>  gdb/testsuite/gdb.arch/amd64-mpx-call.c   | 126 ++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-mpx-call.exp |  92 ++++++++++++++++++++++
>  5 files changed, 345 insertions(+), 8 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 6096ce9..dee900f 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -461,6 +461,7 @@ amd64_pseudo_register_write (struct gdbarch *gdbarch,
>  
>  enum amd64_reg_class
>  {
> +  AMD64_POINTER,
>    AMD64_INTEGER,
>    AMD64_SSE,
>    AMD64_SSEUP,
> @@ -492,18 +493,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;
>  }
>  
> @@ -636,14 +641,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.  */

I couldn't understand why you used the parentheses to delimit
the types, until I saw the next comment. But in the next comment,
"(signed and unsigned)" applied to _Bool. But the comment was
really saying "Arguments of types _Bool, char, [etc]".

I would change the comment to simplay say:

  /* Pointers and references are of the POINTER class.  */

> +  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;
>  
> @@ -693,16 +701,25 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
>      amd64_classify_aggregate (type, theclass);
>  }
>  
> +/* Defines whether bound registers will be initialized when "return"
> +   command is used.  Default is one avoiding bound violations.  */

I would rather make the relationship with the setting it support.
That way, we know what the purpose of the variable is.
I would also omit what the default is, since you can see it
plainly just by looking at the definition. And that way, if we
ever decide to change the default, we don't have to change
the comment as well (or worse, forget to update the comment).
My suggestion:

/* The value of the "set mpx-bnd-init-on-return" setting.  */


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

The last line is indented one character too deep. It should be:

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

By the way, shouldn't the second entry be AMD64_BND1R_REGNUM?

>    int integer_reg = 0;
> +  int bnd_reg = 0;
>    int sse_reg = 0;
>    int i;
>  
> @@ -730,6 +747,28 @@ 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
> +	      case the application is MPX enabled.
> +
> +	      When the command "return" is issued, here writebuf not null,
> +	      the state of bnd0 might not be ready to be returned
> +	      (may be holding the bounds of another pointer).
> +	      The variable mpx_bnd_init_on_return control initialization
> +	      of bnd0. Zero value in the register mean all memory range,
> +	      see Instruction Set Architecture.  In case bnd0 is not reset
> +	      unrelated bound violations may occur.
> +	      Otherwise, mpx_bnd_init_on_return is zero, the actual
> +	      register value will be returned.  */

The text inside the comment is indented one space too deep (because
you inserted 2 spaces instead of one after the opening '/*'.

Thanks to the new version of your comment, I think I am finally
seeing the scenario when this is being used. Basically, you are
trying to prevent a program to fails with a bound violation when
a user uses the "return" command (a command which causes the
program to return prematurely, and therefore possibly before
the bound registers are set) from failing on a spurious bound
violation, right?

If I am right, I think we can further improve your comment with
the following wording:

	  /* In MPX-enabled programs, if the class is MEMORY,
             Boundary is expected to be stored in the bnd0 register.

             If the return value we are setting up is due to the user
             using the "return" command (WRITEBUF is not NULL),
             it is likely that the return is made prematurely,
             and therefore before the bnd0 register is being set.
             Leaving this bnd0 register unset could then cause
             the program to receive a spurious bound violation.
             Initializing the bnd0 register to zero avoids this
             violation, but this is only done if "set
             mpx-bnd-init-on-return" is true.  */

A small general comments on our coding style: The GNU Coding
Standard (which the GDB Coding Style follows as all GNU projects,
and then adds to) says that, by convention, VARIABLE_NAME means
"the value of variable_name". Hence my use of "WRITE_BUF" above.

Note also that the previous authors of this file have been using
the class names in all-caps: Eg. "MEMORY" rather than "memory".
I made that change in my new suggestion.

> +	  if (writebuf != NULL
> +	      && mpx_bnd_init_on_return && AMD64_BND0R_REGNUM > 0)

AMD64_BND0R_REGNUM is always > 0. I think what you meant was
to use I387_BND0R_REGNUM(tdep). I think you should actually
use that throughout, rather than reference AMD64_BND0R_REGNUM,
since AMD64_BND0R_REGNUM is only meaningful if the program
is MPX-enabled (correct?). Also, one of the upside of using
I387_BND0R_REGNUM(tdep) is that we could conceivably make
that regnum dynamic, if we ever needed to.

> +	    {
> +	      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;
> @@ -765,6 +804,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])
> @@ -775,6 +815,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 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.  */
> @@ -823,6 +870,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);
> +    }

There is something disconnected between the fact that you are
adding new variables bnd_regnum, bnd_reg_count and bnd_reg,
and yet, you don't really do anything with it. The code above
just initializes all bnd registers...

By the way, bnd_regnum has 4 bnd registers, but if the class is
POINTER, how can you have a length greather than 16, and therefore
use more than two bnd registers?

[also, just a second and last reminder to use I387_BND0R_REGNUM (tdep)
rather than AMD64_BND0R_REGNUM directly].

> +
>    return RETURN_VALUE_REGISTER_CONVENTION;
>  }
>  
> @@ -876,7 +934,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++;
> @@ -907,6 +965,7 @@ amd64_push_arguments (struct regcache *regcache, int nargs,
>  
>  	      switch (theclass[j])
>  		{
> +		case AMD64_POINTER:
>  		case AMD64_INTEGER:
>  		  regnum = integer_regnum[integer_reg++];
>  		  break;
> @@ -966,8 +1025,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 an undesired  bound violation
> +     during function's execution.  */

Can you remove the second space between "undesired" and "bound"?
Also, please add "the" before "function's".

> +  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);
>  
> @@ -2932,6 +3006,19 @@ static const int amd64_record_regmap[] =
>    AMD64_DS_REGNUM, AMD64_ES_REGNUM, AMD64_FS_REGNUM, AMD64_GS_REGNUM
>  };
>  
> +/*  show state of the setting variable mpx-bnd-init-on-return.  */
> +static void
> +show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
> +			  struct cmd_list_element *c, const char *value)

The arguments on the 2 (and subsequent lines) much be aligned
with the arguments on the first line. Hence:

show_mpx_bnd_init_on_return (struct ui_file *file, int from_tty,
			     struct cmd_list_element *c, const char *value)

> +{
> +  if (mpx_bnd_init_on_return == 1)

Boolean settings are actually implemented differently. Any non-zero
value means "true". So can you remove the "== 1"?

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

Same as above, the indentation on the second line is off.

> +}
> +
>  void
>  amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>  {
> @@ -2982,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_bnd_init_on_return,
> +			    &setlist, &showlist);

I think it's simpler and less confusing to define the command
regardless of whether the target supports it. For those whose
target does not support it, it will just be a no-op. Also, it has
the nice property that people debugging on multiple platforms who
want to change the default can do so in their $HOME/.gdbinit.
If you only enable this when mpx is supported, then you'll trigger
an error during .gdbinit evaluation, which then aborts its
processing. Not cool.

Actually, amd64_init_abi is the wrong place to be adding this
command, since amd64_init_abi can be called multiple times,
I believe. Each time you provide GDB with a binary, it looks
at it, and determines its architecture. So it is possible that
GDB ends up switching between architectures, meaning this function
could be called multiple times.


>        tdep->mpx_register_names = amd64_mpx_names;
>        tdep->bndcfgu_regnum = AMD64_BNDCFGU_REGNUM;
>        tdep->bnd0r_regnum = AMD64_BND0R_REGNUM;
> 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,
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index b82f3c6..fd7fc24 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -22064,6 +22064,29 @@ 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 from the debugger, of an Intel(R) MPX enabled program,
> +boundary registers have to be initialized before performing the call, to avoid
> +boundary violations while performing the call.  A bound is defined to be
> +initialized when the pointer associated to that boundary can access the whole
> +memory, in this case the register bound register associated to it has value 0,
> +e.g. if the register associated is bnd0raw its value will be @{0x0, 0x0@}.
> +It is possible to change the boundary values, if desired, by placing
> +a breakpoint at prologue's end and setting bound registers as wished.
> +After the call is performed bound register might be keept or not for
> +further investigations.
> +
> +While the using the @command{return} bounds can propagate through
> +execution causing a boundary violation.
> +The behaviour of initializing bounds when using @command{return}
> +can be controlled and vizualized via the following commands:

If I understand the purpose of this feature (see comment I tried
to write above), then the note mentioning how to change the value
of the bounds register (placing a breakpoint after the function's
prologue) adds more information which dilutes the message, making
you wonder why this is mentioned.

If handling of the bound registers (in the sense of the usual
register handling commands) is the same as with all other registers,
then the suggestion to insert a breakpoint at end of prologue is
nearly SOP, and therefore is it worth mentioning here? I don't
have a strong opinion, but this contributed to my taking a long
time understanding the purpose of this new feature (assuming
I do).

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

This is a boolean setting, so "1" is not the right wording, I think.
It shuld be @samp{on} I believe.

> +@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..7f12e3e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.c
> @@ -0,0 +1,126 @@
> +/* Test for inferior function calls MPX context.
> +
> +   Copyright (C) 2015 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 OUR_SIZE    5
> +
> +int gx[OUR_SIZE];
> +int ga[OUR_SIZE];
> +int gb[OUR_SIZE];
> +int gc[OUR_SIZE];
> +int 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 (int value)
> +{
> +  return 1;
> +}
> +
> +int
> +bp2 (int value)
> +{
> +  return 1;
> +}
> +
> +int
> +upper (int *p, int *a, int *b, int *c, int *d, int len)
> +{
> +  int value;
> +
> +  value = * (p + len);
> +  value = * (a + len);
> +  value = * (b + len);
> +  value = * (c + len);
> +  value = * (d + len);

No space after the '*' -> *(p + len)

> +
> +  value = value - value + 1;
> +  return value;
> +}
> +
> +int *
> +upper_ptr (int *p, int *a, int *b, int *c, int *d, int len)
> +{
> +  int 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 (int));
> +
> +  return ++p;
> +}
> +
> +
> +int
> +main (void)
> +{
> +  if (have_mpx ())
> +    {
> +      int sx[OUR_SIZE];
> +      int sa[OUR_SIZE];
> +      int sb[OUR_SIZE];
> +      int sc[OUR_SIZE];
> +      int sd[OUR_SIZE];
> +      int *x, *a, *b, *c, *d;
> +
> +      x = calloc (OUR_SIZE, sizeof (int));
> +      a = calloc (OUR_SIZE, sizeof (int));
> +      b = calloc (OUR_SIZE, sizeof (int));
> +      c = calloc (OUR_SIZE, sizeof (int));
> +      d = calloc (OUR_SIZE, sizeof (int));
> +
> +      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..6b756b2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-mpx-call.exp
> @@ -0,0 +1,92 @@
> +# Copyright (C) 2015 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}]] } {

The indentation of the second line is off. It's the same principle
as in the C code. Therefore:

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

I'm curious. Why '..' before the '='. I'm more used to seing '.*'...

> +}
> +
> +# Needed by the return command.
> +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)" "= 1"\
> +  "test the call of int function - int"

In the testcases (which is expect/tcl), we indent the second
line by 4 characters. We also typically add a space before
a terminating backslash (\), which I think helps readability.
Thus:

    | gdb_test "p upper (x, a, b, c, d, 0)" "= 1" \
    |     "test the call of int function - int"

Can you take care of finding all other instances, and fixing those
as well?

> +gdb_test "p upper_ptr (x, a, b, c, d, 0)"\
> +  "= \\\(int \\\*\\\) $hex" "test the call of function - ptr"
> +
> +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" "" "Saving the\
> +  bnd0 result on a convenience variable 1"

"in" a convenience variable (rather than "on"). I would also format
this a little different so as to avoid breaking in the middle of
a string, thus causing 2 spaces between "the" and "bnd0" (no "the",
by the way, or else say "the bnd0 register").

The test is also strange. You use the "print" command, but yet
you do not test the command's output. Why not do:

gdb_test_no_output "set variable \$bound0 = \$bnd0" \
    "Saving bnd0 in a convenience variable 1"

?

> +gdb_test "return a"

No verification of output?
Also, this same test is performed more than once, so please
provide a test name of each one of them (to make sure it test
has a unique name).

> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
> +  "= 0" "return with bnd initialization off - ubound"
> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
> +  "= 0" "return with bnd initialization off - lbound"
> +
> +runto_main
> +gdb_test_no_output "set mpx-bnd-init-on-return off"\
> +  Turn off initialization on return"

I think there is a missing double-quote for the last argument,
which makes me think that the test will terminate abruptly
with an exception at this stage.

> +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}"]

I don't think you need those, do you? You have already created
the breakpoints earlier, and haven't deleted them. So these
tests are probably just creating duplicates.

If, for some reason, I missed something and you actually do need
to re-create those breakpoints, then make sure that you provide
unique names for these tests (since you are doing the same test
twice for each of these tests).

> +gdb_test "p \$bound0 = \$bnd0" "" "Saving the\
> +  bnd0 result on a convenience variable 2"

Same as above.

> +gdb_test "return a"

Same as above.

> +gdb_test "p \(\$bnd0\.ubound == \$bound0\.ubound\)"\
> +  "= 1" "return with bnd initialization on - ubound"
> +gdb_test "p \(\$bnd0\.lbound ==\$bound0\.lbound\)"\
> +  "= 1" "return with bnd initialization on - lbound"
> +
> +gdb_test "show mpx-bnd-init-on-return"\
> +  "BND registers will not be initialized on return."\

This is a bit of a nit, but make the test a little more precise:
Make sure to escape the last period. Otherwise, it'll match anything,
not just a period (ie "\\." instead of ".").

> +  "double check initialization on return"

-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]