Check if the C++ compiler used to build the Concurrency library has the -fswift-async-fp flag #39368

finagolfin · 2021-09-19T18:52:23Z

This clang flag is only available in upstream trunk this week, so it breaks my Android CI that tries to build this library with clang 12 from Android NDK 23~~. Only commenting this out so we remember to re-enable it once clang 14 comes out.~~, which this pull fixes.

@aschwaighofer, you added this flag in #39245. @davezarzycki, you've tried to keep this repo building with upstream clang.

davezarzycki · 2021-09-20T13:33:39Z

It looks like recent versions of CMake (3.19 and newer?) have a check_compiler_flag builtin. Might we be able to use this?

CC: @compnerd

finagolfin · 2021-09-20T14:07:44Z

Oh, nice, I'll try that.

aschwaighofer · 2021-09-20T14:44:29Z

I have upstreamed the necessary clang/llvm commits to LLVM in https://reviews.llvm.org/D109451 and https://reviews.llvm.org/D109392.

Are you still seeing issues beside having these 2 commits in your llvm tree?

aschwaighofer · 2021-09-20T14:45:35Z

Oh, sorry I should have read the commit message. Nevermind my comment.

Thanks for fixing this.

aschwaighofer · 2021-09-20T14:47:13Z

We do want this disabled for Linux though (Linux does not have support for the extended frame information so some tests that try to print a backtrace will fail when we emit it). Can you instead explicitly check for Android?

finagolfin · 2021-09-20T15:00:53Z

@aschwaighofer, I'll try that new CMake flag first, which would be perfect. If it doesn't work, we can discuss alternatives.

davezarzycki · 2021-09-20T15:25:50Z

Just FYI, that CMake flag behaves counterintuitively (to some?). From the flang project:

# Builtin check_cxx_compiler_flag doesn't seem to work correctly
macro(check_compiler_flag flag resultVar)
  unset(${resultVar} CACHE)
  check_cxx_compiler_flag("${flag}" ${resultVar})
endmacro()

finagolfin · 2021-09-20T15:36:25Z

I guess the issue is CMake caches the result of looking for a compiler flag and the flang devs want the result evicted and checked again? Doubt that will be an issue for us most of the time, but good to know.

…he -fswift-async-fp flag

finagolfin · 2021-09-20T16:04:39Z

Alright, this fixed the issue for me when cross-compiling the standalone stdlib with the Android NDK clang.

@LucianoPAlmeida, could you apply this patch to your Xcode build and make sure this error is triggered? I'll be happy to change that error text to whatever is deemed best.

LucianoPAlmeida · 2021-09-20T21:11:09Z

Alright, this fixed the issue for me when cross-compiling the standalone stdlib with the Android NDK clang.

@LucianoPAlmeida, could you apply this patch to your Xcode build and make sure this error is triggered? I'll be happy to change that error text to whatever is deemed best.

Sure, no problem I can check it :)

LucianoPAlmeida · 2021-09-21T16:40:38Z

Alright, this fixed the issue for me when cross-compiling the standalone stdlib with the Android NDK clang.

@LucianoPAlmeida, could you apply this patch to your Xcode build and make sure this error is triggered? I'll be happy to change that error text to whatever is deemed best.

@buttaface I'm seeing this

ld: warning: static initializer found in '${BUILD_DIR}/build/Xcode-RelWithDebInfoAssert+swift-DebugAssert/swift-macosx-arm64/stdlib/public/Concurrency/Swift.build/Debug/swift_Concurrency-macosx-arm64.build/Objects-normal/arm64/Task.o'. Use -no_inits to make this an error.  Use -no_warn_inits to suppress warning
0  0x103010224  __assert_rtn + 128
1  0x103015304  ld::tool::OutputFile::makeSplitSegInfoV2(ld::Internal&) (.cold.7) + 0
2  0x102f4bc80  ld::tool::OutputFile::makeSplitSegInfoV2(ld::Internal&) + 1464
3  0x102f45aa8  ld::tool::OutputFile::write(ld::Internal&) + 164
4  0x102ed31d8  main + 584
A linker snapshot was created at:
	/tmp/libswift_Concurrency.dylib-2021-08-21-112009.ld-snapshot
ld: Assertion failed: (_machoSection != 0), function machoSection, file ld.hpp, line 1173.
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Not sure is related, but I just switch from Xcode RC to the official release, perhaps I should try a clean build ...

aschwaighofer · 2021-09-21T16:47:15Z

This linker error likely indicates that we are building the standard library with the just built compiler (i.e the one we build from the current llvm-project checkout) but are not using the flag -fswift-async-fp=always (when building cpp files of the stdlib) and/or -Xfrontend -swift-async-frame-pointer=always (when building swift files).

aschwaighofer · 2021-09-21T16:53:03Z

Which compiler will this check apply to: the just built one or the platform one?

LucianoPAlmeida · 2021-09-22T01:21:13Z

This linker error likely indicates that we are building the standard library with the just built compiler (i.e the one we build from the current llvm-project checkout) but are not using the flag -fswift-async-fp=always (when building cpp files of the stdlib) and/or -Xfrontend -swift-async-frame-pointer=always (when building swift files).

Ah I see, I'm building with SKIP_XCODE_VERSION_CHECK=1 ./swift/utils/build-script --xcode --release-debuginfo --debug-swift --skip-build-benchmarks --swift-darwin-supported-archs "$(uname -m)" --sccache --skip-ios --skip-tvos --skip-watchos, the path for the CXX compiler seems to be /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ using (Xcode 13.0) where this flag is not supported right?

finagolfin · 2021-09-22T07:31:18Z

stdlib/public/Concurrency/CMakeLists.txt

+      "-fswift-async-fp=${swift_concurrency_async_fp_mode}")
+    list(APPEND SWIFT_RUNTIME_CONCURRENCY_SWIFT_FLAGS
+      "-Xfrontend"
+      "-swift-async-frame-pointer=${swift_concurrency_async_fp_mode}")


The idea is to only use this block if the C++ compiler used on macOS supports this flag. That's not working?

No, this is not going to work. We normally use the "just-built" compiler (a compiler build from the checked out llvm-project) to build the standard library's c++ files. Unconditionally checking the platform compiler is not the right thing.

Does Android set SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER to use that version of clang? If so you could check if SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER is true -- and only in the case it is true do compiler check logic.

No, this is not going to work. We normally use the "just-built" compiler (a compiler build from the checked out llvm-project) to build the standard library's c++ files. Unconditionally checking the platform compiler is not the right thing.

Oh, right, forgot about the weird way the native build works, as I mostly cross-compile.

Does Android set SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER to use that version of clang?

No, I use the third option, setting --native-clang-tools-path when cross-compiling the standalone stdlib for Android.

I'll look into this more. Sorry about the wild goose chase, @LucianoPAlmeida.

I'll look into this more. Sorry about the wild goose chase, @LucianoPAlmeida.

No worries, thanks for working on this :)

We normally use the "just-built" compiler (a compiler build from the checked out llvm-project) to build the standard library's c++ files. Unconditionally checking the platform compiler is not the right thing.

Hey @aschwaighofer

If I understand correctly that is true for the ninja builds which are working fine, the original problem we are facing see discussion here is that when building with --xcode the CXX compiler seems to be /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ the one that ships with Xcode version installed which don't have this flag available yet. This happened already sometimes before with other flags for example: https://bugs.swift.org/browse/SR-14714 due the differences of the "just-built" from source clang compiler checked out from apple/llvm-project and the Xcode pre-built clang.
I guess even though they are related to the same flag, the android and Xcode problems are slightly different issues. Does this makes sense?

I understand. What we need to happen is to check the compiler that is used to compile the concurrency c++ sources for the availability of the flag.

Which compiler does the check check_cxx_compiler_flag use to perform the check on a standard ninja build? The just build one or the platform toolchain one?

I would hope whichever one was chosen to build the stdlib, but I'm not sure.

Do you know where the Xcode build chooses to build the stdlib with the platform compiler and not the just-built one? I don't see where that's set.

There is this flag SWIFT_PREBUILT_CLANG that is set based on something passed from somewhere else, but I'm not sure if that is the one doing this ...

finagolfin · 2021-09-22T07:32:06Z

stdlib/public/Concurrency/CMakeLists.txt

+    message(ERROR
+      "The -fswift-async-fp clang flag is not available. This is most likely "
+      "because you are using the Xcode toolchain, which may not have the "
+      "latest flags yet.")


This error should trigger if an older toolchain is used that doesn't have this flag.

You know, we don't even need to error here. Let's say we end up building the C++ parts of the concurrency library with an older Clang. It only affects the back-deployment library, and it only breaks async stack traces through the C++ parts of that library. It's probably more valuable to make this configuration not fail outright than to have it perfect.

My understanding was that you require this flag on macOS, so if the compiler didn't support it, it was better to error here than not pass the flag. You're saying it's okay to build without this flag on macOS when clang doesn't have it?

Yeah, it will break some use cases (stack traces on older macOS versions), but it's not fatal.

finagolfin · 2021-09-22T07:34:04Z

@LucianoPAlmeida, what CMake version are you using? That could cause problems.

LucianoPAlmeida · 2021-09-22T09:45:51Z

@LucianoPAlmeida, what CMake version are you using? That could cause problems.

cmake version 3.21.1

finagolfin · 2021-09-22T11:02:01Z

That should be fine. Is it a fresh build? If not, maybe you're hitting the caching issue Dave mentioned above.

LucianoPAlmeida · 2021-09-22T11:38:22Z

That should be fine. Is it a fresh build? If not, maybe you're hitting the caching issue Dave mentioned above.

The first one not, but I tried a --clean latter and the same result

finagolfin · 2021-09-22T13:20:04Z

Check your CMake log for the Swift compiler: is it showing this error message? Maybe I need to make it a fatal error, I'm not familiar with the CMake error levels.

aschwaighofer · 2021-09-24T18:08:07Z

@swift-ci please smoke test macOS

DougGregor · 2021-09-24T20:20:56Z

stdlib/public/Concurrency/CMakeLists.txt

+      "-Xfrontend"
+      "-swift-async-frame-pointer=${swift_concurrency_async_fp_mode}")
+  else()
+    message(ERROR


Suggested change

message(ERROR

message(WARNING

DougGregor · 2021-09-24T20:34:01Z

Hmm, didn't work:

11:26:00 -- Performing Test CXX_COMPILER_HAS_SWIFT_ASYNC_FP_FLAG
11:26:00 -- Performing Test CXX_COMPILER_HAS_SWIFT_ASYNC_FP_FLAG - Failed
11:26:00 ERRORThe -fswift-async-fp clang flag is not available. This is most likely because you are using the Xcode toolchain, which may not have the latest flags yet.

aschwaighofer · 2021-09-24T20:40:50Z

Yes, this is what I was worried of. The check check_cxx_compiler_flag uses the platform compiler not the just-build one.

aschwaighofer · 2021-09-24T20:44:57Z

Maybe, the logic should be something like (in spirit):

  if not using SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER and not using native-clang-tools path:
     set (CXX_COMPILER_HAS_SWIFT_ASYNC_FP_FLAG TRUE)
  else
     check_cxx_compiler_flag(-fswift-async-fp=always CXX_COMPILER_HAS_SWIFT_ASYNC_FP_FLAG)
  endif
   ...

finagolfin · 2021-09-24T22:16:57Z

if not using SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER and not using native-clang-tools path:

Not going to work as SWIFT_NATIVE_CLANG_TOOLS_PATH is always set by then, as we use it to set the C/C++ compilers to build the stdlib. We might be able to check the SWIFT_PREBUILT_CLANG flag from earlier though.

finagolfin · 2021-09-27T15:23:07Z

I think I'm going to need to switch the clang used to cross-compile this library to the Swift-forked one instead, as XCTest recently switched over to using async when running tests, swiftlang/swift-corelibs-xctest#326, and running Swift package tests on the Android emulator now fails as a result.

I was able to cross-compile this Concurrency library for Android with the Swift-forked clang instead and running a package's tests started working again. I'm not sure if this flag is needed but I no longer need to check for it if I'm using a clang that has it.

As for macOS, I'm not sure why the Xcode build doesn't also use the Swift-forked clang when building the stdlib. When @LucianoPAlmeida reported that difference on the forum, I assumed it was an intentional change in the CMake configuration, but I see no such Xcode-specific CMake config. More likely, the change of the compiler to the Swift-forked clang happens to work for CMake's Ninja generator but not its Xcode generator, ie this is some CMake snafu.

I'm closing this pull since I no longer need it for Android, and I don't know what the issue is on macOS, nor am I well-placed to find it since I don't use macOS.

LucianoPAlmeida · 2021-09-27T23:50:09Z

I think I'm going to need to switch the clang used to cross-compile this library to the Swift-forked one instead, as XCTest recently switched over to using async when running tests, apple/swift-corelibs-xctest#326, and running Swift package tests on the Android emulator now fails as a result.

I was able to cross-compile this Concurrency library for Android with the Swift-forked clang instead and running a package's tests started working again. I'm not sure if this flag is needed but I no longer need to check for it if I'm using a clang that has it.

As for macOS, I'm not sure why the Xcode build doesn't also use the Swift-forked clang when building the stdlib. When @LucianoPAlmeida reported that difference on the forum, I assumed it was an intentional change in the CMake configuration, but I see no such Xcode-specific CMake config. More likely, the change of the compiler to the Swift-forked clang happens to work for CMake's Ninja generator but not its Xcode generator, ie this is some CMake snafu.

Maybe we can force CXX compiler with CMAKE_CXX_COMPILER.html for Xcode generator? I don't know much about those cmake trick so not sure... @aschwaighofer would something like this make sense?

I'm closing this pull since I no longer need it for Android, and I don't know what the issue is on macOS, nor am I well-placed to find it since I don't use macOS.

Thank you for working on this :)

finagolfin · 2021-10-02T20:46:27Z

I just submitted an alternate pull for macOS alone, that simply disables this flag for the Xcode build, #39561. That should let people build with Xcode again, but I have not tested it whatsoever, as I don't use macOS.

Check if the C++ compiler used to build the Concurrency library has t…

d004955

…he -fswift-async-fp flag

finagolfin force-pushed the async-fp branch from 4af4749 to d004955 Compare September 20, 2021 15:59

finagolfin changed the title ~~Comment out disabling extended frame info on non-Darwin platforms, because the flag hasn't made it into an upstream clang release yet.~~ Check if the C++ compiler used to build the Concurrency library has the -fswift-async-fp flag Sep 20, 2021

finagolfin commented Sep 22, 2021

View reviewed changes

DougGregor reviewed Sep 24, 2021

View reviewed changes

finagolfin closed this Sep 27, 2021

finagolfin deleted the async-fp branch September 28, 2021 11:25

LucianoPAlmeida mentioned this pull request Oct 2, 2021

[Concurrency] Disable applying the -fswift-async-fp flag for Xcode builds until someone figures out the root cause #39561

Closed

DougGregor mentioned this pull request Oct 5, 2021

[DNM] Disable use of Clang flag -fswift-async-fp= #39573

Closed

Check if the C++ compiler used to build the Concurrency library has the -fswift-async-fp flag #39368

Check if the C++ compiler used to build the Concurrency library has the -fswift-async-fp flag #39368

Conversation

finagolfin commented Sep 19, 2021 • edited Loading

davezarzycki commented Sep 20, 2021

finagolfin commented Sep 20, 2021

aschwaighofer commented Sep 20, 2021 • edited Loading

aschwaighofer commented Sep 20, 2021

aschwaighofer commented Sep 20, 2021

finagolfin commented Sep 20, 2021

davezarzycki commented Sep 20, 2021

finagolfin commented Sep 20, 2021

finagolfin commented Sep 20, 2021

LucianoPAlmeida commented Sep 20, 2021

LucianoPAlmeida commented Sep 21, 2021

aschwaighofer commented Sep 21, 2021 • edited Loading

aschwaighofer commented Sep 21, 2021 • edited Loading

LucianoPAlmeida commented Sep 22, 2021

Choose a reason for hiding this comment

aschwaighofer Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Sep 22, 2021

LucianoPAlmeida commented Sep 22, 2021

finagolfin commented Sep 22, 2021

LucianoPAlmeida commented Sep 22, 2021

finagolfin commented Sep 22, 2021

aschwaighofer commented Sep 24, 2021

Choose a reason for hiding this comment

DougGregor commented Sep 24, 2021

aschwaighofer commented Sep 24, 2021

aschwaighofer commented Sep 24, 2021 • edited Loading

finagolfin commented Sep 24, 2021

finagolfin commented Sep 27, 2021

LucianoPAlmeida commented Sep 27, 2021

finagolfin commented Oct 2, 2021

finagolfin commented Sep 19, 2021 •

edited

Loading

aschwaighofer commented Sep 20, 2021 •

edited

Loading

aschwaighofer commented Sep 21, 2021 •

edited

Loading

aschwaighofer commented Sep 21, 2021 •

edited

Loading

aschwaighofer Sep 22, 2021 •

edited

Loading

aschwaighofer commented Sep 24, 2021 •

edited

Loading