[sanitizer] When building tests on Darwin, always pass -arch and other common flags

[yln]

Can you provide some context? Dan is probably the best person to comment on this. @delcypher

[kubamracek]

Ah, sorry :) The context is that any test config for arm64 or arm64e isn’t getting the right -arch flag passed in.

[delcypher]

Could you use get_test_cflags_for_apple_platform platform instead at all the call site?

compiler-rt/cmake/config-ix.cmake

221

The CMake documentation (https://cmake.org/cmake/help/v3.1/command/if.html) makes it look like (I've not verified this) the operator precedence is

NOT (APPLE AND (ANDROID OR ${arch} MATCHES "arm|aarch64"))

which is wrong because the second condition has been flipped compared to what existed prior to this patch.

Please explicitly use parentheses here to remove any ambiguity.

I'm guessing you wanted:

(NOT APPLE) AND (ANDROID OR ${arch} MATCHES "arm|aarch64"))

[delcypher]

Side note: This get_test_cc_for_arch function is bad in multiple ways.

• ${arch} MATCHES "arm|aarch64" doesn't mean cross compilation. I'm guessing this was s a "good enough" heuristic at the time.
• For Apple platforms it assumes macOS. Which is necessarily correct.

This function should probably just check if COMPILER_RT_TEST_COMPILER and COMPILER_RT_TEST_COMPILER_CFLAGS are set and then have the convention globally that these are only set when doing cross-compilation and the compiler and testing flags need overriding.

[delcypher]

compiler-rt/cmake/config-ix.cmake

221

Sorry I meant

(NOT APPLE) AND (ANDROID OR ${arch} MATCHES "arm|aarch64")

[kubamracek]

compiler-rt/cmake/config-ix.cmake

221

No, "NOT" behaves sensibly in CMake, i.e. if (NOT A AND B) actually means if ((NOT A) AND B). Given that (and the fact that we use this short form in many other places in compiler-rt CMake), do you still want me to add the parens?

[kubamracek]

Also, I understand that the code in question isn't best already, but I don't have the bandwidth to refactor and test any more extensive changes. I'm only trying to address the problem of getting -arch arm64 / arm64e passed into tests, otherwise we're testing the wrong thing. Let's focus on that, please.

[kubamracek]

Specifically, I don't want to mess with the setup for Android and/or non-Darwin crosscompiling, because I have no way of testing that.

[delcypher]

compiler-rt/cmake/config-ix.cmake

221

Ah I read

if(NOT <expression>)
   True if the expression is not true.

and took that to mean that NOT would be evaluated last when used in an if() this way.

but yeah it seems that NOT has higher precedence than AND and OR when reading the text more carefully.

Parenthetical expressions are evaluated first followed by unary tests such as EXISTS, COMMAND, and DEFINED. Then any binary tests such as EQUAL, LESS, GREATER, STRLESS, STRGREATER, STREQUAL, and MATCHES will be evaluated. Then boolean NOT operators and finally boolean AND and then OR operators will be evaluated.

I checked manually and this precedence appears to be respected. So no need to change the parentheses here.

[delcypher]

In D86379#2248114, @kubamracek wrote:

Also, I understand that the code in question isn't best already, but I don't have the bandwidth to refactor and test any more extensive changes. I'm only trying to address the problem of getting -arch arm64 / arm64e passed into tests, otherwise we're testing the wrong thing. Let's focus on that, please.

I'm not saying you have to fix it in more an extensive way. I am just making sure that fixing this in a more extensive and better way is at least considered as an option.

[delcypher]

LGTM provided that you explain in the commit message why this change is being made.

compiler-rt/cmake/config-ix.cmake

else()
     endif()
   endif()
 endfunction()
 
 # Returns a compiler and CFLAGS that should be used to run tests for the
 # specific architecture.  When cross-compiling, this is controled via
 # COMPILER_RT_TEST_COMPILER and COMPILER_RT_TEST_COMPILER_CFLAGS.
 macro(get_test_cc_for_arch arch cc_out cflags_out)
-  if(ANDROID OR ${arch} MATCHES "arm|aarch64")
+  if(NOT APPLE AND (ANDROID OR ${arch} MATCHES "arm|aarch64"))
     # This is only true if we are cross-compiling.
     # Build all tests with host compiler and use host tools.
     set(${cc_out} ${COMPILER_RT_TEST_COMPILER})
     set(${cflags_out} ${COMPILER_RT_TEST_COMPILER_CFLAGS})
   else()
     get_target_flags_for_arch(${arch} ${cflags_out})
     if(APPLE)
       list(APPEND ${cflags_out} ${DARWIN_osx_CFLAGS})