[cxx-interop] Lazily import members of clang namespaces and records via requests. #38675

zoecarver · 2021-07-28T20:31:45Z

This patch does three things:

It makes namespaces lazy and transparent. What I mean by this is that all members will be loaded lazily. Also, namespaces will always appear to be empty (i.e., have no members). Whenever a member is requested, the specific member must be requested. This prevents us from accidently doing things we shouldn't. Namespaces themselves aren't physical things and shouldn't appear to exist or hold anything.
It makes records lazy. They will now lazily load their members, but will also hold on to the loaded members unlike namespaces. As time goes on I want to move more stuff out of the record visitor but I'll do that in follow up patches.
We now print things in the __ObjC module along with whatever module was requested.

As a meta-note: while there are about the same number of lines added as removed, this code is now a lot simpler. This turned many loops with ad-hoc conditions into one loop in a request. It also composes a bit better: the logic for printing namespaces isn't in the namespace visitor it's in the ASTPrinter. The logic for bailing on fields is in the field visitor not the record visitor. Etc.

As more people start working on C++ interop, I'm hoping to simplify the code more and more so that it's easier to understand the codebase. This also helps me find bugs and understand things better :)

zoecarver · 2021-07-28T20:33:20Z

lib/AST/NameLookup.cpp

-
-    if (!disableAdditionalExtensionLoading) {
-      populateLookupTableEntryFromExtensions(ctx, Table, baseName, decl);
+    if (decl->getClangDecl() &&


In the future, I imagine we'll have a switch here, or a lot of if statements, that dispatch out to not just importing namespaces but also records, etc.

egorzhdan

This looks very promising! I think that with this approach the issues when recursive C++ decl importing produces incorrect results will mostly go away.
I've tried importing a few C++ modules from the test suite with this change, and it worked well for me (I left a couple comments where it didn't – feel free to ignore them if you'd like to focus on the bigger picture for now).

It might be too early for this, but I've also tried importing the C++ standard library, and it crashed on type_traits, here's the stacktrace:

Stacktrace

PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /Users/egorzh/swift/swift/cmake-build-debug/bin/swift-ide-test --print-module --source-filename x -I . --module-to-print=std.iosfwd --enable-cxx-interop -tools-directory /Users/egorzh/swift/build/Ninja-DebugAssert/llvm-macosx-x86_64/bin --sdk /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk
1.	/Users/egorzh/swift/build/Ninja-DebugAssert/llvm-macosx-x86_64/include/c++/v1/type_traits:1364:50: importing 'std::remove_reference'
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  swift-ide-test           0x00000001109975ed llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 61
1  swift-ide-test           0x0000000110997b1b PrintStackTraceSignalHandler(void*) + 27
2  swift-ide-test           0x000000011099599b llvm::sys::RunSignalHandlers() + 123
3  swift-ide-test           0x000000011099973d SignalHandler(int) + 205
4  libsystem_platform.dylib 0x00007fff20544d7d _sigtramp + 29
5  libsystem_platform.dylib 0x00007fbbd4aeb4e0 _sigtramp + 18446743784677599104
6  swift-ide-test           0x0000000107f57935 clang::Redeclarable<clang::TagDecl>::getMostRecentDecl() + 21
7  swift-ide-test           0x0000000107f5771e clang::RecordDecl::getMostRecentDecl() + 30
8  swift-ide-test           0x0000000107f57668 clang::CXXRecordDecl::getMostRecentDecl() + 24
9  swift-ide-test           0x000000010eb37275 clang::CXXRecordDecl::getMostRecentNonInjectedDecl() + 21
10 swift-ide-test           0x000000010eb370d8 clang::ClassTemplateSpecializationDecl::getMostRecentDecl() + 24
11 swift-ide-test           0x000000010dacfb68 clang::RedeclarableTemplateDecl::SpecIterator<clang::ClassTemplateSpecializationDecl, clang::RedeclarableTemplateDecl::SpecEntryTraits<clang::ClassTemplateSpecializationDecl>, clang::ClassTemplateSpecializationDecl>::operator*() const + 40
12 swift-ide-test           0x0000000107ff3b45 (anonymous namespace)::SwiftDeclConverter::VisitClassTemplateDecl(clang::ClassTemplateDecl const*) + 133
13 swift-ide-test           0x0000000107fb12e1 clang::declvisitor::Base<llvm::make_const_ptr, (anonymous namespace)::SwiftDeclConverter, swift::Decl*>::Visit(clang::Decl const*) + 817
14 swift-ide-test           0x0000000107fb06bb swift::ClangImporter::Implementation::importDeclImpl(clang::NamedDecl const*, swift::importer::ImportNameVersion, bool&, bool&) + 331
15 swift-ide-test           0x0000000107fb28d2 swift::ClangImporter::Implementation::importDeclAndCacheImpl(clang::NamedDecl const*, swift::importer::ImportNameVersion, bool, bool) + 578
16 swift-ide-test           0x0000000107f28737 swift::ClangImporter::Implementation::importDecl(clang::NamedDecl const*, swift::importer::ImportNameVersion, bool) + 71
17 swift-ide-test           0x0000000107f29333 swift::ClangModuleUnit::getTopLevelDecls(llvm::SmallVectorImpl<swift::Decl*>&) const + 851
18 swift-ide-test           0x0000000107f29d64 swift::ClangModuleUnit::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) const + 68
19 swift-ide-test           0x00000001052f50e0 swift::ModuleDecl::getDisplayDecls(llvm::SmallVectorImpl<swift::Decl*>&) const + 112
20 swift-ide-test           0x00000001058ec952 swift::ide::printModuleInterface(swift::ModuleDecl*, llvm::ArrayRef<llvm::StringRef>, swift::OptionSet<swift::ide::ModuleTraversal, unsigned int>, swift::ASTPrinter&, swift::PrintOptions const&, bool) + 258
21 swift-ide-test           0x000000010496e215 doPrintModules(swift::CompilerInvocation const&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >, swift::OptionSet<swift::ide::ModuleTraversal, unsigned int>, swift::PrintOptions const&, bool, bool) + 1301
22 swift-ide-test           0x0000000104966f54 main + 10404
23 libdyld.dylib            0x00007fff2051af5d start + 1
24 libdyld.dylib            0x000000000000000c start + 18446603339973939376

This might be caused by the fact that there's a place in the importer where we still add members to the namespace enums directly, that would probably need to switch to the new ClangDirectLookupRequest API that you've added:

https://github.com/apple/swift/blob/d61d9238e706b75bdf02077eaa229e6abea3e104/lib/ClangImporter/ImportDecl.cpp#L4550-L4559

The Module Printer will now print decls in the __ObjC module as well.

This sounds good to me, although I think it's important to make sure to prevent decls from dependent modules leaking into the generated module interfaces via the __ObjC module (it would probably be nice to have a test for this in the future).

lib/IDE/ModuleInterfacePrinting.cpp

egorzhdan · 2021-07-31T12:22:28Z

include/swift/AST/NameLookupRequests.h

+    : public SimpleRequest<ClangDirectLookupRequest,
+                           SmallVector<SingleEntry, 4>(
+                               ClangDirectLookupDescriptor),
+                           RequestFlags::Uncached> {


Do you think this could be SeparatelyCached instead of Uncached, since the results of the request are cached by ClangImporter?
I don't know if there would be any real benefits from it, so I don't have a strong opinion here.

Maybe Doug or someone more familiar with the request evaluator can correct me here, but I think that SeparatelyCached still requires you to give it the caching logic, which isn't what we want (at least not yet). Right now, I don't think the request evaluator should reach into the importer and mess its cache. In the future, that might be a good thing to do, so that we get some more benefits of the request evaluator, but right now I'm worried this would break things.

lib/AST/ASTPrinter.cpp

egorzhdan · 2021-07-31T14:04:21Z

lib/ClangImporter/ClangImporter.cpp

+// REVIEW-QUESTION: does it make sense for this to be its own request? It's
+// modifying global state via the importDeclDirectly call, so that's why I made
+// it a request. If this is the correct approach, I'll pull more of the
+// ImportDecl visitors into their own requests, like I do here for namespaces.


This makes perfect sense to me, although I'm not super familiar with the Request API design. If I'm understanding it correctly, the ideal state would be to eventually migrate all of the ClangImporter to Requests, right? I that case, I think it makes sense to have these visitor invocations as separate requests.

Fantastic. I'm glad we agree :)

I think this design might work really well. In the future I think the clang importer could get a lot of benefit from being requestified. Not only for performance and correctness, but it will also make the importing logic more self contained and the crashes easier to debug.

zoecarver · 2021-08-10T22:06:10Z

Making this a draft again. I ran into some problems that would be solved by making records lazily imported as well, so I'm just going to do that as part of this patch.

zoecarver · 2021-08-18T03:56:09Z

lib/ClangImporter/ImportDecl.cpp

@@ -3524,11 +3421,6 @@ namespace {
      SmallVector<VarDecl *, 4> members;


In the future we'll be able to refactor and remove a lot of this visitor. But I want to do that in another nfc.

I think we will still have to import every member here unless we can make hasUnreferenceableStorage lazy, which is unfortunate. But we should be able to remove a lot of the rest of this logic. I'm hoping to move the logic for synthesizing members (such as for unnamed unions) into those member's visitors. That should also help reduce what needs to be done when only importing the class itself.

zoecarver · 2021-08-18T04:00:10Z

This still isn't quite ready to be a PR yet. I need to clean up the tests a bit first. But the logic should be ready for review if you want to take another look @egorzhdan.

Edit: also I checked and this no longer crashes when importing the stdlib :) Thanks for catching that Egor!

zoecarver · 2021-08-18T04:16:23Z

@swift-ci please smoke test.

egorzhdan

I think this change looks much better now! I haven't seen any crashes so far.

One thing I've noticed is that this code no longer compiles:

import std

extension std.__1.string {
}

The error is:

error: 'string' is not a member type of enum '__ObjC.std.__1'
extension std.__1.string {
          ~~~~~~~ ^
__ObjC.std:2:17: note: '__1' declared here
    public enum __1 {
                ^

One more issue is that the module interface printer seems to be printing namespace contents from other submodules. This is visible, for example, on std submodules & on one of the tests – test/Interop/Cxx/namespace/Inputs/submodule-a.h & module Submodules.SubmoduleA. The module interface is printed as:

enum NS1 {
  enum NS2 {
    struct BasicDeepA {
      init()
    }
    struct BasicDeepB {
      init()
    }
  }
  struct BasicB {
    init()
  }
  struct BasicA {
    init()
  }
}

Here BasicB & BasicDeepB shouldn't be printed since they belong to a different submodule (Submodules.SubmoduleB).

zoecarver · 2021-08-30T22:47:04Z

@swift-ci please smoke test.

zoecarver · 2021-09-01T01:24:20Z

@swift-ci please smoke test.

zoecarver · 2021-09-03T01:33:44Z

@swift-ci please smoke test.

zoecarver · 2021-09-03T21:28:45Z

test/api-digester/Outputs/clang-module-dump.txt

@@ -568,6 +568,77 @@
          "usr": "c:objc(pl)NSObject"
        }
      ]
+    },


Review note: this change is here because we now print the __ObjC module.

zoecarver · 2021-09-03T21:29:02Z

@swift-ci please smoke test.

Don't assume that struct fields will be in the same order in C++ and Swift. Look through all the Swift fields to find the matching C++ field. This allows us to lazily import fields in any order. This is a change broken out of swiftlang#38675.

This is one change from swiftlang#38675.

…XNamespaceMemberLookup`, and `ClangRecordMemberLookup`. None of these requests are used, so this is a non-functional change.

zoecarver · 2021-10-14T20:37:23Z

@swift-ci please smoke test.

zoecarver · 2021-10-14T20:40:46Z

Friendly ping @guitard0g @egorzhdan @beccadax. I'd love a final review on this if you have time :)

beccadax

I didn't do a really deep review, but everything I noticed seemed pretty minor.

lib/AST/NameLookup.cpp

lib/AST/ASTPrinter.cpp

beccadax · 2021-10-14T21:14:52Z

lib/ClangImporter/ImportDecl.cpp

-          if (!nd)
-            continue;
+      ImportedName importedName;
+      std::tie(importedName, std::ignore) = importFullName(decl);


Is there no possibility of the correctSwiftName being present or important? If so, why is that?

I'm just copying this from above. I suppose there's no reason we shouldn't use it, though. I'll look into that as a follow up commit.

CC @egorzhdan thoughts?

I don't think there's a specific reason to ignore it, it's just that compatibility wasn't considered here before.

lib/AST/NameLookup.cpp

zoecarver · 2021-10-14T21:32:05Z

@beccadax Thank you for the speedy review! I'll apply those comments after the CI passes (or doesn't).

zoecarver · 2021-10-14T21:32:46Z

@swift-ci please test Windows.

zoecarver · 2021-10-14T21:33:02Z

@swift-ci please test Windows platform.

zoecarver · 2021-10-14T21:33:12Z

@swift-ci please test Windows

zoecarver · 2021-10-15T16:21:31Z

@swift-ci please smoke test.

zoecarver · 2021-10-15T16:22:07Z

Refs swiftlang/llvm-project/pull/3423 which fixed the bots (hopefully!)

zoecarver · 2021-10-15T16:55:27Z

@swift-ci please test Windows

egorzhdan

This looks good!

egorzhdan · 2021-10-17T15:34:23Z

test/Interop/Cxx/operators/member-inline.swift

@@ -185,4 +185,13 @@ OperatorsTestSuite.test("PtrToPtr.subscript (inline)") {
  expectEqual(23, arr[0]![0]![0])
 }

+// TODO: this causes a crash (does it also crash on main?)


Yes, it crashes on main too, it's probably a separate issue.

Good to know, thanks for looking into this :)

zoecarver · 2021-10-19T23:37:09Z

@swift-ci please smoke test.

zoecarver · 2021-10-19T23:39:20Z

Thanks for the review, Egor!

…ia requests. Also adds a ClangImporter request zone and move some requests into it.

zoecarver · 2021-10-20T21:53:18Z

@swift-ci please smoke test.

zoecarver · 2021-10-21T04:15:15Z

MacOS is passing!!!

@swift-ci please test Windows

zoecarver · 2021-10-21T05:39:21Z

@swift-ci please test Windows.

zoecarver · 2021-10-21T05:39:43Z

@swift-ci please test Windows

zoecarver requested review from egorzhdan and DougGregor July 28, 2021 20:31

zoecarver added the c++ interop Feature: Interoperability with C++ label Jul 28, 2021

zoecarver commented Jul 28, 2021

View reviewed changes

zoecarver changed the title ~~[cxx-interop] Let's re-write the logic for importing namespaces again!~~ [cxx-interop] Experiment with making the Clang Importer lazier. Jul 28, 2021

zoecarver changed the title ~~[cxx-interop] Experiment with making the Clang Importer lazier.~~ [cxx-interop] Experiment with making the Clang Importer lazier (requestify importing namespaces). Jul 28, 2021

egorzhdan reviewed Jul 31, 2021

View reviewed changes

zoecarver force-pushed the lazy-importer-namespaces branch from 84d26b8 to 62aa56c Compare August 10, 2021 16:40

zoecarver marked this pull request as ready for review August 10, 2021 16:40

zoecarver marked this pull request as draft August 10, 2021 22:05

zoecarver force-pushed the lazy-importer-namespaces branch from 62aa56c to b4605fd Compare August 18, 2021 03:51

zoecarver changed the title ~~[cxx-interop] Experiment with making the Clang Importer lazier (requestify importing namespaces).~~ [cxx-interop] Lazily import members of clang namespaces and records via requests. Aug 18, 2021

zoecarver commented Aug 18, 2021

View reviewed changes

zoecarver force-pushed the lazy-importer-namespaces branch from b4605fd to 1621cf3 Compare August 18, 2021 04:10

egorzhdan self-requested a review August 18, 2021 17:53

egorzhdan reviewed Aug 19, 2021

View reviewed changes

zoecarver force-pushed the lazy-importer-namespaces branch 2 times, most recently from d2d16c4 to 30fee7a Compare August 30, 2021 22:46

zoecarver marked this pull request as ready for review August 31, 2021 00:20

zoecarver force-pushed the lazy-importer-namespaces branch from 765d5e2 to a53a2c3 Compare September 3, 2021 01:31

zoecarver force-pushed the lazy-importer-namespaces branch from a53a2c3 to 2f5a550 Compare September 3, 2021 21:27

zoecarver commented Sep 3, 2021

View reviewed changes

zoecarver force-pushed the lazy-importer-namespaces branch from 2f5a550 to 48f1b7e Compare September 4, 2021 00:13

zoecarver added a commit to zoecarver/swift that referenced this pull request Sep 30, 2021

[cxx-interop] Print decls in the __ObjC module.

c919aad

This is one change from swiftlang#38675.

zoecarver force-pushed the lazy-importer-namespaces branch from 6f4bbdb to 2720ad6 Compare September 30, 2021 18:16

zoecarver mentioned this pull request Oct 3, 2021

C++ Interop: add a test for std module interface #39562

Merged

[nfc][cxx-interop] Add three requests ClangDirectLookupRequest, `CX…

0ca8dd3

…XNamespaceMemberLookup`, and `ClangRecordMemberLookup`. None of these requests are used, so this is a non-functional change.

zoecarver force-pushed the lazy-importer-namespaces branch 2 times, most recently from c2a82e6 to e2a2339 Compare October 14, 2021 20:37

beccadax approved these changes Oct 14, 2021

View reviewed changes

zoecarver force-pushed the lazy-importer-namespaces branch from e2a2339 to 1ad1bc9 Compare October 15, 2021 16:21

egorzhdan approved these changes Oct 17, 2021

View reviewed changes

zoecarver force-pushed the lazy-importer-namespaces branch from 1ad1bc9 to 8362a27 Compare October 19, 2021 23:35

[cxx-interop] Lazily import members of Clang namespaces and records v…

b8e52a7

…ia requests. Also adds a ClangImporter request zone and move some requests into it.

zoecarver force-pushed the lazy-importer-namespaces branch from 8362a27 to b8e52a7 Compare October 20, 2021 21:53

zoecarver merged commit 401f334 into swiftlang:main Oct 21, 2021

drodriguez mentioned this pull request Nov 24, 2021

[DO NOT MERGE] Test, workaround for compiler crash #40288

Closed

		@@ -3524,11 +3421,6 @@ namespace {
		SmallVector<VarDecl *, 4> members;

[cxx-interop] Lazily import members of clang namespaces and records via requests. #38675

[cxx-interop] Lazily import members of clang namespaces and records via requests. #38675

Uh oh!

Conversation

zoecarver commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorzhdan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoecarver commented Aug 18, 2021

Uh oh!

egorzhdan left a comment

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Aug 30, 2021

Uh oh!

zoecarver commented Sep 1, 2021

Uh oh!

zoecarver commented Sep 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Sep 3, 2021

Uh oh!

zoecarver commented Oct 14, 2021

Uh oh!

zoecarver commented Oct 14, 2021

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beccadax Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

zoecarver commented Oct 14, 2021

Uh oh!

zoecarver commented Oct 14, 2021

Uh oh!

zoecarver commented Oct 14, 2021

Uh oh!

zoecarver commented Oct 14, 2021

Uh oh!

zoecarver commented Oct 15, 2021

Uh oh!

zoecarver commented Oct 15, 2021

Uh oh!

zoecarver commented Oct 15, 2021

Uh oh!

egorzhdan left a comment

Choose a reason for hiding this comment

zoecarver commented Jul 28, 2021 •

edited

Loading

zoecarver commented Aug 18, 2021 •

edited

Loading

beccadax Oct 14, 2021 •

edited

Loading