Page MenuHomePhabricator

[CMake] Fix the value of `config.target_cflags` for non-macOS Apple platforms.
ClosedPublic

Authored by delcypher on Feb 23 2019, 5:26 AM.

Details

Summary

The main problem here is that -*-version_min= was not being passed to
the compiler when building test cases. This can cause problems when
testing on devices running older OSs because Clang would previously
assume the minimum deployment target is the the latest OS in the SDK
which could be much newer than what the device is running.

Previously the generated value looked like this:

-arch arm64 -isysroot <path_to_xcode>/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.1.sdk

With this change it now looks like:

-arch arm64 -stdlib=libc++ -miphoneos-version-min=8.0 -isysroot <path_to_xcode>/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.1.sdk

This mirrors the setting of config.target_cflags on macOS.

This change is made for ASan, LibFuzzer, TSan, and UBSan.

To implement this a new get_test_cflags_for_apple_platform() function
has been added that when given an Apple platform name and architecture
returns a string containing the C compiler flags to use when building
tests. This also calls a new helper function is_valid_apple_platform()
that validates Apple platform names.

rdar://problem/50124489

Diff Detail

Event Timeline

delcypher created this revision.Feb 23 2019, 5:26 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 23 2019, 5:26 AM
Herald added subscribers: Restricted Project, jdoerfert, kristof.beyls and 2 others. · View Herald Transcript
yln added inline comments.Feb 25 2019, 10:45 AM
compiler-rt/cmake/config-ix.cmake
221 ↗(On Diff #188042)

Nitpick: is_valid_apple_platform can return true/false, or error.
From the name I would not expect that it can error.
I would prefer:
is_valid_apple_platform -> true/false
[assert/check/ensure/...]_is_valid_apple_platform -> no return value, just assert.

delcypher marked an inline comment as done.Feb 25 2019, 10:48 AM
delcypher added inline comments.
compiler-rt/cmake/config-ix.cmake
221 ↗(On Diff #188042)

The error is a misuse of the API. Passing an empty string is almost certain a bug and we should stop immediately.

delcypher edited the summary of this revision. (Show Details)Apr 23 2019, 3:41 AM
yln accepted this revision.Apr 23 2019, 10:48 AM

Overall changes: LGTM, thanks!

As already noted, I would probably change the is_valid_apple_platform to:
is_apple_platform -> just yes/no, even for the empty string.
or rename to assert_valid_apple_platform() -> no return value, aborts if not.

An is_xxx "getter" having 3 kinds of "results": true, false, or abort; is unexpected to me.
I don't feel too strongly strongly about this though, so land this at your discretion.

This revision is now accepted and ready to land.Apr 23 2019, 10:48 AM
delcypher updated this revision to Diff 196825.Apr 26 2019, 3:15 AM

No real update, just for fixing up commit message on my local machine.

This revision was automatically updated to reflect the committed changes.
In D58578#1475863, @yln wrote:

Overall changes: LGTM, thanks!

As already noted, I would probably change the is_valid_apple_platform to:
is_apple_platform -> just yes/no, even for the empty string.
or rename to assert_valid_apple_platform() -> no return value, aborts if not.

An is_xxx "getter" having 3 kinds of "results": true, false, or abort; is unexpected to me.

One way of looking at this is the empty platform name check being a precondition for the is_valid_apple_platform function.
It seems reasonable (to me at least) that if you violate the pre-condition that we should abort. In a language like C/C++ I probably
wouldn't have a precondition like this but in a language like CMake where it's incredibly easy to accidently pass the empty string
it seems worth having the check.

I don't feel too strongly strongly about this though, so land this at your discretion.

Okay. I've landed it as is. We can revisit this later if it really becomes a problem.

This patch has broken our bots -- when it's building the osx host runtime, it's now triggering an assert on the tsan arm64 test case that wasn't being triggered before. Would you be able to take a look?

Error:

CMake Error at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:177 (message):
  Unsupported architecture: arm64
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:216 (get_target_flags_for_arch)
  /b/s/w/ir/k/llvm-project/compiler-rt/test/tsan/CMakeLists.txt:78 (get_test_cflags_for_apple_platform)

Full log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout

This patch has broken our bots -- when it's building the osx host runtime, it's now triggering an assert on the tsan arm64 test case that wasn't being triggered before. Would you be able to take a look?

Error:

CMake Error at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:177 (message):
  Unsupported architecture: arm64
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:216 (get_target_flags_for_arch)
  /b/s/w/ir/k/llvm-project/compiler-rt/test/tsan/CMakeLists.txt:78 (get_test_cflags_for_apple_platform)

Full log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Could you clarify what you mean by "assert on the tsan arm64 test case", all I see in the log is a CMake error, relating to arm64. I don't see any lit tests being executed.

This patch has broken our bots -- when it's building the osx host runtime, it's now triggering an assert on the tsan arm64 test case that wasn't being triggered before. Would you be able to take a look?

Error:

CMake Error at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:177 (message):
  Unsupported architecture: arm64
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:216 (get_target_flags_for_arch)
  /b/s/w/ir/k/llvm-project/compiler-rt/test/tsan/CMakeLists.txt:78 (get_test_cflags_for_apple_platform)

Full log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Could you clarify what you mean by "assert on the tsan arm64 test case", all I see in the log is a CMake error, relating to arm64. I don't see any lit tests being executed.

Sure -- to clarify, it's not a test failure, it's a build one. The error was triggered during the CMake configuration step for host runtimes, where we have OSX supported arches: x86_64 only. test/tsan/CMakeLists.txt:78 now calls into the new function on the arm64 case, which down the line triggers a CMake assert that arm64 is in the list of supported arches when it isn't.

Seems like prior to this change, the lit tests were being configured without doing any checks on that list of supported arches. I'm not as familiar with how this code is set up, so I can't really say if it's a problem with this patch or if this patch is exposing another bug. Thanks for taking a look!

@juliehockett

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Reverted in r359327. Do you have a script (or something better?) that would let me easily replicate your set up. I suspect your use of LLD + Clang is probably what is different from my set up and leads to the CMake failure. You're building compiler-rt with open source Clang rather than AppleClang (i.e. from Xcode or Xcode command line tools) right?

phosek added a subscriber: phosek.Apr 26 2019, 11:07 AM

@juliehockett

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Reverted in r359327. Do you have a script (or something better?) that would let me easily replicate your set up. I suspect your use of LLD + Clang is probably what is different from my set up and leads to the CMake failure. You're building compiler-rt with open source Clang rather than AppleClang (i.e. from Xcode or Xcode command line tools) right?

To clarify, this is failing when building runtimes for Darwin so shouldn't be Fuchsia specific, but it could be affected by some of the Clang defaults we set in our CMake build. We use lld as the default linker for Fuchsia but not for Darwin where use the system linker.

You can see the CMake invocation at top of this page: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/configure/0/stdout. It's also worth noting that this was only failing in case of the 2-stage build, the single stage build remained green. I can try to reproduce this locally on my Mac as well if that's helpful.

To clarify, this is failing when building runtimes for Darwin so shouldn't be Fuchsia specific, but it could be affected by some of the Clang defaults we set in our CMake build. We use lld as the default linker for Fuchsia but not for Darwin where use the system linker.

Based on the log the https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout log that @juliehockett pointed to the CMake output suggests that LLD is being used when building the compiler-rt runtime

[3397/3401] Performing configure step for 'runtimes'
-- The C compiler identification is Clang 9.0.0
-- The CXX compiler identification is Clang 9.0.0
-- The ASM compiler identification is Clang
-- Found assembler: /b/s/w/ir/k/recipe_cleanup/clangp5sgXt/llvm_build_dir/./bin/clang
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
LLD 9.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project 41327e352285df0f437affc5d6b1522ad650f750) (compatible with GNU linkers)
-- Linker detection: unknown
...
LLD 9.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project 41327e352285df0f437affc5d6b1522ad650f750) (compatible with GNU linkers)
CMake Warning at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake:57 (message):
  Detecting supported architectures from 'ld -v' failed.  Returning default
  set.
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:374 (darwin_get_toolchain_supported_archs)
  /b/s/w/ir/k/llvm-project/compiler-rt/CMakeLists.txt:280 (include)

This isn't actually the source of the problem though so I'll ignore this for now.

Sure -- to clarify, it's not a test failure, it's a build one. The error was triggered during the CMake configuration step for host runtimes, where we have OSX supported arches: x86_64 only. test/tsan/CMakeLists.txt:78 now calls into the new function on the arm64 case, which down the line triggers a CMake assert that arm64 is in the list of supported arches when it isn't.

Seems like prior to this change, the lit tests were being configured without doing any checks on that list of supported arches. I'm not as familiar with how this code is set up, so I can't really say if it's a problem with this patch or if this patch is exposing another bug. Thanks for taking a look!

Yep it's this patch exposing another bug. Your configuration only seems to allow x86_64 as an architecture (any idea why?) and the CMake code in test/tsan/CMakeLists.txt just assumes that arm64 is supported. I'll just hack my build so it's as if x86_64 was the only supported architecture to replicate this failure, then I'll fix the bug and submit a new patch for review.