[cxx-interop] Pull over fix from 8e7766b and apply to strstr, sin, an… #66896

zoecarver · 2023-06-23T19:05:25Z

…d cos.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves #NNNNN, fix apple/llvm-project#MMMMM.

…d cos.

zoecarver · 2023-06-23T19:33:43Z

@swift-ci please smoke test

zoecarver · 2023-06-23T22:15:43Z

It's so odd that the linux CI is failing like this :(

zoecarver · 2023-06-27T16:44:22Z

@swift-ci please smoke test

…ons. Also add back `string.h` as a valid header and ban imports from `_SwiftConcurrencyShims`.

…t`, it's not longer relevant.

zoecarver · 2023-06-28T05:21:18Z

@swift-ci please test

zoecarver · 2023-06-28T05:22:04Z

test/Interop/Cxx/stdlib/print-swiftconcurrencyshims-interface.swift

@@ -1,10 +0,0 @@
-// RUN: %target-swift-ide-test -print-module -module-to-print=_SwiftConcurrencyShims -source-filename=x -enable-experimental-cxx-interop | %FileCheck %s


@egorzhdan can you review this change. Is there some reason we'd want to import this function? It conflicts with Darwin.exit.

I think it used from swift/stdlib/public/Concurrency/Task.swift, I wonder if _Concurrency still builds if we don't import this function.

I think re-declaring exit() in the shim header isn't really great, it would be nice to just import Darwin.exit/Glibc.exit/etc from Task.swift and use that directly from Swift.

I'm fine with either solution as long as _Concurrency builds with interop enabled 🙂

Mind taking another look?

zoecarver · 2023-06-28T05:25:10Z

test/Interop/Cxx/stdlib/avoid-import-cxx-math.swift


-    let _ = CxxStdlib.abs(x) // expected-error {{module 'CxxStdlib' has no member named 'abs'}}


I was getting errors like this:

Darwin.pow:1:13: note: diagnostic produced elsewhere: candidate expects value of type 'Double' for parameter #1 (got 'Float') public func pow(_: Double, _: Double) -> Double

So I figured we could just test the positive case.

Also testing the positive case seems more direct in terms of making sure the thing we want to work works. It also is a bit less fragile and more portable.

… conflict with declarations in the `Darwin` module; improve the test.

zoecarver · 2023-06-28T16:42:46Z

@swift-ci please smoke test

zoecarver · 2023-06-28T16:44:01Z

stdlib/public/SwiftShims/swift/shims/_SwiftConcurrency.h

@@ -16,6 +16,8 @@
 #ifndef SWIFT_CONCURRENCY_H
 #define SWIFT_CONCURRENCY_H

+#include <stdlib.h>


@DougGregor can you please review this change?

Actually, I think I am not going to make this change in this PR. I'm just going to ban the one from stdlib.h. Otherwise, we will have to update SILGenModule::getExit to handle a number of modulemaps/platforms.

egorzhdan · 2023-06-28T16:52:59Z

stdlib/public/SwiftShims/swift/shims/_SwiftConcurrency.h

-extern "C" [[noreturn]]
-#endif
-void exit(int);
-
 #define EXIT_SUCCESS 0


I think EXIT_SUCCESS can also be removed since we now import stdlib.h where it is defined.

…rom `_SwiftConcurrency.h`.

zoecarver · 2023-06-28T17:18:29Z

@egorzhdan I decided to actually just ban exit from stdlib and make everyone use the _SwiftConcurrency. Just because it was too hard to look through all the different places it might live when updating SILGenModule::getExit. WDYT? Can we just update to have everyone use the stdlib declaration after the fact?

zoecarver · 2023-06-28T17:18:45Z

@swift-ci please smoke test

zoecarver · 2023-06-28T21:21:08Z

@swift-ci please smoke test

zoecarver · 2023-06-29T00:20:14Z

@swift-ci please smoke test

zoecarver · 2023-06-29T00:21:05Z

test/Interop/Cxx/stdlib/avoid-import-cxx-math.swift

-    let _ = CxxStdlib.div(x) // expected-error {{module 'CxxStdlib' has no member named 'div'}}
+    let _ = abs(x)
+    // https://github.com/apple/swift/issues/67006
+    // let _ = div(42, 2)


Perfect example of why we should test the positive case: div wasn't available on linux even before this patch because it's in stdlib.h. Issue filed.

egorzhdan · 2023-06-29T11:44:07Z

I decided to actually just ban exit from stdlib and make everyone use the _SwiftConcurrency

Wouldn't this break existing code that uses exit if someone enables C++ interop?

zoecarver · 2023-06-29T16:02:28Z

I decided to actually just ban exit from stdlib and make everyone use the _SwiftConcurrency

Wouldn't this break existing code that uses exit if someone enables C++ interop?

No. Everyone imports _SwiftConcurrency. You can see that exit continues to work in test/Interop/Cxx/stdlib/avoid-import-cxx-math.swift.

[cxx-interop] Pull over fix from 8e7766b and apply to strstr, sin, an…

067abeb

…d cos.

zoecarver requested review from hyp and egorzhdan as code owners June 23, 2023 19:05

[cxx-interop] Fix ambiguous 'exit' (same solution as above).

26e3056

[cxx-interop] check file and directory, not or.

f07a9ff

zoecarver added 2 commits June 27, 2023 22:19

[cxx-interop] Re-work control flow when checking for duplicate functi…

42bc167

…ons. Also add back `string.h` as a valid header and ban imports from `_SwiftConcurrencyShims`.

[cxx-interop][nfc] remove `print-swiftconcurrencyshims-interface.swif…

744783a

…t`, it's not longer relevant.

zoecarver commented Jun 28, 2023

View reviewed changes

[cxx-interop] only ban strstr, sin, cos, and exit on darwin where the…

173329e

… conflict with declarations in the `Darwin` module; improve the test.

zoecarver commented Jun 28, 2023

View reviewed changes

zoecarver requested a review from DougGregor June 28, 2023 16:44

egorzhdan reviewed Jun 28, 2023

View reviewed changes

[cxx-interop] Don't import exit from Darwin module, use the one f…

50ce843

…rom `_SwiftConcurrency.h`.

zoecarver force-pushed the fix-ambiguous-math-func branch from 21e56b2 to 50ce843 Compare June 28, 2023 17:16

zoecarver mentioned this pull request Jun 29, 2023

[cxx-interop] div is not found on linux. #67006

Open

[cxx-interop][nfc] Disable test for div.

6233a65

zoecarver force-pushed the fix-ambiguous-math-func branch from 5718a4b to 6233a65 Compare June 29, 2023 00:20

zoecarver commented Jun 29, 2023

View reviewed changes

zoecarver merged commit 29c0b0d into swiftlang:main Jun 29, 2023

zoecarver mentioned this pull request Jun 29, 2023

[🍒][cxx-interop] Use translation unit scope when doing lookups; disambiguate math functions. #67020

Merged

		@@ -1,10 +0,0 @@
		// RUN: %target-swift-ide-test -print-module -module-to-print=_SwiftConcurrencyShims -source-filename=x -enable-experimental-cxx-interop \| %FileCheck %s


		let _ = CxxStdlib.abs(x) // expected-error {{module 'CxxStdlib' has no member named 'abs'}}

[cxx-interop] Pull over fix from 8e7766b and apply to strstr, sin, an… #66896

[cxx-interop] Pull over fix from 8e7766b and apply to strstr, sin, an… #66896

Uh oh!

Conversation

zoecarver commented Jun 23, 2023

Uh oh!

zoecarver commented Jun 23, 2023

Uh oh!

zoecarver commented Jun 23, 2023

Uh oh!

zoecarver commented Jun 27, 2023

Uh oh!

zoecarver commented Jun 28, 2023

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!

zoecarver commented Jun 28, 2023

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!

zoecarver commented Jun 28, 2023

Uh oh!

zoecarver commented Jun 28, 2023

Uh oh!

zoecarver commented Jun 28, 2023

Uh oh!

zoecarver commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorzhdan commented Jun 29, 2023

Uh oh!

zoecarver commented Jun 29, 2023

Uh oh!

Uh oh!