From b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Mon, 13 Dec 2021 16:56:16 +0000 Subject: [PATCH] gdb: on x86-64 non-trivial C++ objects are returned in memory 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.30.2