Page MenuHomePhabricator

Preserve the owning module information from DWARF in the synthesized AST
ClosedPublic

Authored by aprantl on Mar 2 2020, 3:54 PM.

Details

Summary

Types that came from a Clang module are nested in DW_TAG_module tags in DWARF. This patch recreates the Clang module hierarchy in LLDB and sets the owning module information accordingly. My primary motivation is to facilitate looking up per-module APINotes for individual declarations, but this likely also has other applications.

The fact that Clang modules are orthogonal to DeclContexts made the implementation in LLDB challenging. In the end I extended lldb_private::CompilerDeclContext to hold a pair of clang::DeclContext * and clang::Module * and made sure to use CompilerDeclContext everywhere in TypeSystemClang. To save memory for all non-Clang typesystems, the extra data for the owning module ID is hidden in the CompilerDeclContext's opaque pointer: If it isn't a raw clang::DeclContext pointer, it is an index into an array of Module ID + DeclContexts.

Diff Detail

Event Timeline

aprantl created this revision.Mar 2 2020, 3:54 PM

Raphael just pointed out to me that since each DeclContext is also a Decl I don't need the machinery to hide the owning module ID in CompilerDeclContext and can just rely on the owning module ID stored before the Decl. That should make this patch a lot shorter.

aprantl updated this revision to Diff 247948.Mar 3 2020, 10:10 AM

Here is an updated version the makes do without the CompilerDeclContext trickery. New is the m_payload field in lldb::Type. This is needed to convey the ownership information to typedefs, which are completed asynchronously.

aprantl updated this revision to Diff 247992.Mar 3 2020, 12:45 PM
aprantl retitled this revision from RFC: Preserve the owning module information from DWARF in the synthesized AST to Preserve the owning module information from DWARF in the synthesized AST.

Now with a more comprehensive testcase and support for deferred completion via DWARFASTParserClang::ParseTypeFromClangModule().

This is good to review in earnest now!

shafik added inline comments.Mar 3 2020, 1:30 PM
lldb/include/lldb/Symbol/CompilerType.h
236

Why not convert this comment style?

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
6965

Do we also need to make sure setModuleOwnershipKind(...) is consistent?

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
71

So we at least expect the last bit to be available, do we expect to add more bits like ObjCClassBit, how many do we expect to have available?

I guess conversely what range do we expect id to fit in?

Does it make sense to document this?

aprantl updated this revision to Diff 248011.Mar 3 2020, 1:34 PM

Add C++ tests.

aprantl updated this revision to Diff 248036.Mar 3 2020, 2:29 PM

Address feedback from Shafik.

labath added a comment.Mar 4 2020, 2:11 AM

This sounds like it could be useful though I can't say I really no much about modules.

I appreciate the effort in splitting this patch up, but I am wondering if we couldn't do this is one more step. Would it be possible to split the TypeSystem changes into a patch of its own (with some TypeSystemClang unit tests), and then have separate a patch where SymbolFileDWARF starts making use of this functionality? I think that would make easier to follow what's going in the two subsystems/plugins as well as reduce some of the noise caused by the interface changes.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
222

Maybe spell out the type here? I have no idea what's the type of this..

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
54–79

Maybe it would be simpler to have a struct full of bitfields and then just memcpy it to/from the uint32_t in the constructor/destructor?

lldb/test/Shell/SymbolFile/DWARF/Inputs/ModuleOwnership/A.h
24 ↗(On Diff #248036)

How about a case where A.h defines a type which references a type from B.h (e.g. contains it as a member variable) ?

lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm
1 ↗(On Diff #248036)

I think this test will fail on windows. At least it did fail for me when I manually forced a windows target here (linux was fine though). I think the problem is that lldb does not know how to open an unlinked COFF file.

I think it would be better to hardcode a triple here instead of using %clang_host.

aprantl updated this revision to Diff 248241.Mar 4 2020, 10:40 AM
aprantl marked 3 inline comments as done.

Address feedback from Pavel.

aprantl updated this revision to Diff 248244.Mar 4 2020, 10:42 AM
aprantl marked an inline comment as done.

Hardcode triple in test

aprantl marked an inline comment as done.Mar 4 2020, 10:45 AM
aprantl marked an inline comment as done.
aprantl added inline comments.
lldb/test/Shell/SymbolFile/DWARF/module-ownership.mm
25 ↗(On Diff #248244)

Note that the *Field* decl still comes from A, but its type (see above) doesn't.

labath added a comment.Mar 5 2020, 1:27 AM

Thanks for splitting this up. This does make things much easier to understand.

I don't have any real objections to this, but I have some "thoughts" in inline comments.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
214–215

Did you deliberately not include the payload as an argument to the Type constructor? I can understand not wanting to add a extra argument to that constructor, but OTOH, having this in the constructor would make it harder to forget setting the module id when creating a type elsewhere. (And it's always nice to have immutable members if it is possible.)

219–235

I don't know how feasible this is, but it has occurred to me that this is basically repeating the same structure iteration that would be done as a part of the import a couple of lines above. If would be nice if the importer could somehow set this property automatically.

teemperor added inline comments.Mar 10 2020, 12:54 PM
lldb/include/lldb/Symbol/Type.h
198

Remove reference?

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
91

Maybe I'm missing something but this is DIEToModuleMap but it seems the value in this map is actually a module ID and not a module map?

aprantl marked an inline comment as done.Mar 13 2020, 9:31 AM
aprantl added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
222

Yeah, this should either be a Visitor or in our ASTImporterDelegate in lldb.

aprantl marked an inline comment as done.Mar 13 2020, 9:36 AM
aprantl added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
222

Another comment from an in-person discussion with @teemperor: We probably should not attach owning module information in the scratch/expression context.

aprantl updated this revision to Diff 250971.Mar 17 2020, 7:56 PM
aprantl marked an inline comment as done.

Address review feedback from Raphael.

aprantl updated this revision to Diff 251116.Mar 18 2020, 9:45 AM

Rolled https://reviews.llvm.org/D75626 back into this patch, addressed more review feedback.

teemperor accepted this revision.Mar 23 2020, 6:54 AM

LGTM

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
302

This probably deserves a hint that this can only be called on an TypeSystemClang that has created its own clang::ASTContext (otherwise the ASTSource could be anything and this asserts) as all other functions in TypeSystemClang otherwise work in both modes. Should not be called when TypeSystemClang adopted an existing clang::ASTContext or something like that.

This revision is now accepted and ready to land.Mar 23 2020, 6:54 AM
aprantl marked an inline comment as done.Mar 25 2020, 10:45 AM
aprantl added inline comments.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1227

@teemperor This is the place where we need Module * instead of a const Module *.

teemperor requested changes to this revision.Mar 31 2020, 10:34 AM

Sorry for resetting this back from accepted :)

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1233

Why is that done? The module should already have the correct name when it is returned from findOrCreateModule (where we pass the same name and then create the new module with that name).

If this line is removed, do we still need D75561?

This revision now requires changes to proceed.Mar 31 2020, 10:34 AM
aprantl updated this revision to Diff 254027.Mar 31 2020, 3:49 PM
aprantl marked an inline comment as done.

(Made const_cast explicit to illustrate mismatch.)

However, I still need to de-constify in order to create the module.

aprantl marked an inline comment as done.Mar 31 2020, 3:50 PM
aprantl added inline comments.
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1229

here ^^

1233

Good catch! This is a leftover from my first version of the patch where I created the Module object manually without instantiating a clang::ModuleMap.

teemperor accepted this revision.Apr 1 2020, 2:28 AM

Ok, no more complaints from my side. LGTM

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
1229

True, it seems we can't fix that. Also the ExternalASTSource uses non-const modules, so I guess that's fine then.

This revision is now accepted and ready to land.Apr 1 2020, 2:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 6:00 PM