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

["aburgess at redhat dot com" <sourceware-bugzilla@sourceware.org>]

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

--- Comment #5 from Andrew Burgess <aburgess at redhat dot com> ---
Sure.  My original comment was all I knew at the time, but I've looked into it
a little more since then.

The call stack when the assert triggers is:

  amd64_return_value
    amd64_classify
      amd64_classify_aggregate
        amd64_has_unaligned_fields

In amd64_has_unaligned_fields we have this loop:

  for (int i = 0; i < type->num_fields (); i++)
    {
      struct type *subtype = check_typedef (type->field (i).type ());

      /* Ignore static fields, empty fields (for example nested
         empty structures), and bitfields (these are handled by
         the caller).  */
      if (field_is_static (&type->field (i))
          || (TYPE_FIELD_BITSIZE (type, i) == 0
              && TYPE_LENGTH (subtype) == 0)
          || TYPE_FIELD_PACKED (type, i))
        continue;

      int bitpos = type->field (i).loc_bitpos ();

      /* ... snip ... */
    }

And it is when we ask for the loc_bitpos that the assert triggers, as the
field's type is no FIELD_LOC_KIND_BITPOS, but is instead,
FIELD_LOC_KIND_DWARF_BLOCK.

In the case we're handling, the field being processed is the field that
describes the virtual base of the type being processed.

I originally starting playing with is_dynamic_type, but noticed that the type
in question doesn't return true for is_dynamic_type.  This seemed weird, as
clearly the offset needs resolving.  But then I found some code in
is_dynamic_type_internal which specifically detects the exact case I'm looking
at, and ensures that such a case is NOT enough to make a type appear dynamic. 
The comment on this code says:

            /* Do not consider C++ virtual base types to be dynamic
               due to the field's offset being dynamic; these are
               handled via other means.  */

But, frustratingly, doesn't indicate what that other means is, or where I might
find it.

However, I might be able to side step this issue for now.

The patch I'm playing with adds a check in amd64_classify_aggregate, if the
aggregate is not trivially_copyable then it should be passed in memory.  No
further checks are required.

I initially added the trivially_copyable check as the last check of the if
condition, 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;
    }

However, while looking at the assertion case, I noticed that the type with a
virtual base was not trivially_copyable, so, if I reorder the checks like this:

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

Then, problem solved!  Or, at least, problem kicked down the road!  I'm still
experimenting, but, so far, I've not been able to create an example that both
uses virtual base classes, and is trivially_copyable.

My plan for now is to place the checks in the "correct" order, and add a solid
comment explaining that the order is important (plus the test will fail if the
order is ever changed).

Things that would help are:

  1. Suggestions for how amd64_has_unaligned_fields can be improved in order to
handle virtual base class fields, e.g. how do we obtain the field offset for
those fields, or

  2. Confirmation that in C++ types with a virtual base class, are, by
definition, never trivially_copyable, this would imply my solution is actually
not such a hack after all, or

  3. An example of a class with a virtual base class, that is trivially
copyable.  Ideally, the class would be less than 8-bytes in size so that it can
be passed in a single register, such an example would then fail in my patched
GDB.

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