C++ Interop: import namespaces redecls as separate extensions #37695

egorzhdan · 2021-05-28T19:21:40Z

Previously a namespace declaration was imported along with all of its redeclarations, and their members were added to a single Swift extension. This was problematic when a single namespace is declared in multiple modules – the extension belonged to only one of them.
For an example of this, try printing a module interface for std.string/std.iosfwd – it will be empty, even though the declarations from those modules are actually imported into Swift correctly.

This change makes sure that when we're importing different redeclarations of the same namespace, we're adding them as separate extensions to appropriate modules.

egorzhdan · 2021-05-29T11:44:40Z

@swift-ci please smoke test

zoecarver

Haven't looked at the tests yet, but here are some comments.

Thanks again for the PR!

lib/ClangImporter/ClangImporter.cpp

lib/ClangImporter/ImportDecl.cpp

lib/ClangImporter/ClangImporter.cpp

egorzhdan · 2021-06-08T19:08:42Z

@swift-ci please smoke test

egorzhdan · 2021-06-16T21:49:09Z

@swift-ci please test Windows

egorzhdan · 2021-06-17T08:24:59Z

@swift-ci please smoke test

zoecarver

Sorry for being so slow to review this :/

This patch is really good. So, let's try to get it in pretty soon. I only have a few small comments.

test/Interop/Cxx/namespace/submodules-module-interface.swift

lib/ClangImporter/ClangImporter.cpp

zoecarver · 2021-06-17T15:23:35Z

Looks like macos is having an unrelated problem. Let's try again.

@swift-ci please smoke test macOS platform.

zoecarver · 2021-06-17T15:23:48Z

@swift-ci please smoke test macOS platform.

egorzhdan · 2021-06-18T21:15:53Z

@swift-ci please smoke test

egorzhdan · 2021-06-18T22:32:27Z

@swift-ci please test Windows

egorzhdan · 2021-06-18T22:32:39Z

@swift-ci please smoke test macOS

egorzhdan · 2021-07-23T17:10:04Z

@swift-ci please smoke test

zoecarver

This is really great work. Thank you for all the time you put into it. This implementation is now much better. I have a few very small nits, you can either take them or leave them. Then 🚢 it.

lib/ClangImporter/ClangImporter.cpp

lib/ClangImporter/ImportDecl.cpp

Previously a namespace declaration was imported along with all of its redeclarations, and their members were added to a single Swift extension. This was problematic when a single namespace is declared in multiple modules – the extension belonged to only one of them. For an example of this, try printing a module interface for `std.string`/`std.iosfwd` – it will be empty, even though the declarations from those modules are actually imported into Swift correctly. This change makes sure that when we're importing different redeclarations of the same namespace, we're adding them as separate extensions to appropriate modules.

egorzhdan · 2021-07-23T20:52:13Z

@swift-ci please smoke test

egorzhdan · 2021-07-23T22:17:46Z

@swift-ci please test Windows

egorzhdan · 2021-07-24T08:03:57Z

@swift-ci please smoke test Linux

egorzhdan · 2021-07-24T10:40:35Z

Thanks for reviewing this @zoecarver!

rintaro · 2021-07-27T16:35:55Z

@egorzhdan @zoecarver Is it possible this change causes ASAN failures?
ASAN test started failing in https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-macos/5835/changes

zoecarver · 2021-07-27T16:47:33Z

@rintaro that would seem a bit odd to me. The test that's failing doesn't seem to touch namespaces whatsoever. That being said, this patch did touch the clang importer more generally, so it is possible that it broke that test.

Maybe we should put up a PR to revert this change and see if that fixes the bots? Side note: I'm almost done with a patch that would undo many of the more general clang importer changes and moves the namespace importing logic elsewhere, if this patch is the culprit, I suspect my change will fix it. So, if that is the case, let's revert the patch and I can submit something later that will probably not have the same issues.

For future readers, here's the Jenkins output (so that you don't have to dig through the huge text file)

******************** TEST 'Swift(macosx-x86_64) :: Interpreter/SDK/weak_objc_interop.swift' FAILED ********************
Script:
--
: 'RUN: at line 1';   rm -rf "/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp" && mkdir -p "/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp"
: 'RUN: at line 3';   cp /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/test/Interpreter/SDK/weak_objc_interop.swift /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/main.swift
: 'RUN: at line 4';   xcrun --toolchain default --sdk '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk' clang++ -target x86_64-apple-macosx10.9 -fmodules-cache-path='/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache' -fno-objc-arc /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/test/Interpreter/SDK/Inputs/ObjCWeak/ObjCWeak.m -c -o /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/ObjCWeak.o
: 'RUN: at line 5';   xcrun --toolchain default --sdk '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk' /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/bin/swiftc -target x86_64-apple-macosx10.9  -module-cache-path /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache  -F '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks' -toolchain-stdlib-rpath -Xlinker -rpath -Xlinker '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks' -Xlinker -rpath -Xlinker /usr/lib/swift -Xlinker -headerpad_max_install_names  -swift-version 4  -Xfrontend -define-availability -Xfrontend 'SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0'  -F '/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/lib' -Xlinker -rpath -Xlinker '/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/lib' /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/main.swift -I /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/test/Interpreter/SDK/Inputs/ObjCWeak/ -Xlinker /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/ObjCWeak.o -o /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/weak_objc_interop -Xfrontend -disable-access-control
: 'RUN: at line 6';   /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/utils/swift-darwin-postprocess.py /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/weak_objc_interop
: 'RUN: at line 7';   /usr/bin/env DYLD_LIBRARY_PATH='/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/lib/swift/macosx' LD_LIBRARY_PATH='/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/lib/swift/macosx:' SIMCTL_CHILD_DYLD_LIBRARY_PATH='/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/lib/swift/macosx'  /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64/test-macosx-x86_64/Interpreter/SDK/Output/weak_objc_interop.swift.tmp/weak_objc_interop 2>&1 | /Applications/Xcode.app/Contents/Developer/usr/bin/python3 /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/utils/PathSanitizingFileCheck --sanitize BUILD_DIR=/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/swift-macosx-x86_64 --sanitize SOURCE_DIR=/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift --use-filecheck /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/buildbot_incremental_asan/llvm-macosx-x86_64/bin/FileCheck  /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/test/Interpreter/SDK/weak_objc_interop.swift
--
Exit Code: 1
Command Output (stderr):
/Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/test/Interpreter/SDK/weak_objc_interop.swift:28:16: error: CHECK-NEXT: expected string not found in input

// CHECK-NEXT: Gone

^

:4:1: note: scanning from here

AddressSanitizer:DEADLYSIGNAL

^

:8:39: note: possible intended match here

#0 0x7fff6e9c9840 in objc_loadWeakRetained+0xa2 (libobjc.A.dylib:x86_64h+0xd840)

^
Input file: 

Check file: /Volumes/swift-ci/jenkins/workspace/oss-swift-incremental-ASAN-RA-macos/swift/test/Interpreter/SDK/weak_objc_interop.swift
-dump-input=help explains the following input dump.
Input was:

<<<<<<

1: before giving up strong reference:

2: Swift Object

3: after giving up strong reference:

4: AddressSanitizer:DEADLYSIGNAL

next:28'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found

5: =================================================================

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

6: ==1632==ERROR: AddressSanitizer: SEGV on unknown address 0x000013800020 (pc 0x7fff6e9c9840 bp 0x7ffeec461600 sp 0x7ffeec4615d0 T0)

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

7: ==1632==The signal is caused by a READ memory access.

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

8:  #0 0x7fff6e9c9840 in objc_loadWeakRetained+0xa2 (libobjc.A.dylib:x86_64h+0xd840)

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

next:28'1                                           ?                                           possible intended match

9:  #1 0x7fff6e9cb67b in objc_loadWeak+0xe (libobjc.A.dylib:x86_64h+0xf67b)

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

10:  #2 0x1037a898c in tryWeakReferencing+0x1cc (weak_objc_interop:x86_64+0x10000a98c)

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

11:  #3 0x1037a505e in main+0x5e (weak_objc_interop:x86_64+0x10000705e)

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

12:  #4 0x7fff6fb77cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)

next:28'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

13:

next:28'0     ~

.

.

.












--

egorzhdan · 2021-07-27T17:24:06Z

I think 3372ab7 is a more likely reason of the ASAN failure, since it touches ObjC closures & ManagedValues.
If the CI run proves me wrong, I'll be ready to investigate further.

rintaro · 2021-07-27T17:49:49Z

Thank you for looking into this @egorzhdan ! I just put up another PR for testing 3372ab7 revert.

rintaro · 2021-07-27T17:55:10Z

Also started building with ASAN locally. Since if 3372ab7 is the reason, the failure is flaky, so we need to run the test multiple times.

rintaro · 2021-07-27T18:48:35Z

Unfortunately, an ASAN PR test without any change https://ci.swift.org/job/swift-PR-macos-ASAN-test/229/console just passed Interpreter/SDK/weak_objc_interop.swift test 😢 So probably this test is indeed flaky. Hope I can reproduce the failure locally.

rintaro · 2021-07-28T17:17:51Z

https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-macos/
So the ASAN bot succeeded 2 times in a row. Now I think the failure was a transient. Sorry for the hassle!

egorzhdan · 2021-07-28T17:24:17Z

Alright, thanks for the update @rintaro!

This is just a cleanup of `importFullName` invocations, discussed here: swiftlang#37695 (comment) This change makes `importFullName` return a pair of a canonical & alternative names instead of having one of them passed as a parameter.

egorzhdan added the c++ interop Feature: Interoperability with C++ label May 28, 2021

egorzhdan requested review from gribozavr and zoecarver May 28, 2021 19:22

egorzhdan force-pushed the cxx-namespaces branch 2 times, most recently from 6c2dc5d to 8dbd18b Compare May 29, 2021 11:43

zoecarver reviewed Jun 5, 2021

View reviewed changes

egorzhdan force-pushed the cxx-namespaces branch 2 times, most recently from 14941cd to 80e4bfc Compare June 8, 2021 19:07

egorzhdan force-pushed the cxx-namespaces branch from 80e4bfc to 83f5146 Compare June 17, 2021 08:24

zoecarver reviewed Jun 17, 2021

View reviewed changes

test/Interop/Cxx/namespace/submodules-module-interface.swift Show resolved Hide resolved

lib/ClangImporter/ClangImporter.cpp Outdated Show resolved Hide resolved

lib/ClangImporter/ClangImporter.cpp Outdated Show resolved Hide resolved

egorzhdan force-pushed the cxx-namespaces branch 2 times, most recently from 6221bc9 to 4723cf7 Compare June 18, 2021 21:15

egorzhdan force-pushed the cxx-namespaces branch from 4723cf7 to 2cc300b Compare July 14, 2021 12:13

egorzhdan mentioned this pull request Jul 17, 2021

C++ Interop: improve skipping already imported struct members #38439

Merged

egorzhdan force-pushed the cxx-namespaces branch from 2cc300b to ec155ca Compare July 23, 2021 17:08

egorzhdan changed the title ~~C++ Interop: import namespace redecls separately~~ C++ Interop: import namespaces redecls as separate extensions Jul 23, 2021

zoecarver approved these changes Jul 23, 2021

View reviewed changes

lib/ClangImporter/ClangImporter.cpp Outdated Show resolved Hide resolved

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

egorzhdan force-pushed the cxx-namespaces branch from ec155ca to cfc9483 Compare July 23, 2021 20:51

egorzhdan merged commit 03ff464 into swiftlang:main Jul 24, 2021

egorzhdan deleted the cxx-namespaces branch July 24, 2021 11:11

rintaro mentioned this pull request Jul 27, 2021

Revert "C++ Interop: import namespaces redecls as separate extensions" #38651

Closed

rintaro mentioned this pull request Jul 27, 2021

Revert "Fix arg cleanup for objc continuations" #38652

Closed

egorzhdan mentioned this pull request Aug 5, 2021

ClangImporter: NFC: simplify importFullName calls #38764

Merged

C++ Interop: import namespaces redecls as separate extensions #37695

C++ Interop: import namespaces redecls as separate extensions #37695

Uh oh!

Conversation

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

Uh oh!

Uh oh!

egorzhdan commented May 29, 2021

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

egorzhdan commented Jun 8, 2021

Uh oh!

egorzhdan commented Jun 16, 2021

Uh oh!

egorzhdan commented Jun 17, 2021

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoecarver commented Jun 17, 2021

Uh oh!

zoecarver commented Jun 17, 2021

Uh oh!

egorzhdan commented Jun 18, 2021

Uh oh!

egorzhdan commented Jun 18, 2021

Uh oh!

egorzhdan commented Jun 18, 2021

Uh oh!

egorzhdan commented Jul 23, 2021

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

egorzhdan commented Jul 23, 2021

Uh oh!

egorzhdan commented Jul 23, 2021

Uh oh!

egorzhdan commented Jul 24, 2021

Uh oh!

egorzhdan commented Jul 24, 2021

Uh oh!

rintaro commented Jul 27, 2021

Uh oh!

zoecarver commented Jul 27, 2021

Command Output (stderr):

Uh oh!

egorzhdan commented Jul 27, 2021

Uh oh!

rintaro commented Jul 27, 2021

Uh oh!

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

Uh oh!

Uh oh!

rintaro commented Jul 27, 2021

Uh oh!

rintaro commented Jul 28, 2021

Uh oh!

egorzhdan commented Jul 28, 2021

Uh oh!

Uh oh!

egorzhdan commented May 28, 2021 •

edited

Loading

rintaro commented Jul 27, 2021 •

edited

Loading