[Bug gdb/28681] Wrong pretty-printed unique_ptr value when using "finish"

["cvs-commit at gcc dot gnu.org" <sourceware-bugzilla@sourceware.org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=28681

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608

commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Mon Dec 13 16:56:16 2021 +0000

    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

-- 
You are receiving this mail because:
You are on the CC list for the bug.