Introduce a DWARFImporter delegate that can look up clang::Decls by name #26493

adrian-prantl · 2019-08-05T20:19:54Z

Traditionally a serialized binary Swift module (as used in debug info)
can only be imported if all of its Clang dependencies can be imported
from source.

Swift's ClangImporter imports Clang modules by converting Clang AST
types into Swift AST types.
LLDB knows how to find Clang types in DWARF or other debug info and
can synthesize a Clang AST from that information.

This patch introduces a DWARFImporter delegate that is implemented by
LLDB to connect these two components. With this, a Clang type can be
found (by name) in the debug info and handed over to ClangImporter to
create a Swift type from it. This path has lower fidelity than
importing the Clang modules from source, since it is missing out on
Swiftication annotations and other metadata that is not serialized in
DWARF, but it's invaluable as a fallback mechanism for the debugger
when source code for the Clang modules isn't available or the modules
are otherwise not buildable.

rdar://problem/49233932

adrian-prantl · 2019-08-05T20:21:25Z

test with apple/swift-lldb#1848
@swift-ci test

swift-ci · 2019-08-05T20:52:14Z

Build failed
Swift Test Linux Platform
Git Sha - 93ddfdb7248bcee2c4081f015a1150816316b19e

adrian-prantl · 2019-08-05T22:53:24Z

test with apple/swift-lldb#1848
@swift-ci smoke test

adrian-prantl · 2019-08-06T00:48:55Z

test with apple/swift-lldb#1848
@swift-ci smoke test

adrian-prantl · 2019-08-06T18:13:40Z

test with apple/swift-lldb#1848
@swift-ci smoke test

adrian-prantl · 2019-08-06T19:29:33Z

test with apple/swift-lldb#1848
@swift-ci smoke test

adrian-prantl · 2019-08-07T00:18:46Z

test with apple/swift-lldb#1848
@swift-ci smoke test

…ame. Traditionally a serialized binary Swift module (as used in debug info) can only be imported if all of its Clang dependencies can be imported *from source*. - Swift's ClangImporter imports Clang modules by converting Clang AST types into Swift AST types. - LLDB knows how to find Clang types in DWARF or other debug info and can synthesize a Clang AST from that information. This patch introduces a DWARFImporter delegate that is implemented by LLDB to connect these two components. With this, a Clang type can be found (by name) in the debug info and handed over to ClangImporter to create a Swift type from it. This path has lower fidelity than importing the Clang modules from source, since it is missing out on Swiftication annotations and other metadata that is not serialized in DWARF, but it's invaluable as a fallback mechanism for the debugger when source code for the Clang modules isn't available or the modules are otherwise not buildable. rdar://problem/49233932

adrian-prantl · 2019-08-07T01:08:55Z

test with apple/swift-lldb#1848
@swift-ci smoke test

adrian-prantl · 2019-08-07T15:49:28Z

include/swift/AST/ClangModuleLoader.h

@@ -58,6 +60,30 @@ class ClangModuleLoader : public ModuleLoader {
                                            const DeclContext *overlayDC,
                                            const DeclContext *importedDC) = 0;

+  /// Look for declarations associated with the given name.


These virtual methods are necessary to break a cyclic dependency on Linux.

Can you explain further? I'm not sure why there'd be a cycle.

The problem only existed on Linux, and no amount of duplicating llink libraries in the CMakeLists would make it go away. Part of the problem was that ASTDemangler in libAST.a had an explicit cast and a method call to DWARFImporter and libDWARFImporter.a in turn depends on libAST. I do not understand why this wasn't also a problem with ClangImporter, which effectively does the same thing. If you are concerned about the interface/performance I can try a little harder, but I also think having a common interface in the common base class of ClangImporter and DWARFImporter is actually good.

ASTDemangler definitely should not be calling DWARFImporter directly, and I'm sorry I missed that part. Let's come up with a better way to let DWARFImporter intervene in looking up Clang types.

jrose-apple · 2019-08-13T16:08:49Z

include/swift/AST/ASTContext.h

+  ///
+  /// If there is no Clang module loader, returns a null pointer.
+  /// The loader is owned by the AST context.
+  ClangModuleLoader *getDWARFModuleLoader() const;


Can you get rid of this? The Clang importer is special because the Swift compiler has to use it for other things. It would definitely be bad if we had to start special-casing the DWARF loader throughout the compiler as well.

The only use of this method is in ASTDemangler.cpp in ASTBuilder::findForeignTypeDecl(), which tries the DWARFImporter after the ClangImporter failed. We could changing ASTContext::getClangModuleLoader() to return a list of ClangModuleLoaders sorted by the order in which they should be queried. Would that be better?

jrose-apple · 2019-08-13T16:10:01Z

include/swift/DWARFImporter/DWARFImporter.h

+/// Clang AST to ClangImporter to import the type into Swift.
+class DWARFImporterDelegate {
+public:
+  virtual ~DWARFImporterDelegate() {}


Nitpick: = default allows a tiny bit more optimization in some cases.

jrose-apple · 2019-08-13T16:11:54Z

lib/ClangImporter/ClangImporter.cpp

-ClangImporter::lookupTypeDecl(StringRef rawName, ClangTypeKind kind,
-                              llvm::function_ref<void(TypeDecl*)> receiver) {
+static Optional<ClangTypeKind>
+getClangTypeKindForNodeKind(Demangle::Node::Kind kind) {


I really don't like moving this. Why is it the importer's responsibility to resolve manglings?

This is basically based on a misunderstanding on my end that I since dicovered and I can revert this change. Because of how ClangTypeKind folds both enums and structs into ClangTypeKind::Tag I was afraid the we'd be loosing information that could make the type search in DWARF more accurate. Later I discovered that structs, unions and many enums are all mangled as structs anyway, so this point doesn't hold any more. I'll prepare a patch to undo this.

enum class ClangTypeKind { Typedef, ObjCClass = Typedef, /// Structs, enums, and unions. Tag, ObjCProtocol, }

Looks like we would be loosing the ability to distinguish between typedefs and ObjC classes, but that may not be such a big deal.

jrose-apple · 2019-08-13T16:13:23Z

lib/DWARFImporter/DWARFImporter.cpp

@@ -13,33 +13,37 @@
 #include "swift/DWARFImporter/DWARFImporter.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/Module.h"
+#include "clang/CodeGen/ObjectFilePCHContainerOperations.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "../ClangImporter/ImporterImpl.h"


This is very questionable. ClangImporter::Implementation isn't designed for reuse; the distribution of information between ClangImporter and ClangImporter::Implementation is basically "whatever's most convenient". If we need a common base class, let's factor that out and put it in the include/ folder.

jrose-apple · 2019-08-13T16:15:15Z

include/swift/ClangImporter/ClangImporter.h

@@ -178,9 +179,9 @@ class ClangImporter final : public ClangModuleLoader {
  /// Note that this method does no filtering. If it finds the type in a loaded
  /// module, it returns it. This is intended for use in reflection / debugging
  /// contexts where access is not a problem.
-  void lookupRelatedEntity(StringRef clangName, ClangTypeKind kind,


Why is it okay to drop the "kind" parameter?

Because it was not used by the function.

This was deliberately an interface designed to handle arbitrary mangled names, even though we're not using the kind for anything yet.

jrose-apple · 2019-08-13T19:03:06Z

Now that I see that the DWARF importer is injecting decls into the real Clang importer anyway (which makes sense because you want them all in one clang::ASTContext), maybe a new module loader wasn't ever the way to go. Maybe it'd be better for the Clang importer to have a DebugClient like the ASTContext does.

adrian-prantl · 2019-08-13T20:19:13Z

Now that I see that the DWARF importer is injecting decls into the real Clang importer anyway (which makes sense because you want them all in one clang::ASTContext), maybe a new module loader wasn't ever the way to go. Maybe it'd be better for the Clang importer to have a DebugClient like the ASTContext does.

I think that this is doable. We can move DWARFImporter's mode of pretending it can import anything into a DWARFImporterDelegate callback handled by ClangImporter that only succeeds when a DWARFImporterDelegate is installed (and potentially actually checks whether we have DWARF for that module...). I'll go ahead and try this!

Revert some unnecessary changes from #26493

adrian-prantl · 2019-08-14T17:34:48Z

-> #26648

Turned out fairly clean.

adrian-prantl mentioned this pull request Aug 5, 2019

Implement a swift::DWARFImporterDelegate. apple/swift-lldb#1848

Merged

adrian-prantl requested review from jrose-apple and dcci August 5, 2019 20:22

adrian-prantl requested a review from slavapestov August 5, 2019 22:23

adrian-prantl force-pushed the 49233932 branch from 93ddfdb to afdf7f3 Compare August 6, 2019 00:48

adrian-prantl force-pushed the 49233932 branch from afdf7f3 to fa1f10c Compare August 6, 2019 19:29

adrian-prantl force-pushed the 49233932 branch 2 times, most recently from d58d4a1 to d8accb8 Compare August 6, 2019 23:49

adrian-prantl force-pushed the 49233932 branch from d8accb8 to 9b045d5 Compare August 7, 2019 01:06

adrian-prantl commented Aug 7, 2019

View reviewed changes

adrian-prantl merged commit 206994c into swiftlang:master Aug 7, 2019

jrose-apple reviewed Aug 13, 2019

View reviewed changes

swift-ci pushed a commit that referenced this pull request Aug 13, 2019

Merge pull request #26640 from adrian-prantl/followup

5adac04

Revert some unnecessary changes from #26493

adrian-prantl mentioned this pull request Aug 15, 2019

Make ASTDemamgler.cpp independent from ClangImporter. #26674

Merged

Introduce a DWARFImporter delegate that can look up clang::Decls by name #26493

Introduce a DWARFImporter delegate that can look up clang::Decls by name #26493

Uh oh!

Conversation

adrian-prantl commented Aug 5, 2019

Uh oh!

adrian-prantl commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Aug 5, 2019

Uh oh!

adrian-prantl commented Aug 5, 2019

Uh oh!

adrian-prantl commented Aug 6, 2019

Uh oh!

adrian-prantl commented Aug 6, 2019

Uh oh!

adrian-prantl commented Aug 6, 2019

Uh oh!

adrian-prantl commented Aug 7, 2019

Uh oh!

adrian-prantl commented Aug 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 13, 2019

Uh oh!

adrian-prantl commented Aug 13, 2019

Uh oh!

adrian-prantl commented Aug 14, 2019

Uh oh!

Uh oh!

adrian-prantl commented Aug 5, 2019 •

edited

Loading