Re: [PATCH] [amd64] Fix AMD64 return value ABI in expression evaluation

[Pedro Alves <palves@redhat.com>]

On 02/14/2019 01:35 PM, Leszek Swirski via gdb-patches wrote:
> The AMD64 System V ABI specifies that when a function has a return type
> classified as MEMORY, the caller provides space for the value and passes
> the address to this space as the first argument to the function (before
> even the "this" pointer). The classification of MEMORY is applied to
> struct that are sufficiently large, or ones with unaligned fields.
> 
> The expression evaluator uses call_function_by_hand to call functions,
> and the hand-built frame has to push arguments in a way that matches the
> ABI of the called function. call_function_by_hand supports ABI-based
> struct returns, based on the value of gdbarch_return_value, however on
> AMD64 the implementation of the classifier incorrectly assumed that all
> non-POD types (implemented as "all types with a base class") should be
> classified as MEMORY and use the struct return.
> 
> This ABI mismatch resulted in issues when calling a function that returns
> a class of size <16 bytes which has a base class, including issues such
> as the "this" pointer being incorrect (as it was passed as the second
> argument rather than the first).
> 
> This is now fixed by checking for field alignment rather than POD-ness,
> and a testsuite is added to test expression evaluation for AMD64.

Thanks!

I wonder whether this fixes PR gdb/24104, 
<https://sourceware.org/bugzilla/show_bug.cgi?id=24104>.

Generally looks good.  Some minor comments below.

> 
> gdb/ChangeLog
> 
> 	* amd64-tdep.c (amd64_classify_aggregate): Use cp_pass_by_reference
> 	rather than a hand-rolled POD check when checking for forced MEMORY
> 	classification.
> 
> gdb/testsuite/ChangeLog
> 
>        * gdb.arch/amd64-eval.cc: New file.
>        * gdb.arch/amd64-eval.exp: New file.

Watch out for tabs vs spaces in the ChangeLog entry above.

> ---
>  gdb/ChangeLog                         |   6 ++
>  gdb/amd64-tdep.c                      |  46 +++++++++---
>  gdb/testsuite/ChangeLog               |   5 ++
>  gdb/testsuite/gdb.arch/amd64-eval.cc  | 103 ++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-eval.exp |  43 +++++++++++
>  5 files changed, 193 insertions(+), 10 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-eval.cc
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-eval.exp
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 191863e491..ffd9f4d699 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2019-02-14  Leszek Swirski  <leszeks@google.com>
> +
> +	* amd64-tdep.c (amd64_classify_aggregate): Use cp_pass_by_reference
> +	rather than a hand-rolled POD check when checking for forced MEMORY
> +	classification.
> +
>  2019-02-12  John Baldwin  <jhb@FreeBSD.org>
>  
>  	* symfile.c (find_separate_debug_file): Use canonical path of
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 3f61997d66..d2c518ed6d 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -541,15 +541,42 @@ amd64_merge_classes (enum amd64_reg_class class1, enum amd64_reg_class class2)
>  
>  static void amd64_classify (struct type *type, enum amd64_reg_class theclass[2]);
>  
> -/* Return non-zero if TYPE is a non-POD structure or union type.  */
> +/* Return non-zero if TYPE is a structure or union with unaligned fields.  */
>  
>  static int
> -amd64_non_pod_p (struct type *type)

Since you're basically rewriting this, please make it
return bool and use true/false.  Update the "non-zero" in
the comment above as well.

> -{
> -  /* ??? A class with a base class certainly isn't POD, but does this
> -     catch all non-POD structure types?  */
> -  if (TYPE_CODE (type) == TYPE_CODE_STRUCT && TYPE_N_BASECLASSES (type) > 0)
> -    return 1;
> +amd64_has_unaligned_fields (struct type *type) {

Formatting, { does on the next line.

> +
> +  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
> +      || TYPE_CODE (type) == TYPE_CODE_UNION)
> +    {
> +      int i;
> +
> +      for (i = 0; i < TYPE_NFIELDS (type); i++)

Write:      

      for (int i = 0; i < TYPE_NFIELDS (type); i++)


> +	{
> +	  struct type *subtype = check_typedef (TYPE_FIELD_TYPE (type, i));
> +	  int bitpos = TYPE_FIELD_BITPOS (type, i);
> +	  int align = type_align(subtype);
> +	  int bytepos;

Can declare bytepod below where it's initialized.

> +
> +	  /* Ignore static fields, or empty fields, for example nested
> +	     empty structures.*/

Missing double space after the period.

> +	  if (field_is_static (&TYPE_FIELD (type, i))
> +	      || (TYPE_FIELD_BITSIZE (type, i) == 0
> +		  && TYPE_LENGTH (subtype) == 0))
> +	    continue;
> +
> +	  if (bitpos % 8 != 0)
> +	   return 1;
> +	  bytepos = bitpos / 8;
> +
> +	  if (bytepos % align != 0)
> +	    return 1;
> +
> +	  if (amd64_has_unaligned_fields(subtype))
> +	    return 1;
> +	}
> +
> +    }
>  
>    return 0;
>  }
> @@ -560,10 +587,9 @@ amd64_non_pod_p (struct type *type)
>  static void
>  amd64_classify_aggregate (struct type *type, enum amd64_reg_class theclass[2])
>  {
> -  /* 1. If the size of an object is larger than two eightbytes, or in
> -        C++, is a non-POD structure or union type, or contains
> +  /* 1. If the size of an object is larger than two eightbytes, or it has
>          unaligned fields, it has class memory.  */
> -  if (TYPE_LENGTH (type) > 16 || amd64_non_pod_p (type))
> +  if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
>      {
>        theclass[0] = theclass[1] = AMD64_MEMORY;
>        return;
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index 1ee4f1bd9b..5fa2b6ef7b 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2019-02-14  Leszek Swirski  <leszeks@google.com>
> +
> +	* gdb.arch/amd64-eval.cc: New file.
> +	* gdb.arch/amd64-eval.exp: New file.
> +
>  2019-02-12  Weimin Pan  <weimin.pan@oracle.com>
>  
>  	PR breakpoints/21870
> diff --git a/gdb/testsuite/gdb.arch/amd64-eval.cc b/gdb/testsuite/gdb.arch/amd64-eval.cc
> new file mode 100644
> index 0000000000..81f9ba589c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-eval.cc
> @@ -0,0 +1,103 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2019 Free Software Foundation, Inc.
> +
> +   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 <cstdio>
> +#include <cstdlib>
> +#include <cassert>
> +
> +// A simple structure with a single integer field. Should be returned in
> +// a register.

We follow GDB's conventions in testcases as well.  Use /**/ comments throughout,
and double space after periods.

> +struct SimpleBase {

{ on the next line.

> +  int32_t x;

Missing cstdint include for int32_t.

> +};
> +
> +// A simple structure derived from the simple base. Should be returned in
> +// a register.
> +struct SimpleDerived : public SimpleBase {};
> +
> +// A structure derived from the simple base with a non-trivial destructor.
> +// Should be returned on the stack.
> +struct NonTrivialDestructorDerived : public SimpleBase {

{ on the next line.


> +  ~NonTrivialDestructorDerived() { x = 1; }
> +};
> +
> +// A structure with unaligned fields. Should be returned on the stack.
> +struct UnalignedFields {
> +  int32_t x;

{ on the next line.


> +  double y;
> +} __attribute__((packed));
> +
> +// A structure with unaligned fields in its base class. Should be
> +// returned on the stack.
> +struct UnalignedFieldsInBase : public UnalignedFields {

{ on the next line.

> +  int32_t x2;
> +};
> +
> +class Foo {

{ on the next line.

> +public:
> +  SimpleBase
> +  return_simple_base ()
> +  {
> +    assert(this->tag == EXPECTED_TAG);
> +    return SimpleBase();

Missing space before parens, both in assert call and
in ctor call.  Happens multiple times.

> +  }
> +
> +  SimpleDerived
> +  return_simple_derived ()
> +  {
> +    assert(this->tag == EXPECTED_TAG);
> +    return SimpleDerived();
> +  }
> +
> +  NonTrivialDestructorDerived
> +  return_non_trivial_destructor ()
> +  {
> +    assert(this->tag == EXPECTED_TAG);
> +    return NonTrivialDestructorDerived();
> +  }
> +
> +  UnalignedFields
> +  return_unaligned ()
> +  {
> +    assert(this->tag == EXPECTED_TAG);
> +    return UnalignedFields();
> +  }
> +
> +  UnalignedFieldsInBase
> +  return_unaligned_in_base ()
> +  {
> +    assert(this->tag == EXPECTED_TAG);
> +    return UnalignedFieldsInBase();
> +  }
> +
> +private:
> +  // Use a tag to detect if the "this" value is correct.
> +  static const int EXPECTED_TAG = 0xF00F00F0;
> +  int tag = EXPECTED_TAG;
> +};
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  Foo foo;
> +  foo.return_simple_base();

Space before parens.

> +  foo.return_simple_derived();
> +  foo.return_non_trivial_destructor();
> +  foo.return_unaligned();
> +  foo.return_unaligned_in_base();
> +  return 0;  // break-here
> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-eval.exp b/gdb/testsuite/gdb.arch/amd64-eval.exp
> new file mode 100644
> index 0000000000..f3da83e4ca
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-eval.exp
> @@ -0,0 +1,43 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2019 Free Software Foundation, Inc.
> +
> +# 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 testcase exercises evaluation with amd64 calling conventions.
> +
> +standard_testfile .cc
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  { debug c++ }] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +gdb_breakpoint [gdb_get_line_number "break-here"]
> +gdb_continue_to_breakpoint "break-here"
> +
> +gdb_test "call foo.return_simple_base()" \
> +    ".* = {x = 0}"

Leading ".*" is unnecessary -- it's implied.

> +gdb_test "call foo.return_simple_derived()" \
> +    ".* = {<SimpleBase> = {x = 0}, <No data fields>}"
> +gdb_test "call foo.return_non_trivial_destructor()" \
> +    ".* = {<SimpleBase> = {x = 0}, <No data fields>}"
> +gdb_test "call foo.return_unaligned()" \
> +    ".* = {x = 0, y = 0}"
> +gdb_test "call foo.return_unaligned_in_base()" \
> +    ".* = {<UnalignedFields> = {x = 0, y = 0}, x2 = 0}"
> 

Is it possible to use values other than "0" in x/y/x2 in
the success cases?  That tends to be more robust to changes,
because it's easier to hit zeroed memory with a bug and
not notice it than some other value.

Thanks,
Pedro Alves