This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][NativePDB] Forcefully complete a record type if it has empty debug info and is required to have complete type.
ClosedPublic

Authored by zequanwu on Sep 16 2022, 12:45 PM.

Details

Summary

It's required in following situations:

  1. As a base class.
  2. As a data member.
  3. As an array element type.

Diff Detail

Event Timeline

zequanwu created this revision.Sep 16 2022, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 12:45 PM
Herald added a subscriber: rnkovacs. · View Herald Transcript
zequanwu requested review of this revision.Sep 16 2022, 12:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 12:45 PM
rnk added inline comments.Sep 16 2022, 2:58 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
681 ↗(On Diff #460854)

Is this what we do for DWARF? The same kind of situation seems like it can arise, where clang requires a type to be complete, but the information is missing, and we still try to proceed with evaluation.

zequanwu added inline comments.Sep 16 2022, 3:44 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
681 ↗(On Diff #460854)
zequanwu planned changes to this revision.Sep 16 2022, 4:00 PM

It seems not correct. The dwarf test expects an error. I should figure out how to reduce that original crash.

labath added inline comments.Sep 19 2022, 4:04 AM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
681 ↗(On Diff #460854)

The trick is to do the forced completion only when the type is used within contexts where the c++ rules require it to be complete. something like class A; A* a; is perfectly legal c++. class A; class B : A {}; is not. You can't do this from within the completion callback. In DWARF code, we check for this when we're parsing the enclosing entity (so, when we're parsing B, we'd check, and if needed, "forcefully complete" the class A.

zequanwu added inline comments.Oct 10 2022, 1:59 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
681 ↗(On Diff #460854)

What does dwarf plugin do in this case that we don't have debug info for class A: class A; class B : A {};? The problem I'm trying to fix is when the base class has no debug info to complete it.

zequanwu added inline comments.Oct 17 2022, 6:18 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
681 ↗(On Diff #460854)

@labath, Do you see anything would go wrong if we forcefully complete a class that has no debug info? It seems fine, for the chrome crash report I was checking.

labath added inline comments.Oct 17 2022, 10:48 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
681 ↗(On Diff #460854)

I don't think anything will blow up, if that's what you're asking. The way I think this will manifest itself is that various opaque types that are supposed to be incomplete (e.g. this is often the case with the FILE type in the C library) will suddenly become complete (and empty). That in turn can have various (more or less subtle) effects on the way these objects can be viewed and manipulated by the user. It's not the end of the world, but given that this forceful completion business is essentially a giant hack, I think it makes sense to limit its scope as much as possible.

zequanwu abandoned this revision.Nov 2 2022, 3:56 PM

It shouldn't reach the code path to complete a class with empty debug info.

labath added a comment.Nov 3 2022, 7:09 AM

It shouldn't reach the code path to complete a class with empty debug info.

How about this?

$ cat a.cc
struct A { int x; A(); }; A::A() : x(47) {}

$ cat b.cc
struct A { int x; A(); };
struct B : A {};

B b;
int main(){}

$ bin\clang a.cc -o a.o -c

$ bin\clang.exe a.o b.cc -g -o b.exe

$ bin\lldb b.exe
(lldb) target create "b.exe"
(lldb) Current executable set to 'b.exe' (x86_64).
(lldb) b main
Breakpoint 1: where = b.exe`main at b.cc:5, address = 0x0000000140007100
(lldb) r
(lldb) Process 14288 launched: 'b.exe' (x86_64)
Process 14288 stopped
* thread #1, stop reason = breakpoint 1.1
    frame #0: 0x00007ff6f37f7100 b.exe`main at b.cc:5
   2    struct B : A {};
   3
   4    B b;
-> 5    int main(){}
(lldb) p b
Assertion failed: DD && "queried property of class with no definition", file clang\include\clang/AST/DeclCXX.h, line 443
zequanwu updated this revision to Diff 473077.Nov 3 2022, 4:20 PM

Update to forcefully complete a record type only if it has empty debug info and is required to have complete type.

I basically copied RequireCompleteType from dwarf plugin. Calling it when the tag type is one of 1. an element type 2. an base class 3. a class member. This is the same way that dwarf plugin is doing.

labath added inline comments.Nov 4 2022, 1:12 AM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
1 ↗(On Diff #473077)

oops?

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
96–98

I suppose this won't hurt, but RequireCompleteType will not actually do anything for method types, right ?

Regarding method types, there is a slightly different problem. C++ rules require that in code like
struct B1 { virtual A1* f(); }; struct B2:B1 { A2* f(); };, the types A1 and A2 must be complete (because that code is only valid if A1 is a base of A2). However, I think that's better left to a separate patch.

159

Are you sure about static member part? Something like struct A; struct B { static A a; }; will compile fine (unlike struct A; struct B { A a; };), so I'd hope that the expression evaluator could handle an incomplete static member. It would be better if p B::a produces something like error: variable has incomplete type 'A' instead of printing an empty struct.

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
26

The A a_array[] case is more interesting, because here the class might be (and probably will be) completed as a part of completing B. It would also be better to use a different incomplete class for each test case, as a class can be completed only once.

zequanwu updated this revision to Diff 473336.Nov 4 2022, 2:10 PM
zequanwu marked an inline comment as done.

Update.

lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
96–98

I suppose this won't hurt, but RequireCompleteType will not actually do anything for method types, right ?

Yeah, it should do nothing. I just replaced all occurrence of CompleteType with RequireCompleteType.

159

Removed.

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
26

Printing A a_array[] will give error: use of undeclared identifier. Because the element type size is 0 and the number of element is calculated by total_size/element_type_size, it won't able to create type in this case. The size of B is 4 bytes, so it's able to print b_array.

zequanwu retitled this revision from [LLDB][NativePDB] Forcefully complete a record type it has incomplete type debug info. to [LLDB][NativePDB] Forcefully complete a record type if it has empty debug info and is required to have complete type..Nov 4 2022, 2:11 PM
zequanwu edited the summary of this revision. (Show Details)
zequanwu marked an inline comment as done.Nov 4 2022, 5:56 PM
zequanwu added inline comments.
lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
96–98

Regarding method types, there is a slightly different problem. C++ rules require that in code like
struct B1 { virtual A1* f(); }; struct B2:B1 { A2* f(); };, the types A1 and A2 must be complete (because that code is only valid if A1 is a base of A2). However, I think that's better left to a separate patch.

I tried this example. It crashed and reminds me of a similar crash stack I seen at https://bugs.chromium.org/p/chromium/issues/detail?id=1359144#c4. I tried to call PdbAstBuilder::RequireCompleteType with the method return type, still not working. Not sure what's missing.

zequanwu updated this revision to Diff 473829.Nov 7 2022, 4:43 PM

Removed PdbAstBuilder::RequireCompleteType from PdbAstBuilder::CreateArrayType as it's not necessary at all.

  1. When the element type info is missing, its size info is missing in pdb, which is 0. Thus we can't calculate the number of element in the array.
  2. When the element type info is present, it will be completed later as needed.
labath added inline comments.Nov 7 2022, 4:46 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
90 ↗(On Diff #473336)

I forgot about this the first time around, but it'd be nice if this code could be shared between the pdb and dwarf plugins. Maybe by putting it into TypeSystemClang ?

lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
21

Maybe you could leave this in (but change the test so that it expects the expression to fail)?

zequanwu updated this revision to Diff 473834.Nov 7 2022, 5:11 PM

Adding a test for incomplete record type as class static member. It returns error, because its type not required to be completed.

Looks good, but I think you missed one comment.

lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.h
90 ↗(On Diff #473336)

Did you perhaps miss this comment/request?

zequanwu updated this revision to Diff 475517.Nov 15 2022, 10:35 AM
zequanwu marked 2 inline comments as done.

Move RequireCompleteType to TypeSystemClang.

rnk added a comment.Nov 15 2022, 1:45 PM

Following the direction of the DWARF plugin sounds good to me.

labath accepted this revision.Nov 16 2022, 5:11 AM
labath added inline comments.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
9815–9818

Please move this comment to the header.

This revision is now accepted and ready to land.Nov 16 2022, 5:11 AM