Re: [PATCH V3 1/2] ABI changes for MPX.

[Joel Brobecker <brobecker@adacore.com>]

Walfred,

On Wed, Jan 13, 2016 at 01:39:58PM +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)
> 
> In addition to that set bound registers to INIT state (access to
> all memory) before performing the inferior function call.
> 
> 2016-01-13  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.
> 	(amd64_return_value): Add AMD64_POINTER.
> 	(amd64_push_arguments): Add new AMD64_POINTER class.
> 	(amd64_push_dummy_call): Set bound registers to INIT state before
> 	performing the call.

I think you need to update the following piece of code as well (in
amd64_push_arguments):

      /* Calculate the number of integer and SSE registers needed for
         this argument.  */
      for (j = 0; j < 2; j++)
        {
          if (theclass[j] == AMD64_INTEGER)
            needed_integer_regs++;

Otherwise, you'll have a discrepancy between needed_integer_regs
and integer_reg.

As far as I understand, POINTER and INTEGER arguments are passed
exactly the same way. The distinction appears to only be useful
for bound register handling. And since this patch unilaterally
sets the bound registers to zero, we could have done it without
adding the extra register classification (and thus reducing
the maintenance cost of having to handle both). I'm really tempted
to suggest we drop the new class altogether, avoiding the risk
of having missed another spot where we needd to add POINTER class
handling, and just keep the change which sets the bound registers
to zero. If, one day, someone decides to enhance the debugger
to handle the bound registers more closely to what the ABI suggests,
I think we could have a separate function that would tell us if
a given type is of class POINTER or not, rather than adding a class.

Am I understanding the situation correctly?

> gdb/doc/ChangeLog:
> 
> 	* gdb.texinfo (Intel Memory Protection Extension): Add entry for
> 	inferior function calls for MPX.


> @@ -693,6 +701,7 @@ amd64_classify (struct type *type, enum amd64_reg_class theclass[2])
>      amd64_classify_aggregate (type, theclass);
>  }
>  
> +
>  static enum return_value_convention
>  amd64_return_value (struct gdbarch *gdbarch, struct value *function,
>  		    struct type *type, struct regcache *regcache,

There is no reason to add a new line, here, so let's avoid this
unnecessary change.

-- 
Joel