[C++ Interop] Cache failed imports. #36747

thomassw66 · 2021-04-04T03:28:35Z

Cached failed import Decl's and modify importDeclCached api to return an optional.

Resolves SR-14137. https://bugs.swift.org/browse/SR-14137

thomassw66 · 2021-04-04T03:28:44Z

@swift-ci Please smoke test

zoecarver · 2021-04-04T16:49:20Z

Hey Thomas! This looks great. I have a few comments on the implementation, but let's run the tests first. IIRC there are some visitors that need to be updated as well. Thanks again for the contribution!

@swift-ci please smoke test.

zoecarver · 2021-04-04T19:05:14Z

It looks like those test failures are unrelated, so I guess I was wrong, the passes are all OK (which is good!).

zoecarver · 2021-04-04T19:07:24Z

lib/ClangImporter/ImporterImpl.h

@@ -418,6 +418,9 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
  /// Mapping of already-imported declarations.
  llvm::DenseMap<std::pair<const clang::Decl *, Version>, Decl *> ImportedDecls;

+  /// Set of failed-imported declarations.
+  llvm::DenseSet<std::pair<const clang::Decl *, Version>> FailedImportedDecls;


So, I think the main question here is whether we want to have both FailedImportedDecls and ImportedDecls or just put everything in ImportedDecls. I think it might be a bit simpler just to have one place where everything cached (especially because it is sometimes checked in the individual visitors).

Sure, I wasn't sure if we wanted to modify ImportedDecls or just importedDeclCached so I opted for importDeclCached initially because it appeared more local (used in ~4 places while ImportedDecls in ~30).

However I suppose we can store a nullptr in the map, and then importDeclCached would return None if not found and a nullptr if found and failed.

lib/ClangImporter/ImportDecl.cpp

thomassw66 · 2021-04-05T02:49:53Z

Is there a way to check which tests are supposed to fail so I can make sure I didn't break any of the new ones?

zoecarver

Thanks again for working on this! This change looks great. The last thing to do is please squash all your commits into one which describes the patch as a whole. (You can use git rebase -i main). Once you force push a new patch I'll test it and land it.

zoecarver · 2021-04-05T03:10:26Z

Is there a way to check which tests are supposed to fail to make sure I didn't break any of the new ones?

There are no tests that are supposed to fail. Unfortunately, some tests still fail. (One of these is the one that was failing last time: the infamous lldb failure, which I think/hope is on someone's to-do list.) If it looks like the failures are unrelated, just re-trigger the CI, and it usually passes the second time.

zoecarver · 2021-04-05T04:03:14Z

@swift-ci please smoke test and merge.

zoecarver · 2021-04-05T04:03:43Z

@swift-ci please smoke test.

zoecarver · 2021-04-05T04:51:01Z

@swift-ci please test Windows.

zoecarver · 2021-04-05T04:51:16Z

@swift-ci please test Windows.

zoecarver · 2021-04-05T04:51:31Z

@swift-ci test Windows platform.

zoecarver · 2021-04-05T05:13:02Z

@swift-ci please smoke test macOS.

zoecarver · 2021-04-05T15:51:32Z

@swift-ci please smoke test macOS.

zoecarver · 2021-04-05T15:51:42Z

@swift-ci test Windows platform.

zoecarver · 2021-04-05T19:01:05Z

Windows, at least, should be fixed now.

@swift-ci test Windows platform.

zoecarver · 2021-04-05T19:06:48Z

@swift-ci please smoke test macOS.

zoecarver · 2021-04-05T20:53:32Z

@thomassw66 I really don't think the LLDB failure is related, but it is odd that the bot has now failed four times in a row with that test failure. Can you try running that test locally and see if it fails?

thomassw66 · 2021-04-06T02:26:17Z

@thomassw66 I really don't think the LLDB failure is related, but it is odd that the bot has now failed four times in a row with that test failure. Can you try running that test locally and see if it fails?

Hey I got my stuff setup up on MacOS, and ran
./llvm-project/llvm/utils/lit/lit.py -s -vv ./build/Ninja-RelWithDebInfoAssert/swift-macosx-x86_64/test-macosx-x86_64/

********************
********************
Failed Tests (1):
  Swift(macosx-x86_64) :: IDE/complete_from_stdlib.swift


Testing Time: 310.60s
  Unsupported      :  216
  Passed           : 7295
  Expectedly Failed:   25
  Failed           :    1

4 warning(s) in tests

But this isn't the LLDB test that is failing. What is the command to run the LLDB test suite?

zoecarver · 2021-04-06T17:42:15Z

@thomassw66 I think you can pass --lldb to the build script and it will run the lldb tests (or/after that you can go into Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/ and run them manually). I don't think the IDE test is related. I'll try the buildbot one more time.

@swift-ci please smoke test macOS.

thomassw66 · 2021-04-06T20:54:02Z

Hmm It doesn't seem to repro locally, but I agree it seems that it wouldn't break CI 5 times if it wasn't actually broken.

./llvm-project/llvm/utils/lit/lit.py -s -vv ./build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/test/ --filter=lang/swift/
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/llvm/utils/lit/lit/discovery.py:233: warning: test suite 'lldb-unit' contained no tests
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find lldb-test in /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/./bin
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/lldb/test/Shell/helper/toolchain.py:126: note: using SDKROOT: '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk'
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/llvm/utils/lit/lit/llvm/config.py:385: note: using clang: /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/llvm-macosx-x86_64/bin/clang
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/lldb/test/Shell/lit.cfg.py:106: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/lldb/test/API/lit.cfg.py:170: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.
Deleting module cache at /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/lldb-test-build.noindex/module-cache-lldb/lldb-api.
Deleting module cache at /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/lldb-test-build.noindex/module-cache-clang/lldb-api.

Testing Time: 142.80s
  Excluded   : 1386
  Unsupported:   13
  Passed     :  235

3 warning(s) in tests

./llvm-project/llvm/utils/lit/lit.py -s -vv ./build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/test/ --filter=lang/swift/tagged_pointer/TestSwiftTaggedPointer.py
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/llvm/utils/lit/lit/discovery.py:233: warning: test suite 'lldb-unit' contained no tests
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/llvm/utils/lit/lit/llvm/subst.py:126: note: Did not find lldb-test in /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/./bin
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/lldb/test/Shell/helper/toolchain.py:126: note: using SDKROOT: '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk'
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/llvm/utils/lit/lit/llvm/config.py:385: note: using clang: /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/llvm-macosx-x86_64/bin/clang
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/lldb/test/Shell/lit.cfg.py:106: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.
lit.py: /Users/thomaswheeler/src/swift-project/llvm-project/lldb/test/API/lit.cfg.py:170: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.
Deleting module cache at /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/lldb-test-build.noindex/module-cache-lldb/lldb-api.
Deleting module cache at /Users/thomaswheeler/src/swift-project/build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/lldb-test-build.noindex/module-cache-clang/lldb-api.

Testing Time: 18.68s
  Excluded: 1633
  Passed  :    1

3 warning(s) in tests

thomassw66 · 2021-04-06T22:45:38Z

Actually sorry scratch that. I've got it repro-ing locally but I had to get the flags right.

 ./llvm-project/llvm/utils/lit/lit.py -vv ./build/Ninja-RelWithDebInfoAssert/lldb-macosx-x86_64/test/ -v --time-tests -v --time-tests '--filter=lang/swift/tagged_pointer/TestSwiftTaggedPointer.py' --param 'dotest-args=--setting symbols.use-swift-clangimporter=false'

However it appears that the issue repros on my branch as well as the main branch.

thomassw66 · 2021-04-07T00:06:46Z

update: Actually it doesn't appear to be repro-ing in the main branch so it looks like this is caused by my change

zoecarver · 2021-04-12T04:04:27Z

@thomassw66 sorry for the slow reply. Thanks for looking into it locally! Are you sure the test ran? Maybe it wasn't supported for some reason? Can you add something like CHECK: somethingthatdoesnotexist to make sure it fails? When I have a bit more time (maybe not until next weekend, though) I can try to take a look on my machine.

In the meantime, I'll run the bots once more, just to rule that out.

@swift-ci please smoke test macOS.

zoecarver · 2021-04-12T04:04:46Z

@swift-ci please smoke test macOS.

zoecarver · 2021-04-20T00:16:54Z

@swift-ci please smoke test.

plotfi · 2021-05-06T06:00:14Z

Is there anything I can do to help here with this PR? Also I was curious, can this caching of failed imports be used to avoid codegening decls at the same location that are dependent?

For instance in #37265 I have a not-so-good work around for an issue I've run into where certain anonymous unions fail to import, but there are FieldDecls/IndirectFieldDecls at the same location that also should not be imported that depend on those failed imports.

Inspired by swiftlang#36747. I thought I'd go a little further. I have found there is quite a lot of usage of dyn_cast_or_null in place of checking for nullptr or using an optional. I think using an llvm::Optional<T> is a lot cleaner.

…ess. As per SR-14137 this caches entries in ImportedDecls even when the import failed. Also have to mention I did this based on Thomas's PR swiftlang#36747. This should help us better handle complex templates and dependant types.

…ess. (#41173) As per SR-14137 this caches entries in ImportedDecls even when the import failed. Also have to mention I did this based on Thomas's PR #36747. This should help us better handle complex templates and dependant types.

thomassw66 changed the title ~~My branch~~ [C++ Interop] Cache failed imports. Apr 4, 2021

zoecarver added the c++ interop Feature: Interoperability with C++ label Apr 4, 2021

zoecarver self-requested a review April 4, 2021 16:49

zoecarver reviewed Apr 4, 2021

View reviewed changes

zoecarver approved these changes Apr 5, 2021

View reviewed changes

Cache failed clang::Decl's so we can avoid reparsing

92fd41c

thomassw66 force-pushed the my-branch branch from 5d1b22b to 92fd41c Compare April 5, 2021 04:02

thomassw66 force-pushed the my-branch branch from 78f8a85 to 92fd41c Compare April 20, 2021 00:02

Cache failed clang::Decl's so we can avoid reparsing

3861220

zoecarver mentioned this pull request May 5, 2021

[cxx-interop] Allows clang importer to visit partially invalid anonymous unions #37265

Closed

plotfi mentioned this pull request Feb 1, 2022

[NFC][ClangImporter] Alter importDecl API to return an optional. #41117

Closed

plotfi mentioned this pull request Feb 3, 2022

[c++-interop] For failed imports in ClangImporter, cache them regardless. #41173

Merged

[C++ Interop] Cache failed imports. #36747

Are you sure you want to change the base?

[C++ Interop] Cache failed imports. #36747

Uh oh!

Conversation

thomassw66 commented Apr 4, 2021

Uh oh!

thomassw66 commented Apr 4, 2021

Uh oh!

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

Uh oh!

Uh oh!

zoecarver commented Apr 4, 2021

Uh oh!

zoecarver Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

thomassw66 Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomassw66 commented Apr 5, 2021

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

zoecarver commented Apr 5, 2021

Uh oh!

thomassw66 commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoecarver commented Apr 6, 2021

Uh oh!

thomassw66 commented Apr 6, 2021

Uh oh!

thomassw66 commented Apr 6, 2021

Uh oh!

thomassw66 commented Apr 7, 2021

Uh oh!

zoecarver commented Apr 12, 2021

Uh oh!

zoecarver commented Apr 12, 2021

Uh oh!

zoecarver commented Apr 20, 2021

Uh oh!

plotfi commented May 6, 2021

Uh oh!

Uh oh!

zoecarver commented Apr 4, 2021 •

edited

Loading

thomassw66 commented Apr 6, 2021 •

edited

Loading