lldb: CXXRecordDecl is incorrectly marked as trivial

[hokein]

Split from the issue #72913.

// CXXRecordDecl emitted by clang is DefinitionData pass_in_registers aggregate standard_layout trivially_copyable literal has_constexpr_non_copy_move_ctor can_const_default_init
// CXXRecordDecl emitted by lldb is DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
struct NonTrivialCtor {
  int a = 0;
};

The synthetic CXXRecordDecl AST node in lldb is incorrectly marked as trivial, this will cause incorrect layout calculation.

Comments from @Michael137

Something I noticed that makes this trickier is that RecordDecl::isTrivial() returns true for bar when using LLDBs AST. Whereas with clang it's actually false (which is correct because bar has a default member initialiser, making the default constructor non-trivial). This is important for mustSkipTailPadding, to determine whether to round up the size to alignment or not. So that's a separate bug that needs fixing first

[llvmbot]

@llvm/issue-subscribers-lldb

Author: Haojian Wu (hokein)

Split from the issue https://github.com//issues/72913.

// CXXRecordDecl emitted by clang is DefinitionData pass_in_registers aggregate standard_layout trivially_copyable literal has_constexpr_non_copy_move_ctor can_const_default_init
// CXXRecordDecl emitted by lldb is DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
struct NonTrivialCtor {
  int a = 0;
};

The synthetic CXXRecordDecl AST node in lldb is incorrectly marked as trivial, this will cause incorrect layout calculation.

Comments from @Michael137

> Something I noticed that makes this trickier is that RecordDecl::isTrivial() returns true for bar when using LLDBs AST. Whereas with clang it's actually false (which is correct because bar has a default member initialiser, making the default constructor non-trivial). This is important for mustSkipTailPadding, to determine whether to round up the size to alignment or not. So that's a separate bug that needs fixing first

[Michael137]

We'll need DWARF to somehow tell us that a structure has a trivial default constructor.

Here are some examples:

// Case 1
struct Foo {
    int mem = 5;
};

// Case 4
struct Foo {
  Foo();
};
Foo::Foo() = default;

// Case 3
struct Foo() {
  Foo() = default;
};

// Case 4
struct Foo {};

In cases 1-2, the default constructor isn't trivial.

For explicitly defaulted constructors we have DW_AT_defaulted attribute which, e.g., takes a value of DW_DEFAULTED_in_class. In the absence of constructor DW_TAG_subprograms, a debugger could potentially infer that a structure has a default constructor. But with what we have, we can't really infer whether the constructors are trivial or not.

Couldn't find anything in DWARF that would be applicable here. Adding a DWARF attribute for this seems very specific, but since this is important for debuggers to get the data layout right, that's motivation enough? @dwblaikie @adrian-prantl

[Michael137]

Thinking some more about it, DWARF does encode the DW_AT_data_member_locations of each field and the DW_AT_byte_size of the record, which should really be the definitive layout that clang should use. The fact that trivial types can be laid out into the padding bits of a base-class is encoded in those offsets. So maybe the layout builder is still doing too much inferring on behalf of external sources.

[dwblaikie]

Yeah - so from a derived class you could probably prove which kind of type the base class is - but if you're presented with only the base class, yeah, there's nothing in DWARF that tells you whether a derived class (say if you wanted to create one in the debugger from scratch) should use the tail padding of this type - so that seems like something else to add to the DWARF spec. Like we added DW_AT_calling_convention for similar reasons in DWARFv5.

But in the interim/for backwards compatibility - would be good to at least use the derived class info to adjust the expectations/properties of the base class, where possible. (does mean a bit of a layering complication where derived class might affect how the base class is built which I'm sure isn't the most elegant thing - and it won't work for every derived class, even - if they need certain alignment, you might not be able to distinguish it from the "don't use tail padding" case (eg: base { int, char }, derived: base {int} - derived will be 12 bytes regardless of whether base is trivial or not)

[dwblaikie]

The thing to put in DWARF might be something like "should use tail padding" rather than "is trivial" - the latter might have a few too many implications? But perhaps triviality is the better thing to encode - if there's other uses for it in consumers, if it's hard for the consumer to derive that property consistently/correctly.

[Michael137]

The thing to put in DWARF might be something like "should use tail padding" rather than "is trivial"

Depending on what this looks like this may help with better supporting [[no_unique_address]], since for non-empty fields it allows folding into tail-padding.