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

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

[Leszek Swirski via gdb-patches]

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.

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.
---
 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)
-{
-  /* ??? 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) {
+
+  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
+      || TYPE_CODE (type) == TYPE_CODE_UNION)
+    {
+      int i;
+
+      for (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;
+
+	  /* Ignore static fields, or empty fields, for example nested
+	     empty structures.*/
+	  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.
+struct SimpleBase {
+  int32_t x;
+};
+
+// 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 {
+  ~NonTrivialDestructorDerived() { x = 1; }
+};
+
+// A structure with unaligned fields. Should be returned on the stack.
+struct UnalignedFields {
+  int32_t x;
+  double y;
+} __attribute__((packed));
+
+// A structure with unaligned fields in its base class. Should be
+// returned on the stack.
+struct UnalignedFieldsInBase : public UnalignedFields {
+  int32_t x2;
+};
+
+class Foo {
+public:
+  SimpleBase
+  return_simple_base ()
+  {
+    assert(this->tag == EXPECTED_TAG);
+    return SimpleBase();
+  }
+
+  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();
+  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}"
+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}"
-- 
2.21.0.rc0.258.g878e2cd30e-goog

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

[Pedro Alves]

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

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

[Leszek Swirski via gdb-patches]

On Thu, Feb 14, 2019 at 3:28 PM Pedro Alves <palves@redhat.com> wrote:
> I wonder whether this fixes PR gdb/24104,
> <https://sourceware.org/bugzilla/show_bug.cgi?id=24104>.
Doesn't look like it, nor would I expect it too -- all those functions
return ints after all. Anything after the first stack frame won't
matter.

> Generally looks good.  Some minor comments below.
Thanks, new patch incoming. Apologies for the style violation churn,
I'm not that used to the GNU style guide.

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

[Leszek Swirski via gdb-patches]

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.

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.
---
 gdb/ChangeLog                         |   6 ++
 gdb/amd64-tdep.c                      |  44 +++++++---
 gdb/testsuite/ChangeLog               |   5 ++
 gdb/testsuite/gdb.arch/amd64-eval.cc  | 120 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-eval.exp |  43 +++++++++
 5 files changed, 207 insertions(+), 11 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..d32638a20a 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -541,17 +541,40 @@ 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 true if TYPE is a structure or union with unaligned fields.  */
 
-static int
-amd64_non_pod_p (struct type *type)
+static bool
+amd64_has_unaligned_fields (struct type *type)
 {
-  /* ??? 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;
+  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
+      || TYPE_CODE (type) == TYPE_CODE_UNION)
+    {
+      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);
 
-  return 0;
+	  /* Ignore static fields, or empty fields, for example nested
+	     empty structures.  */
+	  if (field_is_static (&TYPE_FIELD (type, i))
+	      || (TYPE_FIELD_BITSIZE (type, i) == 0
+		  && TYPE_LENGTH (subtype) == 0))
+	    continue;
+
+	  if (bitpos % 8 != 0)
+	    return true;
+
+	  int bytepos = bitpos / 8;
+	  if (bytepos % align != 0)
+	    return true;
+
+	  if (amd64_has_unaligned_fields(subtype))
+	    return true;
+	}
+    }
+
+  return false;
 }
 
 /* Classify TYPE according to the rules for aggregate (structures and
@@ -560,10 +583,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..a986a49db3
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-eval.cc
@@ -0,0 +1,120 @@
+/* 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 <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <cassert>
+
+/* A simple structure with a single integer field. Should be returned in
+   a register.  */
+struct SimpleBase
+{
+  SimpleBase (int32_t x) : x (x) {}
+
+  int32_t x;
+};
+
+/* A simple structure derived from the simple base. Should be returned in
+   a register.  */
+struct SimpleDerived : public SimpleBase
+{
+  SimpleDerived (int32_t x) : SimpleBase (x) {}
+};
+
+/* A structure derived from the simple base with a non-trivial destructor.
+   Should be returned on the stack.  */
+struct NonTrivialDestructorDerived : public SimpleBase
+{
+  NonTrivialDestructorDerived (int32_t x) : SimpleBase (x) {}
+  ~NonTrivialDestructorDerived() { x = 1; }
+};
+
+/* A structure with unaligned fields. Should be returned on the stack.  */
+struct UnalignedFields
+{
+  UnalignedFields (int32_t x, double y) : x (x), y (y) {}
+
+  int32_t x;
+  double y;
+} __attribute__((packed));
+
+/* A structure with unaligned fields in its base class. Should be
+   returned on the stack.  */
+struct UnalignedFieldsInBase : public UnalignedFields
+{
+  UnalignedFieldsInBase (int32_t x, double y, int32_t x2)
+  : UnalignedFields (x, y), x2 (x2) {}
+
+  int32_t x2;
+};
+
+class Foo
+{
+public:
+  SimpleBase
+  return_simple_base (int32_t x)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return SimpleBase (x);
+  }
+
+  SimpleDerived
+  return_simple_derived (int32_t x)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return SimpleDerived (x);
+  }
+
+  NonTrivialDestructorDerived
+  return_non_trivial_destructor (int32_t x)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return NonTrivialDestructorDerived (x);
+  }
+
+  UnalignedFields
+  return_unaligned (int32_t x, double y)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return UnalignedFields (x, y);
+  }
+
+  UnalignedFieldsInBase
+  return_unaligned_in_base (int32_t x, double y, int32_t x2)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return UnalignedFieldsInBase (x, y, x2);
+  }
+
+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(1);
+  foo.return_simple_derived(2);
+  foo.return_non_trivial_destructor(3);
+  foo.return_unaligned(4, 5);
+  foo.return_unaligned_in_base(6, 7, 8);
+  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..c33777d2b4
--- /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(12)" \
+    " = {x = 12}"
+gdb_test "call foo.return_simple_derived(34)" \
+    " = {<SimpleBase> = {x = 34}, <No data fields>}"
+gdb_test "call foo.return_non_trivial_destructor(56)" \
+    " = {<SimpleBase> = {x = 56}, <No data fields>}"
+gdb_test "call foo.return_unaligned(78, 9.25)" \
+    " = {x = 78, y = 9.25}"
+gdb_test "call foo.return_unaligned_in_base(23, 4.5, 67)" \
+    " = {<UnalignedFields> = {x = 23, y = 4.5}, x2 = 67}"
-- 
2.21.0.rc0.258.g878e2cd30e-goog

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

[Leszek Swirski via gdb-patches]

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.

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.
---
 gdb/ChangeLog                         |   6 ++
 gdb/amd64-tdep.c                      |  44 +++++++---
 gdb/testsuite/ChangeLog               |   5 ++
 gdb/testsuite/gdb.arch/amd64-eval.cc  | 120 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-eval.exp |  43 +++++++++
 5 files changed, 207 insertions(+), 11 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..d32638a20a 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -541,17 +541,40 @@ 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 true if TYPE is a structure or union with unaligned fields.  */
 
-static int
-amd64_non_pod_p (struct type *type)
+static bool
+amd64_has_unaligned_fields (struct type *type)
 {
-  /* ??? 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;
+  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
+      || TYPE_CODE (type) == TYPE_CODE_UNION)
+    {
+      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);
 
-  return 0;
+	  /* Ignore static fields, or empty fields, for example nested
+	     empty structures.  */
+	  if (field_is_static (&TYPE_FIELD (type, i))
+	      || (TYPE_FIELD_BITSIZE (type, i) == 0
+		  && TYPE_LENGTH (subtype) == 0))
+	    continue;
+
+	  if (bitpos % 8 != 0)
+	    return true;
+
+	  int bytepos = bitpos / 8;
+	  if (bytepos % align != 0)
+	    return true;
+
+	  if (amd64_has_unaligned_fields(subtype))
+	    return true;
+	}
+    }
+
+  return false;
 }
 
 /* Classify TYPE according to the rules for aggregate (structures and
@@ -560,10 +583,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..a986a49db3
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/amd64-eval.cc
@@ -0,0 +1,120 @@
+/* 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 <cstdint>
+#include <cstdio>
+#include <cstdlib>
+#include <cassert>
+
+/* A simple structure with a single integer field. Should be returned in
+   a register.  */
+struct SimpleBase
+{
+  SimpleBase (int32_t x) : x (x) {}
+
+  int32_t x;
+};
+
+/* A simple structure derived from the simple base. Should be returned in
+   a register.  */
+struct SimpleDerived : public SimpleBase
+{
+  SimpleDerived (int32_t x) : SimpleBase (x) {}
+};
+
+/* A structure derived from the simple base with a non-trivial destructor.
+   Should be returned on the stack.  */
+struct NonTrivialDestructorDerived : public SimpleBase
+{
+  NonTrivialDestructorDerived (int32_t x) : SimpleBase (x) {}
+  ~NonTrivialDestructorDerived() { x = 1; }
+};
+
+/* A structure with unaligned fields. Should be returned on the stack.  */
+struct UnalignedFields
+{
+  UnalignedFields (int32_t x, double y) : x (x), y (y) {}
+
+  int32_t x;
+  double y;
+} __attribute__((packed));
+
+/* A structure with unaligned fields in its base class. Should be
+   returned on the stack.  */
+struct UnalignedFieldsInBase : public UnalignedFields
+{
+  UnalignedFieldsInBase (int32_t x, double y, int32_t x2)
+  : UnalignedFields (x, y), x2 (x2) {}
+
+  int32_t x2;
+};
+
+class Foo
+{
+public:
+  SimpleBase
+  return_simple_base (int32_t x)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return SimpleBase (x);
+  }
+
+  SimpleDerived
+  return_simple_derived (int32_t x)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return SimpleDerived (x);
+  }
+
+  NonTrivialDestructorDerived
+  return_non_trivial_destructor (int32_t x)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return NonTrivialDestructorDerived (x);
+  }
+
+  UnalignedFields
+  return_unaligned (int32_t x, double y)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return UnalignedFields (x, y);
+  }
+
+  UnalignedFieldsInBase
+  return_unaligned_in_base (int32_t x, double y, int32_t x2)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return UnalignedFieldsInBase (x, y, x2);
+  }
+
+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(1);
+  foo.return_simple_derived(2);
+  foo.return_non_trivial_destructor(3);
+  foo.return_unaligned(4, 5);
+  foo.return_unaligned_in_base(6, 7, 8);
+  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..c33777d2b4
--- /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(12)" \
+    " = {x = 12}"
+gdb_test "call foo.return_simple_derived(34)" \
+    " = {<SimpleBase> = {x = 34}, <No data fields>}"
+gdb_test "call foo.return_non_trivial_destructor(56)" \
+    " = {<SimpleBase> = {x = 56}, <No data fields>}"
+gdb_test "call foo.return_unaligned(78, 9.25)" \
+    " = {x = 78, y = 9.25}"
+gdb_test "call foo.return_unaligned_in_base(23, 4.5, 67)" \
+    " = {<UnalignedFields> = {x = 23, y = 4.5}, x2 = 67}"
-- 
2.21.0.rc0.258.g878e2cd30e-goog

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

[Pedro Alves]

OK.

Thanks,
Pedro Alves
On 02/14/2019 03:18 PM, Leszek Swirski 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.
> 
> 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.
> ---
>  gdb/ChangeLog                         |   6 ++
>  gdb/amd64-tdep.c                      |  44 +++++++---
>  gdb/testsuite/ChangeLog               |   5 ++
>  gdb/testsuite/gdb.arch/amd64-eval.cc  | 120 ++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-eval.exp |  43 +++++++++
>  5 files changed, 207 insertions(+), 11 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..d32638a20a 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -541,17 +541,40 @@ 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 true if TYPE is a structure or union with unaligned fields.  */
>  
> -static int
> -amd64_non_pod_p (struct type *type)
> +static bool
> +amd64_has_unaligned_fields (struct type *type)
>  {
> -  /* ??? 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;
> +  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
> +      || TYPE_CODE (type) == TYPE_CODE_UNION)
> +    {
> +      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);
>  
> -  return 0;
> +	  /* Ignore static fields, or empty fields, for example nested
> +	     empty structures.  */
> +	  if (field_is_static (&TYPE_FIELD (type, i))
> +	      || (TYPE_FIELD_BITSIZE (type, i) == 0
> +		  && TYPE_LENGTH (subtype) == 0))
> +	    continue;
> +
> +	  if (bitpos % 8 != 0)
> +	    return true;
> +
> +	  int bytepos = bitpos / 8;
> +	  if (bytepos % align != 0)
> +	    return true;
> +
> +	  if (amd64_has_unaligned_fields(subtype))
> +	    return true;
> +	}
> +    }
> +
> +  return false;
>  }
>  
>  /* Classify TYPE according to the rules for aggregate (structures and
> @@ -560,10 +583,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..a986a49db3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-eval.cc
> @@ -0,0 +1,120 @@
> +/* 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 <cstdint>
> +#include <cstdio>
> +#include <cstdlib>
> +#include <cassert>
> +
> +/* A simple structure with a single integer field. Should be returned in
> +   a register.  */
> +struct SimpleBase
> +{
> +  SimpleBase (int32_t x) : x (x) {}
> +
> +  int32_t x;
> +};
> +
> +/* A simple structure derived from the simple base. Should be returned in
> +   a register.  */
> +struct SimpleDerived : public SimpleBase
> +{
> +  SimpleDerived (int32_t x) : SimpleBase (x) {}
> +};
> +
> +/* A structure derived from the simple base with a non-trivial destructor.
> +   Should be returned on the stack.  */
> +struct NonTrivialDestructorDerived : public SimpleBase
> +{
> +  NonTrivialDestructorDerived (int32_t x) : SimpleBase (x) {}
> +  ~NonTrivialDestructorDerived() { x = 1; }
> +};
> +
> +/* A structure with unaligned fields. Should be returned on the stack.  */
> +struct UnalignedFields
> +{
> +  UnalignedFields (int32_t x, double y) : x (x), y (y) {}
> +
> +  int32_t x;
> +  double y;
> +} __attribute__((packed));
> +
> +/* A structure with unaligned fields in its base class. Should be
> +   returned on the stack.  */
> +struct UnalignedFieldsInBase : public UnalignedFields
> +{
> +  UnalignedFieldsInBase (int32_t x, double y, int32_t x2)
> +  : UnalignedFields (x, y), x2 (x2) {}
> +
> +  int32_t x2;
> +};
> +
> +class Foo
> +{
> +public:
> +  SimpleBase
> +  return_simple_base (int32_t x)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return SimpleBase (x);
> +  }
> +
> +  SimpleDerived
> +  return_simple_derived (int32_t x)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return SimpleDerived (x);
> +  }
> +
> +  NonTrivialDestructorDerived
> +  return_non_trivial_destructor (int32_t x)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return NonTrivialDestructorDerived (x);
> +  }
> +
> +  UnalignedFields
> +  return_unaligned (int32_t x, double y)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return UnalignedFields (x, y);
> +  }
> +
> +  UnalignedFieldsInBase
> +  return_unaligned_in_base (int32_t x, double y, int32_t x2)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return UnalignedFieldsInBase (x, y, x2);
> +  }
> +
> +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(1);
> +  foo.return_simple_derived(2);
> +  foo.return_non_trivial_destructor(3);
> +  foo.return_unaligned(4, 5);
> +  foo.return_unaligned_in_base(6, 7, 8);
> +  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..c33777d2b4
> --- /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(12)" \
> +    " = {x = 12}"
> +gdb_test "call foo.return_simple_derived(34)" \
> +    " = {<SimpleBase> = {x = 34}, <No data fields>}"
> +gdb_test "call foo.return_non_trivial_destructor(56)" \
> +    " = {<SimpleBase> = {x = 56}, <No data fields>}"
> +gdb_test "call foo.return_unaligned(78, 9.25)" \
> +    " = {x = 78, y = 9.25}"
> +gdb_test "call foo.return_unaligned_in_base(23, 4.5, 67)" \
> +    " = {<UnalignedFields> = {x = 23, y = 4.5}, x2 = 67}"
> 

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

[Leszek Swirski via gdb-patches]

On Thu, Feb 14, 2019 at 10:26 AM Pedro Alves <palves@redhat.com> wrote:
> OK.

Following up on this, what are my next steps? Does this need further
review, or can someone push it?

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

[Simon Marchi]

On 2019-04-13 12:24 p.m., Leszek Swirski via gdb-patches wrote:
> On Thu, Feb 14, 2019 at 10:26 AM Pedro Alves <palves@redhat.com> wrote:
>> OK.
> 
> Following up on this, what are my next steps? Does this need further
> review, or can someone push it?
> 

Maybe Pedro presumed that you had push access.  In any case, I've
applied the patch, tested it locally, then pushed it.

Thanks!

Simon

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

[Tom Tromey]

>>>>> ">" == Leszek Swirski via gdb-patches <gdb-patches@sourceware.org> writes:

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

I'm still looking into the problem, but this regressed an internal test
case here at AdaCore.  In particular, this patch doesn't seem to treat
bitfields the same way that gcc does.

>> 	* amd64-tdep.c (amd64_classify_aggregate): Use cp_pass_by_reference
>> 	rather than a hand-rolled POD check when checking for forced MEMORY
>> 	classification.

This mentions cp_pass_by_reference but the patch doesn't actually
introduce a call to this function.

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

This area seems to differ between gcc and gdb as well.  The psABI
mentions using 8 eightbytes here, but then has a complicated footnote
about the post-merge cleanup, so I'm not certain if gcc and gdb always
agree in practice.

Tom

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

[Tom Tromey]

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

Tom> I'm still looking into the problem, but this regressed an internal test
Tom> case here at AdaCore.  In particular, this patch doesn't seem to treat
Tom> bitfields the same way that gcc does.

The appended fixes the test case that's included, but I'm not really
sure it's correct, since I am not sure that the test for
"bit-field-ness" is correct here.

Let me know what you think.

Tom

commit 93fb3e610bcd0ff5763334588bd84e41a4f73ef9
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Apr 16 11:11:10 2019 -0600

    Fix passing of struct with bitfields on x86-64
    
    Commit 4aa866af ("Fix AMD64 return value ABI in expression
    evaluation") introduced a regression when calling a function with a
    structure that contains bitfields.
    
    Because the caller of amd64_has_unaligned_fields handles bitfields
    already, it seemed to me that the simplest fix was to ignore bitfields
    here.
    
    gdb/ChangeLog
    2019-04-16  Tom Tromey  <tromey@adacore.com>
    
            * amd64-tdep.c (amd64_has_unaligned_fields): Ignore bitfields.
    
    gdb/testsuite/ChangeLog
    2019-04-16  Tom Tromey  <tromey@adacore.com>
    
            * gdb.arch/amd64-eval.exp: Test bitfield return.
            * gdb.arch/amd64-eval.cc (struct Bitfields): New.
            (class Foo) <return_bitfields>: New method.
            (main): Call it.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ba1300d57ef..bc4a4b50ff5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2019-04-16  Tom Tromey  <tromey@adacore.com>
+
+	* amd64-tdep.c (amd64_has_unaligned_fields): Ignore bitfields.
+
 2019-04-15  Leszek Swirski  <leszeks@google.com>
 
 	* amd64-tdep.c (amd64_classify_aggregate): Use cp_pass_by_reference
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index d4c96de5716..31791f9a9f1 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -555,11 +555,13 @@ amd64_has_unaligned_fields (struct type *type)
 	  int bitpos = TYPE_FIELD_BITPOS (type, i);
 	  int align = type_align(subtype);
 
-	  /* Ignore static fields, or empty fields, for example nested
-	     empty structures.  */
+	  /* Ignore static fields, empty fields (for example nested
+	     empty structures), and bitfields (these are handled by
+	     the caller).  */
 	  if (field_is_static (&TYPE_FIELD (type, i))
 	      || (TYPE_FIELD_BITSIZE (type, i) == 0
-		  && TYPE_LENGTH (subtype) == 0))
+		  && TYPE_LENGTH (subtype) == 0)
+	      || TYPE_FIELD_PACKED (type, i))
 	    continue;
 
 	  if (bitpos % 8 != 0)
@@ -569,7 +571,7 @@ amd64_has_unaligned_fields (struct type *type)
 	  if (bytepos % align != 0)
 	    return true;
 
-	  if (amd64_has_unaligned_fields(subtype))
+	  if (amd64_has_unaligned_fields (subtype))
 	    return true;
 	}
     }
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d46422635be..f9cc0259c56 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2019-04-16  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.arch/amd64-eval.exp: Test bitfield return.
+	* gdb.arch/amd64-eval.cc (struct Bitfields): New.
+	(class Foo) <return_bitfields>: New method.
+	(main): Call it.
+
 2019-04-15  Leszek Swirski  <leszeks@google.com>
 
 	* gdb.arch/amd64-eval.cc: New file.
diff --git a/gdb/testsuite/gdb.arch/amd64-eval.cc b/gdb/testsuite/gdb.arch/amd64-eval.cc
index a986a49db34..907233ff04a 100644
--- a/gdb/testsuite/gdb.arch/amd64-eval.cc
+++ b/gdb/testsuite/gdb.arch/amd64-eval.cc
@@ -63,6 +63,16 @@ struct UnalignedFieldsInBase : public UnalignedFields
   int32_t x2;
 };
 
+struct Bitfields
+{
+  Bitfields(unsigned int x, unsigned int y)
+    : fld(x), fld2(y)
+  {}
+
+  unsigned fld : 7;
+  unsigned fld2 : 7;
+};
+
 class Foo
 {
 public:
@@ -101,6 +111,13 @@ public:
     return UnalignedFieldsInBase (x, y, x2);
   }
 
+  Bitfields
+  return_bitfields (unsigned int x, unsigned int y)
+  {
+    assert (this->tag == EXPECTED_TAG);
+    return Bitfields(x, y);
+  }
+
 private:
   /* Use a tag to detect if the "this" value is correct.  */
   static const int EXPECTED_TAG = 0xF00F00F0;
@@ -116,5 +133,6 @@ main (int argc, char *argv[])
   foo.return_non_trivial_destructor(3);
   foo.return_unaligned(4, 5);
   foo.return_unaligned_in_base(6, 7, 8);
+  foo.return_bitfields(23, 74);
   return 0;  // break-here
 }
diff --git a/gdb/testsuite/gdb.arch/amd64-eval.exp b/gdb/testsuite/gdb.arch/amd64-eval.exp
index c33777d2b41..beef46ad133 100644
--- a/gdb/testsuite/gdb.arch/amd64-eval.exp
+++ b/gdb/testsuite/gdb.arch/amd64-eval.exp
@@ -41,3 +41,5 @@ gdb_test "call foo.return_unaligned(78, 9.25)" \
     " = {x = 78, y = 9.25}"
 gdb_test "call foo.return_unaligned_in_base(23, 4.5, 67)" \
     " = {<UnalignedFields> = {x = 23, y = 4.5}, x2 = 67}"
+gdb_test "call foo.return_bitfields(23, 74)" \
+    " = {fld = 23, fld2 = 74}"

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

[Tom de Vries]

On 16-04-19 18:39, Tom Tromey wrote:
>>>>>> ">" == Leszek Swirski via gdb-patches <gdb-patches@sourceware.org> writes:
> 
>>> 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).
> 
> I'm still looking into the problem, but this regressed an internal test
> case here at AdaCore.

For me, this commit regresses like this:
...
FAIL: gdb.base/call-ar-st.exp: print print_small_structs (timeout)
FAIL: gdb.base/call-ar-st.exp: print print_small_structs from
print_long_arg_list (timeout)
FAIL: gdb.base/call-ar-st.exp: print
print_bit_flags_combo(*bit_flags_combo) (timeout)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_char(*cflags)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_char(*cflags)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_short(*sflags)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_short(*sflags)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags(*flags)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags(*flags)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_combo(*flags_combo)
FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_combo(*flags_combo)
...

Filed as PR24463 - "call-ar-st.exp/call-rt-st.exp regressions".

Thanks,
- Tom

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> For me, this commit regresses like this:
Tom> ...
Tom> FAIL: gdb.base/call-ar-st.exp: print print_small_structs (timeout)
Tom> FAIL: gdb.base/call-ar-st.exp: print print_small_structs from
[...]

I can confirm that my patch fixes this.

Tom

Regressions on gdb.base/call-{ar,rt}-st.exp on x86_64 native (was: Re: [PATCH v3] [amd64] Fix AMD64 return value ABI in expression evaluation)

[Sergio Durigan Junior]

On Thursday, February 14 2019, 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.

Hi Leszek,

I found that this patch has caused two testcase regressions when testing
on x86_64 native:

  FAIL: gdb.base/call-ar-st.exp: print print_small_structs (timeout)
  FAIL: gdb.base/call-ar-st.exp: print print_small_structs from print_long_arg_list (timeout)
  FAIL: gdb.base/call-ar-st.exp: print print_bit_flags_combo(*bit_flags_combo) (timeout)

  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_char(*cflags)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_char(*cflags)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_short(*sflags)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_short(*sflags)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags(*flags)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags(*flags)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_combo(*flags_combo)
  FAIL: gdb.base/call-rt-st.exp: print print_bit_flags_combo(*flags_combo)

I haven't investigated further, but it should be easy to reproduce these
failures.  The BuildBot hasn't reached this commit yet, so it doesn't
have logs.  Let me know if you need help reproducing it.

Thanks,

> 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.
> ---
>  gdb/ChangeLog                         |   6 ++
>  gdb/amd64-tdep.c                      |  44 +++++++---
>  gdb/testsuite/ChangeLog               |   5 ++
>  gdb/testsuite/gdb.arch/amd64-eval.cc  | 120 ++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-eval.exp |  43 +++++++++
>  5 files changed, 207 insertions(+), 11 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..d32638a20a 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -541,17 +541,40 @@ 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 true if TYPE is a structure or union with unaligned fields.  */
>  
> -static int
> -amd64_non_pod_p (struct type *type)
> +static bool
> +amd64_has_unaligned_fields (struct type *type)
>  {
> -  /* ??? 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;
> +  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
> +      || TYPE_CODE (type) == TYPE_CODE_UNION)
> +    {
> +      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);
>  
> -  return 0;
> +	  /* Ignore static fields, or empty fields, for example nested
> +	     empty structures.  */
> +	  if (field_is_static (&TYPE_FIELD (type, i))
> +	      || (TYPE_FIELD_BITSIZE (type, i) == 0
> +		  && TYPE_LENGTH (subtype) == 0))
> +	    continue;
> +
> +	  if (bitpos % 8 != 0)
> +	    return true;
> +
> +	  int bytepos = bitpos / 8;
> +	  if (bytepos % align != 0)
> +	    return true;
> +
> +	  if (amd64_has_unaligned_fields(subtype))
> +	    return true;
> +	}
> +    }
> +
> +  return false;
>  }
>  
>  /* Classify TYPE according to the rules for aggregate (structures and
> @@ -560,10 +583,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..a986a49db3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-eval.cc
> @@ -0,0 +1,120 @@
> +/* 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 <cstdint>
> +#include <cstdio>
> +#include <cstdlib>
> +#include <cassert>
> +
> +/* A simple structure with a single integer field. Should be returned in
> +   a register.  */
> +struct SimpleBase
> +{
> +  SimpleBase (int32_t x) : x (x) {}
> +
> +  int32_t x;
> +};
> +
> +/* A simple structure derived from the simple base. Should be returned in
> +   a register.  */
> +struct SimpleDerived : public SimpleBase
> +{
> +  SimpleDerived (int32_t x) : SimpleBase (x) {}
> +};
> +
> +/* A structure derived from the simple base with a non-trivial destructor.
> +   Should be returned on the stack.  */
> +struct NonTrivialDestructorDerived : public SimpleBase
> +{
> +  NonTrivialDestructorDerived (int32_t x) : SimpleBase (x) {}
> +  ~NonTrivialDestructorDerived() { x = 1; }
> +};
> +
> +/* A structure with unaligned fields. Should be returned on the stack.  */
> +struct UnalignedFields
> +{
> +  UnalignedFields (int32_t x, double y) : x (x), y (y) {}
> +
> +  int32_t x;
> +  double y;
> +} __attribute__((packed));
> +
> +/* A structure with unaligned fields in its base class. Should be
> +   returned on the stack.  */
> +struct UnalignedFieldsInBase : public UnalignedFields
> +{
> +  UnalignedFieldsInBase (int32_t x, double y, int32_t x2)
> +  : UnalignedFields (x, y), x2 (x2) {}
> +
> +  int32_t x2;
> +};
> +
> +class Foo
> +{
> +public:
> +  SimpleBase
> +  return_simple_base (int32_t x)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return SimpleBase (x);
> +  }
> +
> +  SimpleDerived
> +  return_simple_derived (int32_t x)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return SimpleDerived (x);
> +  }
> +
> +  NonTrivialDestructorDerived
> +  return_non_trivial_destructor (int32_t x)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return NonTrivialDestructorDerived (x);
> +  }
> +
> +  UnalignedFields
> +  return_unaligned (int32_t x, double y)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return UnalignedFields (x, y);
> +  }
> +
> +  UnalignedFieldsInBase
> +  return_unaligned_in_base (int32_t x, double y, int32_t x2)
> +  {
> +    assert (this->tag == EXPECTED_TAG);
> +    return UnalignedFieldsInBase (x, y, x2);
> +  }
> +
> +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(1);
> +  foo.return_simple_derived(2);
> +  foo.return_non_trivial_destructor(3);
> +  foo.return_unaligned(4, 5);
> +  foo.return_unaligned_in_base(6, 7, 8);
> +  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..c33777d2b4
> --- /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(12)" \
> +    " = {x = 12}"
> +gdb_test "call foo.return_simple_derived(34)" \
> +    " = {<SimpleBase> = {x = 34}, <No data fields>}"
> +gdb_test "call foo.return_non_trivial_destructor(56)" \
> +    " = {<SimpleBase> = {x = 56}, <No data fields>}"
> +gdb_test "call foo.return_unaligned(78, 9.25)" \
> +    " = {x = 78, y = 9.25}"
> +gdb_test "call foo.return_unaligned_in_base(23, 4.5, 67)" \
> +    " = {<UnalignedFields> = {x = 23, y = 4.5}, x2 = 67}"
> -- 
> 2.21.0.rc0.258.g878e2cd30e-goog

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Regressions on gdb.base/call-{ar,rt}-st.exp on x86_64 native

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> I found that this patch has caused two testcase regressions when testing
Sergio> on x86_64 native:

FWIW I sent a patch for this: https://sourceware.org/ml/gdb-patches/2019-04/msg00273.html

It's been a week now so I guess I'll land it.

Tom

Re: Regressions on gdb.base/call-{ar,rt}-st.exp on x86_64 native

[Sergio Durigan Junior]

On Wednesday, April 24 2019, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> I found that this patch has caused two testcase regressions when testing
> Sergio> on x86_64 native:
>
> FWIW I sent a patch for this: https://sourceware.org/ml/gdb-patches/2019-04/msg00273.html
>
> It's been a week now so I guess I'll land it.

Ah, great, I hadn't realized.  Thanks, Tom!

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

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

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> The appended fixes the test case that's included, but I'm not really
Tom> sure it's correct, since I am not sure that the test for
Tom> "bit-field-ness" is correct here.

Tom> Let me know what you think.

I'm checking this in now.
It seems to me that since this fixes the regression and also does not
regress the other new tests that it must be at least an improvement.

Tom