[PATCH] gdb: on x86-64 non-trivial C++ objects are returned in memory

[PATCH] gdb: on x86-64 non-trivial C++ objects are returned in memory

[Andrew Burgess]

Fixes PR gdb/28681.  It was observed that after using the `finish`
command an incorrect value was displayed in some cases.  Specifically,
this behaviour was observed on an x86-64 target.

Consider this test program:

  struct A
  {
    int i;

    A ()
    { this->i = 0; }
    A (const A& a)
    { this->i = a.i; }
  };

  A
  func (int i)
  {
    A a;
    a.i = i;
    return a;
  }

  int
  main ()
  {
    A a = func (3);
    return a.i;
  }

And this GDB session:

  $ gdb -q ex.x
  Reading symbols from ex.x...
  (gdb) b func
  Breakpoint 1 at 0x401115: file ex.cc, line 14.
  (gdb) r
  Starting program: /home/andrew/tmp/ex.x

  Breakpoint 1, func (i=3) at ex.cc:14
  14	  A a;
  (gdb) finish
  Run till exit from #0  func (i=3) at ex.cc:14
  main () at ex.cc:23
  23	  return a.i;
  Value returned is $1 = {
    i = -19044
  }
  (gdb) p a
  $2 = {
    i = 3
  }
  (gdb)

Notice how after the `finish` the contents of $1 are junk, but, when I
immediately ask for the value of `a`, I get back the correct value.

The problem here is that after the finish command GDB calls the
function amd64_return_value to figure out where the return value can
be found (on x86-64 targets anyway).

This function makes the wrong choice for the struct A in our case, as
sizeof(A) <= 8, then amd64_return_value decides that A will be
returned in a register.  GDB then reads the return value register an
interprets the contents as an instance of A.

Unfortunately, A is not trivially copyable (due to its copy
constructor), and the sys-v specification for argument and return
value passing, says that any non-trivial C++ object should have space
allocated for it by the caller, and the address of this space is
passed to the callee as a hidden first argument.  The callee should
then return the address of this space as the return value.

And so, the register that GDB is treating as containing an instance of
A, actually contains the address of an instance of A (in this case on
the stack), this is why GDB shows the incorrect result.

The call stack within GDB for where we actually go wrong is this:

  amd64_return_value
    amd64_classify
      amd64_classify_aggregate

And it is in amd64_classify_aggregate that we should be classifying
the type as AMD64_MEMORY, instead of as AMD64_INTEGER as we currently
do (via a call to amd64_classify_aggregate_field).

At the top of amd64_classify_aggregate we already have this logic:

  if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
    {
      theclass[0] = theclass[1] = AMD64_MEMORY;
      return;
    }

Which handles some easy cases where we know a struct will be placed
into memory, that is (a) the struct is more than 16-bytes in size,
or (b) the struct has any unaligned fields.

All we need then, is to add a check here to see if the struct is
trivially copyable.  If it is not then we know the struct will be
passed in memory.

I originally structured the code like this:

  if (TYPE_LENGTH (type) > 16
      || amd64_has_unaligned_fields (type)
      || !language_pass_by_reference (type).trivially_copyable)
    {
      theclass[0] = theclass[1] = AMD64_MEMORY;
      return;
    }

This solved the example from the bug, and my small example above.  So
then I started adding some more extensive tests to the GDB testsuite,
and I ran into a problem.  I hit this error:

  gdbtypes.h:676: internal-error: loc_bitpos: Assertion `m_loc_kind == FIELD_LOC_KIND_BITPOS' failed.

This problem is triggered from:

  amd64_classify_aggregate
    amd64_has_unaligned_fields
      field::loc_bitpos

Inside the unaligned field check we try to get the bit position of
each field.  Unfortunately, in some cases the field location is not
FIELD_LOC_KIND_BITPOS, but is FIELD_LOC_KIND_DWARF_BLOCK.

An example that shows this bug is:

  struct B
  {
    short j;
  };

  struct A : virtual public B
  {
    short i;

    A ()
    { this->i = 0; }
    A (const A& a)
    { this->i = a.i; }
  };

  A
  func (int i)
  {
    A a;
    a.i = i;
    return a;
  }

  int
  main ()
  {
    A a = func (3);
    return a.i;
  }

It is the virtual base class, B, that causes the problem.  The base
class is represented, within GDB, as a field within A.  However, the
location type for this field is a DWARF_BLOCK.

I spent a little time trying to figure out how to convert the
DWARF_BLOCK to a BITPOS, however, I realised that, in this case at
least, conversion is not needed.

The C++ standard says that a class is not trivially copyable if it has
any virtual base classes.  And so, in this case, even if I could
figure out the BITPOS for the virtual base class fields, I know for
sure that I would immediately fail the trivially_copyable check.  So,
lets just reorder the checks in amd64_classify_aggregate to:

  if (TYPE_LENGTH (type) > 16
      || !language_pass_by_reference (type).trivially_copyable
      || amd64_has_unaligned_fields (type))
    {
      theclass[0] = theclass[1] = AMD64_MEMORY;
      return;
    }

Now, if we have a class with virtual bases we will fail quicker, and
avoid the unaligned fields check completely.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28681
---
 gdb/amd64-tdep.c                            | 16 ++++++++++---
 gdb/testsuite/gdb.cp/non-trivial-retval.cc  | 19 ++++++++++++++-
 gdb/testsuite/gdb.cp/non-trivial-retval.exp | 26 +++++++++++++++++++++
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 7c67359678b..4956b1c60a7 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -656,9 +656,19 @@ amd64_classify_aggregate_field (struct type *type, int i,
 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 it has
-	unaligned fields, it has class memory.  */
-  if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
+  /* 1. If the size of an object is larger than two times eight bytes, or
+	it is a non-trivial C++ object, or it has unaligned fields, then it
+	has class memory.
+
+	It is important that the trivially_copyable check is before the
+	unaligned fields check, as C++ classes with virtual base classes
+	will have fields (for the virtual base classes) with non-constant
+	loc_bitpos attributes, which will cause an assert to trigger within
+	the unaligned field check.  As classes with virtual bases are not
+	trivially copyable, checking that first avoids this problem.  */
+  if (TYPE_LENGTH (type) > 16
+      || !language_pass_by_reference (type).trivially_copyable
+      || amd64_has_unaligned_fields (type))
     {
       theclass[0] = theclass[1] = AMD64_MEMORY;
       return;
diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.cc b/gdb/testsuite/gdb.cp/non-trivial-retval.cc
index 833dd420d86..58b2d0a3b10 100644
--- a/gdb/testsuite/gdb.cp/non-trivial-retval.cc
+++ b/gdb/testsuite/gdb.cp/non-trivial-retval.cc
@@ -142,11 +142,28 @@ f4 (int i1, int i2)
   return e;
 }
 
+/* We place a breakpoint on the call to this function.  */
+
+void
+breakpt ()
+{
+}
+
 int
 main (void)
 {
   int i1 = 23;
   int i2 = 100;
 
-  return 0;  /* Break here  */
+  breakpt ();	/* Break here.  */
+
+  /* The copy constructor of A takes a non-const reference, so we can't
+     pass in the temporary returned from f1.  */
+  (void) f1 (i1, i2);
+  B b = f2 (i1, i2);
+  B1 b1 = f22 (i1, i2);
+  C c = f3 (i1, i2);
+  E e = f4 (i1, i2);
+
+  return 0;
 }
diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
index 2c646074309..8d3efc4361f 100644
--- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
+++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
@@ -35,3 +35,29 @@ gdb_test "p f2 (i1, i2)" ".* = {b = 123}"
 gdb_test "p f22 (i1, i2)" ".* = {b1 = 123}"
 gdb_test "p f3 (i1, i2)" ".* = {.* c = 123}"
 gdb_test "p f4 (i1, i2)" ".* = {.* e = 123}"
+
+gdb_breakpoint "f1"
+gdb_breakpoint "f2"
+gdb_breakpoint "f22"
+gdb_breakpoint "f3"
+gdb_breakpoint "f4"
+
+gdb_continue_to_breakpoint "Break in f1"
+gdb_test "finish" " = {a = 123}" \
+    "finish from f1"
+
+gdb_continue_to_breakpoint "Break in f2"
+gdb_test "finish" " = {b = 123}" \
+    "finish from f2"
+
+gdb_continue_to_breakpoint "Break in f22"
+gdb_test "finish" " = {b1 = 123}" \
+    "finish from f22"
+
+gdb_continue_to_breakpoint "Break in f3"
+gdb_test "finish" " = {.* c = 123}" \
+    "finish from f3"
+
+gdb_continue_to_breakpoint "Break in f4"
+gdb_test "finish" " = {.* e = 123}" \
+    "finish from f4"
-- 
2.25.4

Re: [PATCH] gdb: on x86-64 non-trivial C++ objects are returned in memory

[Joel Brobecker]

Hi Andrew,

On Wed, Dec 15, 2021 at 05:38:15PM +0000, Andrew Burgess via Gdb-patches wrote:
> Fixes PR gdb/28681.  It was observed that after using the `finish`
> command an incorrect value was displayed in some cases.  Specifically,
> this behaviour was observed on an x86-64 target.

Thanks very much for the very clear description of the how situation.
This made the review of the patch that much easier. The patch looks
good to me.

> 
> Consider this test program:
> 
>   struct A
>   {
>     int i;
> 
>     A ()
>     { this->i = 0; }
>     A (const A& a)
>     { this->i = a.i; }
>   };
> 
>   A
>   func (int i)
>   {
>     A a;
>     a.i = i;
>     return a;
>   }
> 
>   int
>   main ()
>   {
>     A a = func (3);
>     return a.i;
>   }
> 
> And this GDB session:
> 
>   $ gdb -q ex.x
>   Reading symbols from ex.x...
>   (gdb) b func
>   Breakpoint 1 at 0x401115: file ex.cc, line 14.
>   (gdb) r
>   Starting program: /home/andrew/tmp/ex.x
> 
>   Breakpoint 1, func (i=3) at ex.cc:14
>   14	  A a;
>   (gdb) finish
>   Run till exit from #0  func (i=3) at ex.cc:14
>   main () at ex.cc:23
>   23	  return a.i;
>   Value returned is $1 = {
>     i = -19044
>   }
>   (gdb) p a
>   $2 = {
>     i = 3
>   }
>   (gdb)
> 
> Notice how after the `finish` the contents of $1 are junk, but, when I
> immediately ask for the value of `a`, I get back the correct value.
> 
> The problem here is that after the finish command GDB calls the
> function amd64_return_value to figure out where the return value can
> be found (on x86-64 targets anyway).
> 
> This function makes the wrong choice for the struct A in our case, as
> sizeof(A) <= 8, then amd64_return_value decides that A will be
> returned in a register.  GDB then reads the return value register an
> interprets the contents as an instance of A.
> 
> Unfortunately, A is not trivially copyable (due to its copy
> constructor), and the sys-v specification for argument and return
> value passing, says that any non-trivial C++ object should have space
> allocated for it by the caller, and the address of this space is
> passed to the callee as a hidden first argument.  The callee should
> then return the address of this space as the return value.
> 
> And so, the register that GDB is treating as containing an instance of
> A, actually contains the address of an instance of A (in this case on
> the stack), this is why GDB shows the incorrect result.
> 
> The call stack within GDB for where we actually go wrong is this:
> 
>   amd64_return_value
>     amd64_classify
>       amd64_classify_aggregate
> 
> And it is in amd64_classify_aggregate that we should be classifying
> the type as AMD64_MEMORY, instead of as AMD64_INTEGER as we currently
> do (via a call to amd64_classify_aggregate_field).
> 
> At the top of amd64_classify_aggregate we already have this logic:
> 
>   if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
>     {
>       theclass[0] = theclass[1] = AMD64_MEMORY;
>       return;
>     }
> 
> Which handles some easy cases where we know a struct will be placed
> into memory, that is (a) the struct is more than 16-bytes in size,
> or (b) the struct has any unaligned fields.
> 
> All we need then, is to add a check here to see if the struct is
> trivially copyable.  If it is not then we know the struct will be
> passed in memory.
> 
> I originally structured the code like this:
> 
>   if (TYPE_LENGTH (type) > 16
>       || amd64_has_unaligned_fields (type)
>       || !language_pass_by_reference (type).trivially_copyable)
>     {
>       theclass[0] = theclass[1] = AMD64_MEMORY;
>       return;
>     }
> 
> This solved the example from the bug, and my small example above.  So
> then I started adding some more extensive tests to the GDB testsuite,
> and I ran into a problem.  I hit this error:
> 
>   gdbtypes.h:676: internal-error: loc_bitpos: Assertion `m_loc_kind == FIELD_LOC_KIND_BITPOS' failed.
> 
> This problem is triggered from:
> 
>   amd64_classify_aggregate
>     amd64_has_unaligned_fields
>       field::loc_bitpos
> 
> Inside the unaligned field check we try to get the bit position of
> each field.  Unfortunately, in some cases the field location is not
> FIELD_LOC_KIND_BITPOS, but is FIELD_LOC_KIND_DWARF_BLOCK.
> 
> An example that shows this bug is:
> 
>   struct B
>   {
>     short j;
>   };
> 
>   struct A : virtual public B
>   {
>     short i;
> 
>     A ()
>     { this->i = 0; }
>     A (const A& a)
>     { this->i = a.i; }
>   };
> 
>   A
>   func (int i)
>   {
>     A a;
>     a.i = i;
>     return a;
>   }
> 
>   int
>   main ()
>   {
>     A a = func (3);
>     return a.i;
>   }
> 
> It is the virtual base class, B, that causes the problem.  The base
> class is represented, within GDB, as a field within A.  However, the
> location type for this field is a DWARF_BLOCK.
> 
> I spent a little time trying to figure out how to convert the
> DWARF_BLOCK to a BITPOS, however, I realised that, in this case at
> least, conversion is not needed.
> 
> The C++ standard says that a class is not trivially copyable if it has
> any virtual base classes.  And so, in this case, even if I could
> figure out the BITPOS for the virtual base class fields, I know for
> sure that I would immediately fail the trivially_copyable check.  So,
> lets just reorder the checks in amd64_classify_aggregate to:
> 
>   if (TYPE_LENGTH (type) > 16
>       || !language_pass_by_reference (type).trivially_copyable
>       || amd64_has_unaligned_fields (type))
>     {
>       theclass[0] = theclass[1] = AMD64_MEMORY;
>       return;
>     }
> 
> Now, if we have a class with virtual bases we will fail quicker, and
> avoid the unaligned fields check completely.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28681
> ---
>  gdb/amd64-tdep.c                            | 16 ++++++++++---
>  gdb/testsuite/gdb.cp/non-trivial-retval.cc  | 19 ++++++++++++++-
>  gdb/testsuite/gdb.cp/non-trivial-retval.exp | 26 +++++++++++++++++++++
>  3 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 7c67359678b..4956b1c60a7 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -656,9 +656,19 @@ amd64_classify_aggregate_field (struct type *type, int i,
>  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 it has
> -	unaligned fields, it has class memory.  */
> -  if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
> +  /* 1. If the size of an object is larger than two times eight bytes, or
> +	it is a non-trivial C++ object, or it has unaligned fields, then it
> +	has class memory.
> +
> +	It is important that the trivially_copyable check is before the
> +	unaligned fields check, as C++ classes with virtual base classes
> +	will have fields (for the virtual base classes) with non-constant
> +	loc_bitpos attributes, which will cause an assert to trigger within
> +	the unaligned field check.  As classes with virtual bases are not
> +	trivially copyable, checking that first avoids this problem.  */
> +  if (TYPE_LENGTH (type) > 16
> +      || !language_pass_by_reference (type).trivially_copyable
> +      || amd64_has_unaligned_fields (type))
>      {
>        theclass[0] = theclass[1] = AMD64_MEMORY;
>        return;
> diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.cc b/gdb/testsuite/gdb.cp/non-trivial-retval.cc
> index 833dd420d86..58b2d0a3b10 100644
> --- a/gdb/testsuite/gdb.cp/non-trivial-retval.cc
> +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.cc
> @@ -142,11 +142,28 @@ f4 (int i1, int i2)
>    return e;
>  }
>  
> +/* We place a breakpoint on the call to this function.  */
> +
> +void
> +breakpt ()
> +{
> +}
> +
>  int
>  main (void)
>  {
>    int i1 = 23;
>    int i2 = 100;
>  
> -  return 0;  /* Break here  */
> +  breakpt ();	/* Break here.  */
> +
> +  /* The copy constructor of A takes a non-const reference, so we can't
> +     pass in the temporary returned from f1.  */
> +  (void) f1 (i1, i2);
> +  B b = f2 (i1, i2);
> +  B1 b1 = f22 (i1, i2);
> +  C c = f3 (i1, i2);
> +  E e = f4 (i1, i2);
> +
> +  return 0;
>  }
> diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> index 2c646074309..8d3efc4361f 100644
> --- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> @@ -35,3 +35,29 @@ gdb_test "p f2 (i1, i2)" ".* = {b = 123}"
>  gdb_test "p f22 (i1, i2)" ".* = {b1 = 123}"
>  gdb_test "p f3 (i1, i2)" ".* = {.* c = 123}"
>  gdb_test "p f4 (i1, i2)" ".* = {.* e = 123}"
> +
> +gdb_breakpoint "f1"
> +gdb_breakpoint "f2"
> +gdb_breakpoint "f22"
> +gdb_breakpoint "f3"
> +gdb_breakpoint "f4"
> +
> +gdb_continue_to_breakpoint "Break in f1"
> +gdb_test "finish" " = {a = 123}" \
> +    "finish from f1"
> +
> +gdb_continue_to_breakpoint "Break in f2"
> +gdb_test "finish" " = {b = 123}" \
> +    "finish from f2"
> +
> +gdb_continue_to_breakpoint "Break in f22"
> +gdb_test "finish" " = {b1 = 123}" \
> +    "finish from f22"
> +
> +gdb_continue_to_breakpoint "Break in f3"
> +gdb_test "finish" " = {.* c = 123}" \
> +    "finish from f3"
> +
> +gdb_continue_to_breakpoint "Break in f4"
> +gdb_test "finish" " = {.* e = 123}" \
> +    "finish from f4"
> -- 
> 2.25.4
> 

-- 
Joel

Re: [PATCH] gdb: on x86-64 non-trivial C++ objects are returned in memory

[Andrew Burgess]

* Joel Brobecker <brobecker@adacore.com> [2021-12-18 15:33:35 +0400]:

> Hi Andrew,
> 
> On Wed, Dec 15, 2021 at 05:38:15PM +0000, Andrew Burgess via Gdb-patches wrote:
> > Fixes PR gdb/28681.  It was observed that after using the `finish`
> > command an incorrect value was displayed in some cases.  Specifically,
> > this behaviour was observed on an x86-64 target.
> 
> Thanks very much for the very clear description of the how situation.
> This made the review of the patch that much easier. The patch looks
> good to me.

Thanks, I pushed this.

Andrew

> 
> > 
> > Consider this test program:
> > 
> >   struct A
> >   {
> >     int i;
> > 
> >     A ()
> >     { this->i = 0; }
> >     A (const A& a)
> >     { this->i = a.i; }
> >   };
> > 
> >   A
> >   func (int i)
> >   {
> >     A a;
> >     a.i = i;
> >     return a;
> >   }
> > 
> >   int
> >   main ()
> >   {
> >     A a = func (3);
> >     return a.i;
> >   }
> > 
> > And this GDB session:
> > 
> >   $ gdb -q ex.x
> >   Reading symbols from ex.x...
> >   (gdb) b func
> >   Breakpoint 1 at 0x401115: file ex.cc, line 14.
> >   (gdb) r
> >   Starting program: /home/andrew/tmp/ex.x
> > 
> >   Breakpoint 1, func (i=3) at ex.cc:14
> >   14	  A a;
> >   (gdb) finish
> >   Run till exit from #0  func (i=3) at ex.cc:14
> >   main () at ex.cc:23
> >   23	  return a.i;
> >   Value returned is $1 = {
> >     i = -19044
> >   }
> >   (gdb) p a
> >   $2 = {
> >     i = 3
> >   }
> >   (gdb)
> > 
> > Notice how after the `finish` the contents of $1 are junk, but, when I
> > immediately ask for the value of `a`, I get back the correct value.
> > 
> > The problem here is that after the finish command GDB calls the
> > function amd64_return_value to figure out where the return value can
> > be found (on x86-64 targets anyway).
> > 
> > This function makes the wrong choice for the struct A in our case, as
> > sizeof(A) <= 8, then amd64_return_value decides that A will be
> > returned in a register.  GDB then reads the return value register an
> > interprets the contents as an instance of A.
> > 
> > Unfortunately, A is not trivially copyable (due to its copy
> > constructor), and the sys-v specification for argument and return
> > value passing, says that any non-trivial C++ object should have space
> > allocated for it by the caller, and the address of this space is
> > passed to the callee as a hidden first argument.  The callee should
> > then return the address of this space as the return value.
> > 
> > And so, the register that GDB is treating as containing an instance of
> > A, actually contains the address of an instance of A (in this case on
> > the stack), this is why GDB shows the incorrect result.
> > 
> > The call stack within GDB for where we actually go wrong is this:
> > 
> >   amd64_return_value
> >     amd64_classify
> >       amd64_classify_aggregate
> > 
> > And it is in amd64_classify_aggregate that we should be classifying
> > the type as AMD64_MEMORY, instead of as AMD64_INTEGER as we currently
> > do (via a call to amd64_classify_aggregate_field).
> > 
> > At the top of amd64_classify_aggregate we already have this logic:
> > 
> >   if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
> >     {
> >       theclass[0] = theclass[1] = AMD64_MEMORY;
> >       return;
> >     }
> > 
> > Which handles some easy cases where we know a struct will be placed
> > into memory, that is (a) the struct is more than 16-bytes in size,
> > or (b) the struct has any unaligned fields.
> > 
> > All we need then, is to add a check here to see if the struct is
> > trivially copyable.  If it is not then we know the struct will be
> > passed in memory.
> > 
> > I originally structured the code like this:
> > 
> >   if (TYPE_LENGTH (type) > 16
> >       || amd64_has_unaligned_fields (type)
> >       || !language_pass_by_reference (type).trivially_copyable)
> >     {
> >       theclass[0] = theclass[1] = AMD64_MEMORY;
> >       return;
> >     }
> > 
> > This solved the example from the bug, and my small example above.  So
> > then I started adding some more extensive tests to the GDB testsuite,
> > and I ran into a problem.  I hit this error:
> > 
> >   gdbtypes.h:676: internal-error: loc_bitpos: Assertion `m_loc_kind == FIELD_LOC_KIND_BITPOS' failed.
> > 
> > This problem is triggered from:
> > 
> >   amd64_classify_aggregate
> >     amd64_has_unaligned_fields
> >       field::loc_bitpos
> > 
> > Inside the unaligned field check we try to get the bit position of
> > each field.  Unfortunately, in some cases the field location is not
> > FIELD_LOC_KIND_BITPOS, but is FIELD_LOC_KIND_DWARF_BLOCK.
> > 
> > An example that shows this bug is:
> > 
> >   struct B
> >   {
> >     short j;
> >   };
> > 
> >   struct A : virtual public B
> >   {
> >     short i;
> > 
> >     A ()
> >     { this->i = 0; }
> >     A (const A& a)
> >     { this->i = a.i; }
> >   };
> > 
> >   A
> >   func (int i)
> >   {
> >     A a;
> >     a.i = i;
> >     return a;
> >   }
> > 
> >   int
> >   main ()
> >   {
> >     A a = func (3);
> >     return a.i;
> >   }
> > 
> > It is the virtual base class, B, that causes the problem.  The base
> > class is represented, within GDB, as a field within A.  However, the
> > location type for this field is a DWARF_BLOCK.
> > 
> > I spent a little time trying to figure out how to convert the
> > DWARF_BLOCK to a BITPOS, however, I realised that, in this case at
> > least, conversion is not needed.
> > 
> > The C++ standard says that a class is not trivially copyable if it has
> > any virtual base classes.  And so, in this case, even if I could
> > figure out the BITPOS for the virtual base class fields, I know for
> > sure that I would immediately fail the trivially_copyable check.  So,
> > lets just reorder the checks in amd64_classify_aggregate to:
> > 
> >   if (TYPE_LENGTH (type) > 16
> >       || !language_pass_by_reference (type).trivially_copyable
> >       || amd64_has_unaligned_fields (type))
> >     {
> >       theclass[0] = theclass[1] = AMD64_MEMORY;
> >       return;
> >     }
> > 
> > Now, if we have a class with virtual bases we will fail quicker, and
> > avoid the unaligned fields check completely.
> > 
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28681
> > ---
> >  gdb/amd64-tdep.c                            | 16 ++++++++++---
> >  gdb/testsuite/gdb.cp/non-trivial-retval.cc  | 19 ++++++++++++++-
> >  gdb/testsuite/gdb.cp/non-trivial-retval.exp | 26 +++++++++++++++++++++
> >  3 files changed, 57 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> > index 7c67359678b..4956b1c60a7 100644
> > --- a/gdb/amd64-tdep.c
> > +++ b/gdb/amd64-tdep.c
> > @@ -656,9 +656,19 @@ amd64_classify_aggregate_field (struct type *type, int i,
> >  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 it has
> > -	unaligned fields, it has class memory.  */
> > -  if (TYPE_LENGTH (type) > 16 || amd64_has_unaligned_fields (type))
> > +  /* 1. If the size of an object is larger than two times eight bytes, or
> > +	it is a non-trivial C++ object, or it has unaligned fields, then it
> > +	has class memory.
> > +
> > +	It is important that the trivially_copyable check is before the
> > +	unaligned fields check, as C++ classes with virtual base classes
> > +	will have fields (for the virtual base classes) with non-constant
> > +	loc_bitpos attributes, which will cause an assert to trigger within
> > +	the unaligned field check.  As classes with virtual bases are not
> > +	trivially copyable, checking that first avoids this problem.  */
> > +  if (TYPE_LENGTH (type) > 16
> > +      || !language_pass_by_reference (type).trivially_copyable
> > +      || amd64_has_unaligned_fields (type))
> >      {
> >        theclass[0] = theclass[1] = AMD64_MEMORY;
> >        return;
> > diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.cc b/gdb/testsuite/gdb.cp/non-trivial-retval.cc
> > index 833dd420d86..58b2d0a3b10 100644
> > --- a/gdb/testsuite/gdb.cp/non-trivial-retval.cc
> > +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.cc
> > @@ -142,11 +142,28 @@ f4 (int i1, int i2)
> >    return e;
> >  }
> >  
> > +/* We place a breakpoint on the call to this function.  */
> > +
> > +void
> > +breakpt ()
> > +{
> > +}
> > +
> >  int
> >  main (void)
> >  {
> >    int i1 = 23;
> >    int i2 = 100;
> >  
> > -  return 0;  /* Break here  */
> > +  breakpt ();	/* Break here.  */
> > +
> > +  /* The copy constructor of A takes a non-const reference, so we can't
> > +     pass in the temporary returned from f1.  */
> > +  (void) f1 (i1, i2);
> > +  B b = f2 (i1, i2);
> > +  B1 b1 = f22 (i1, i2);
> > +  C c = f3 (i1, i2);
> > +  E e = f4 (i1, i2);
> > +
> > +  return 0;
> >  }
> > diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> > index 2c646074309..8d3efc4361f 100644
> > --- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> > +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> > @@ -35,3 +35,29 @@ gdb_test "p f2 (i1, i2)" ".* = {b = 123}"
> >  gdb_test "p f22 (i1, i2)" ".* = {b1 = 123}"
> >  gdb_test "p f3 (i1, i2)" ".* = {.* c = 123}"
> >  gdb_test "p f4 (i1, i2)" ".* = {.* e = 123}"
> > +
> > +gdb_breakpoint "f1"
> > +gdb_breakpoint "f2"
> > +gdb_breakpoint "f22"
> > +gdb_breakpoint "f3"
> > +gdb_breakpoint "f4"
> > +
> > +gdb_continue_to_breakpoint "Break in f1"
> > +gdb_test "finish" " = {a = 123}" \
> > +    "finish from f1"
> > +
> > +gdb_continue_to_breakpoint "Break in f2"
> > +gdb_test "finish" " = {b = 123}" \
> > +    "finish from f2"
> > +
> > +gdb_continue_to_breakpoint "Break in f22"
> > +gdb_test "finish" " = {b1 = 123}" \
> > +    "finish from f22"
> > +
> > +gdb_continue_to_breakpoint "Break in f3"
> > +gdb_test "finish" " = {.* c = 123}" \
> > +    "finish from f3"
> > +
> > +gdb_continue_to_breakpoint "Break in f4"
> > +gdb_test "finish" " = {.* e = 123}" \
> > +    "finish from f4"
> > -- 
> > 2.25.4
> > 
> 
> -- 
> Joel
>