Page MenuHomePhabricator

[lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr
Changes PlannedPublic

Authored by jankratochvil on Apr 24 2021, 9:37 AM.

Details

Summary

libstc++ 11 started using [[no_unique_address]] which crashes LLDB:

echo -e '#include <memory>\nstd::unique_ptr<int> p;'|clang -Wall -g -c -o a.o -x c++ -;lldb -b ./a.o -o 'p p'
lldb: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:742: void (anonymous namespace)::CGRecordLowering::clipTailPadding(): Assertion `Prior->Kind == MemberInfo::Field && "Only storage fields have tail padding!"' failed.

As DWARF does not contain the [[no_unique_address]] attribute the patch adds it to every record member, could it be a problem?
This patch requires: D101236, D103910 and D103966.

Diff Detail

Event Timeline

jankratochvil created this revision.Apr 24 2021, 9:37 AM
jankratochvil added inline comments.Apr 24 2021, 9:47 AM
lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp
103

This is in fact unrelated patch, it could be 3/3 in the series. It was failing on TestDataFormatterStdUniquePtr.py line 41:

self.assertFalse(frame.GetValueForVariablePath("iup.deleter").IsValid())

But whole TestDataFormatterStdUniquePtr.py does not yet PASS even with this patch, there are some unrelated other issues on later testcases.

lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
47 ↗(On Diff #340293)

There is now always a new leaf element:

| | |-IntegerLiteral 0x260af78 <<invalid sloc>> 'int' 5 
| | `-NoUniqueAddressAttr 0x260aff0 <<invalid sloc>> Implicit

As DWARF does not contain the [[no_unique_address]] attribute the patch adds it to every record member, could it be a problem?

Given that no_unique_address changes the layout of data structures ( https://clang.llvm.org/docs/AttributeReference.html#no-unique-address ) adding it to a member that doesn't actually have it specified I believe would break layout of structures (have you tried it on the lldb test suite/experimented with examples? I would expect a lot of tests to fail if I'm understanding this correctly)

If it were correct to interpret every field as having no_unique_address, then we wouldn't need users to write the attribute/wouldn't need the attribute, right?

So I suspect the only way to get the layout right is to add the information to the DWARF in some way (probably an extension form/propose it for standardization)?

(@aprantl ?)

Given that no_unique_address changes the layout of data structures ( https://clang.llvm.org/docs/AttributeReference.html#no-unique-address ) adding it to a member that doesn't actually have it specified I believe would break layout of structures

Not so because LLDB sends FieldOffsets (from DW_AT_data_member_locations) to clang. I believe it should work but then I understand it is a bit bold change. clang is already forced for the DWARF-compliant layout by FieldOffsets.

Before this patch LLDB still sends FieldOffsets to clang but clang wants a larger layout because it does not see the [[no_unique_address]] attribute. And therefore it asserts then because the clang offsets cannot fit the FieldOffsets from DW_AT_data_member_locations.

Thanks for this possible catch but I hope the patch does solve that.

(have you tried it on the lldb test suite/experimented with examples? I would expect a lot of tests to fail if I'm understanding this correctly)

Sure the clang+llvm+lldb testsuites do PASS with this patch. (Otherwise I would not send this patch for approval, at least I would mark it by RFC/WIP which I did not.)

Given that no_unique_address changes the layout of data structures ( https://clang.llvm.org/docs/AttributeReference.html#no-unique-address ) adding it to a member that doesn't actually have it specified I believe would break layout of structures

Not so because LLDB sends FieldOffsets (from DW_AT_data_member_locations) to clang. I believe it should work but then I understand it is a bit bold change. clang is already forced for the DWARF-compliant layout by FieldOffsets.

Before this patch LLDB still sends FieldOffsets to clang but clang wants a larger layout because it does not see the [[no_unique_address]] attribute. And therefore it asserts then because the clang offsets cannot fit the FieldOffsets from DW_AT_data_member_locations.

Thanks for this possible catch but I hope the patch does solve that.

(have you tried it on the lldb test suite/experimented with examples? I would expect a lot of tests to fail if I'm understanding this correctly)

Sure the clang+llvm+lldb testsuites do PASS with this patch. (Otherwise I would not send this patch for approval, at least I would mark it by RFC/WIP which I did not.)

Fair enough - I'd be a bit worried - but if it all seems to work, I don't have any further knowledge to suggest it's invalid given the offsets are derived from the authoritative offsets given in the DWARF.

grep tells me that attribute also influences a bunch of other things in subtle ways (It seems to potentially influence ABIs and Obj-C encodings?) and we don't really know what other things this might influence in the future. So I have the suspicion that putting that attr on all record fields might break something in a subtle way.

In any case, what LLDB is usually doing is these situations is inferring from the existing debug information. I assume by looking at the DW_AT_data_member_locations values we could determine what the no_unique_address fields are in our record (and then only put the attributes on that)? Not sure what all the corner cases are but the ones I checked (e.g., two empty record fields with only one having the attr at start/end/middle) are surprisingly not ambiguous.

grep tells me that attribute also influences a bunch of other things in subtle ways (It seems to potentially influence ABIs and Obj-C encodings?) and we don't really know what other things this might influence in the future. So I have the suspicion that putting that attr on all record fields might break something in a subtle way.

I have a similar feeling but without a proof of anything ...

In any case, what LLDB is usually doing is these situations is inferring from the existing debug information. I assume by looking at the DW_AT_data_member_locations values we could determine what the no_unique_address fields are in our record (and then only put the attributes on that)?

... LLDB can become overcomplicated as if you remove the [[no_unique_address]] from this testcase's class B the only effect on DWARF is for class C:

 0x00000033:   DW_TAG_structure_type
                 DW_AT_calling_convention       (DW_CC_pass_by_value)
                 DW_AT_name     ("C")
-                DW_AT_byte_size        (0x01)
+                DW_AT_byte_size        (0x02)
                 DW_AT_decl_file        ("uniqX.C")
                 DW_AT_decl_line        (5)

 0x0000003c:     DW_TAG_inheritance
                   DW_AT_type   (0x0000004f "B")
                   DW_AT_data_member_location   (0x00)

 0x00000042:     DW_TAG_member
                   DW_AT_name   ("c")
                   DW_AT_type   (0x0000006e "char")
                   DW_AT_decl_file      ("uniqX.C")
                   DW_AT_decl_line      (6)
-                  DW_AT_data_member_location   (0x00)
+                  DW_AT_data_member_location   (0x01)

 0x0000004e:     NULL

As LLDB could be already using class B as CompilerType LLDB will need to create a second/new CompilerType for class B when creating CompilerType for class C.

This comment was removed by teemperor.

Ignore my comments above. I assumed we didn't have a minimal crash reproducer from the comment in the dependent patch.

A bit of history on laying out classes: layout used to be a problem before we were able to give all of the field offsets directly to clang to assist in laying out. The main issues were:

  • #pragma pack directives are not in DWARF so we must use the DW_AT_data_member_location
  • Some DWARF optimizations cause class definitions to be omitted and we can have a class A that inherits from class B but we have no real definition for class B, just a forward declaration. In this case we will create a class A that inherits from a class B that has nothing in it, but the field offsets will ensure we show all other instance variables of class A correctly. Furthermore, we have code that can detect when we have such a case and it can grab the right definition for class B when it is imported into another AST, such as when evaluating an expression. This will only happen if class B is in another shared library from class A, and if we do have debug info for class B.
  • any other keywords or attributes such as [[no_unique_address]] that can affect layout that don't appear in DWARF.

It seems it would be a nice attribute to have add to DWARF in case the current solution affects things adversely.

I will let the expression parser experts comment on the viability of always setting "[[no_unique_address]]" on every field. Seems dangerous to me, but I will defer to expression experts.

shafik added a subscriber: shafik.Apr 26 2021, 6:33 PM

I think @dblaikie original idea of adding a DWARF attribute for this case is the right way to go here. AFAICT this will change the answer to basic questions such as what size a struct is and this will likely lead to confusion from our users who will expect the answers in expression parsing to match what they are seeing elsewhere e.g.:

struct X {
    int i;
    [[no_unique_address]] Empty e;
};
 
struct X2 {
    int i;
    Empty e;
};
 
struct Z {
    char c;
    [[no_unique_address]] Empty e1, e2;
};

struct Z2 {
    char c;
    Empty e1, e2;
};


struct W {
    char c[2];
    [[no_unique_address]] Empty e1, e2;
};

struct W2 {
    char c[2];
    Empty e1, e2;
};


int main()
{
    std::cout << sizeof(Empty) << "\n"
              << "X: " << sizeof(X) << "\n"
              << "X2: " << sizeof(X2) << "\n"
              << "Z: " << sizeof(Z) << "\n"
              << "Z2: " << sizeof(Z2) << "\n"
              << "W: " << sizeof(W) << "\n"
              << "W2: " << sizeof(W2) << "\n";
  
}

See godbolt which shows this result:

1
X: 4
X2: 8
Z: 2
Z2: 3
W: 3
W2: 4
jankratochvil planned changes to this revision.Apr 26 2021, 9:51 PM

AFAICT this will change the answer to basic questions such as what size a struct is and this will likely lead to confusion from our users who will expect the answers in expression parsing to match what they are seeing elsewhere e.g.:

OK, that justifies the new DW_AT_*. I did not notice there is this difference. For my needs the new DW_AT_* thus needs to be implemented also in GCC.

I think @dblaikie original idea of adding a DWARF attribute for this case is the right way to go here. AFAICT this will change the answer to basic questions such as what size a struct is and this will likely lead to confusion from our users who will expect the answers in expression parsing to match what they are seeing elsewhere e.g.:

struct X {
    int i;
    [[no_unique_address]] Empty e;
};
 
struct X2 {
    int i;
    Empty e;
};
 
struct Z {
    char c;
    [[no_unique_address]] Empty e1, e2;
};

struct Z2 {
    char c;
    Empty e1, e2;
};


struct W {
    char c[2];
    [[no_unique_address]] Empty e1, e2;
};

struct W2 {
    char c[2];
    Empty e1, e2;
};


int main()
{
    std::cout << sizeof(Empty) << "\n"
              << "X: " << sizeof(X) << "\n"
              << "X2: " << sizeof(X2) << "\n"
              << "Z: " << sizeof(Z) << "\n"
              << "Z2: " << sizeof(Z2) << "\n"
              << "W: " << sizeof(W) << "\n"
              << "W2: " << sizeof(W2) << "\n";
  
}

See godbolt which shows this result:

1
X: 4
X2: 8
Z: 2
Z2: 3
W: 3
W2: 4

Hmm, but is this still a problem for the particular way lldb does things - where it simultaneiously marks all the fields as no_unique_address (in this patch), but then still tells clang exactly where the fields will go? (and it could tell clang exactly how large the structure is too - from the DWARF)

@jankratochvil - have you been able to use these examples from @shafik to demonstrate problems with this patch when applied to lldb? What sort of problems arise in lldb?

(that said, an extension flag attribute should be fairly low cost to add and nice to explicitly encode this parameter in any case, I think - but I'd like to understand the tradeoffs/justification/etc)

(and it could tell clang exactly how large the structure is too - from the DWARF)

We are actually doing that to my knowledge and return the DW_AT_byte_size value for the record type. The relevant API that LLDB implements to get layout/size info back to Clang is:

bool layoutRecordType(
    const clang::RecordDecl *Record, uint64_t &Size, uint64_t &Alignment,
    llvm::DenseMap<const clang::FieldDecl *, uint64_t> &FieldOffsets,
    llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
        &BaseOffsets,
    llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
        &VirtualBaseOffsets) override;

I think the sizeof part actually works* in this regard as we just return whatever we got from DWARF. I get the correct results for the example above (both with and without this patch). There might be some weirder corner cases that could go wrong but I think the main concern are more complicated situations like in the crash that is fixed here.

FWIW, I took a look at the DWARF standard and I think that is actually something we should already emit in the form of a DW_AT_byte_size 0 attribute at the field? Quote:

If the size of a data member is not the same as the size of the type given for the data member, the data member has either a DW_AT_byte_size or a DW_AT_bit_size attribute whose integer constant value (see Section 2.19) is the amount of storage needed to hold the value of the data member.

I am not a DWARF laywer so maybe I understand that part wrong.

(*I actually found a bug that miscalculated empty structs while testing, but that's unrelated. Patch incoming).

(and it could tell clang exactly how large the structure is too - from the DWARF)

We are actually doing that to my knowledge and return the DW_AT_byte_size value for the record type. The relevant API that LLDB implements to get layout/size info back to Clang is:

bool layoutRecordType(
    const clang::RecordDecl *Record, uint64_t &Size, uint64_t &Alignment,
    llvm::DenseMap<const clang::FieldDecl *, uint64_t> &FieldOffsets,
    llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
        &BaseOffsets,
    llvm::DenseMap<const clang::CXXRecordDecl *, clang::CharUnits>
        &VirtualBaseOffsets) override;

I think the sizeof part actually works* in this regard as we just return whatever we got from DWARF. I get the correct results for the example above (both with and without this patch). There might be some weirder corner cases that could go wrong but I think the main concern are more complicated situations like in the crash that is fixed here.

FWIW, I took a look at the DWARF standard and I think that is actually something we should already emit in the form of a DW_AT_byte_size 0 attribute at the field? Quote:

If the size of a data member is not the same as the size of the type given for the data member, the data member has either a DW_AT_byte_size or a DW_AT_bit_size attribute whose integer constant value (see Section 2.19) is the amount of storage needed to hold the value of the data member.

I am not a DWARF laywer so maybe I understand that part wrong.

(*I actually found a bug that miscalculated empty structs while testing, but that's unrelated. Patch incoming).

Hmm, sounds sort of plausible - but might not be right. The cppreference article on no_unique_address presents some interesting challenges that will be maybe surprising (well, I guess not, given the need to render these situations for empty bases already) - yeah, I guess probably the question to ask is: How are empty bases rendered today? I think the trick is they're rendered not as zero length, but as having a location that overlaps with another field - and having a length of 1 (which is the real length of the "empty" object).

jankratochvil abandoned this revision.May 6 2021, 8:28 AM

IIUC it will get replaced by D101950.
I will just provide some testcase for this case if not already contained therein.

jankratochvil reclaimed this revision.May 6 2021, 8:32 AM

Reopening as D101950 may take some time.

jankratochvil planned changes to this revision.May 6 2021, 8:32 AM

DW_AT_byte_size 0 implementation as suggested by @teemperor.

Herald added a reviewer: shafik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
jankratochvil added inline comments.Jun 7 2021, 10:55 AM
llvm/include/llvm/IR/DebugInfoFlags.def
62 ↗(On Diff #350361)

This would need to be handled better if approved.

jankratochvil edited the summary of this revision. (Show Details)

This patch now requires: D101236, D103910 and D103966.

jankratochvil planned changes to this revision.Jun 15 2021, 12:56 AM