Page MenuHomePhabricator

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

Authored by kubamracek on Aug 21 2020, 3:19 PM.

Details

Diff Detail

Event Timeline

kubamracek created this revision.Aug 21 2020, 3:19 PM
Herald added subscribers: Restricted Project, mgorny. · View Herald TranscriptAug 21 2020, 3:19 PM
kubamracek requested review of this revision.Aug 21 2020, 3:19 PM
yln added a comment.Aug 21 2020, 4:15 PM

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

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

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 requested changes to this revision.Aug 31 2020, 1:08 PM
This revision now requires changes to proceed.Aug 31 2020, 1:08 PM

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 added inline comments.Aug 31 2020, 1:23 PM
compiler-rt/cmake/config-ix.cmake
221

Sorry I meant

(NOT APPLE) AND (ANDROID OR ${arch} MATCHES "arm|aarch64")
kubamracek added inline comments.Aug 31 2020, 1:28 PM
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?

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.

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 added inline comments.Aug 31 2020, 3:21 PM
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.

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 accepted this revision.Aug 31 2020, 3:28 PM

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

This revision is now accepted and ready to land.Aug 31 2020, 3:28 PM