[compiler-rt] Honor CMAKE_SYSROOT in tests

[abrachet]

Summary

This makes sure that compiler-rt properly uses --sysroot when compiling.
This isn't an issue for normal CMake targets, tests however often invoke
clang on their own. A few places were not correctly using the flags from
COMPILER_RT_TEST_COMPILER_CFLAGS, this patch fixes that.

[phosek]

I tried to address a similar issue in D126936. I wonder if the concern raised @delcypher applies to this change as well. Specifically, that there is no single sysroot that would be apply to all Darwin targets. Concretely, on Apple platforms we use find_darwin_sdk_dir to find the right sysroot for each Darwin flavor, ignoring CMAKE_SYSROOT.

[phosek]

compiler-rt/CMakeLists.txt

573–575

A potential path forward that sidesteps the Darwin issue would be to restrict this only to non-Apple platforms.

[phosek]

compiler-rt/lib/tsan/rtl/CMakeLists.txt

117

This is a bit surprising but after reading how EXTRA_CFLAGS is being used, I understand why this needed. I'd add a short comment explaining why we need to add test CFLAGS here.

[vitalybuka]

LGTM if it does not break bots

[delcypher]

@abrachet This change looks reasonable but it could really do with an actual description of the change being made and why.

The fact its only touching the TSan, SafeStack, and LibFuzzer test suites (and not others) should also be explained.

[delcypher]

compiler-rt/CMakeLists.txt

574

@abrachet There should probably be comment here (or at the very least an explanation in the commit message/phabricator review description) as to why this is needed.

CMake's docs say CMAKE_SYSROOT is set in toolchain files, so I'm guessing the motivation here is to make sure if compiler-rt's CMake configure is set to use that if a toolchain file was used?

compiler-rt/test/tsan/CMakeLists.txt

43 ↗

(On Diff #524523)

@abrachet get_test_cc_for_arch is already supposed to add COMPILER_RT_TEST_COMPILER_CFLAGS in several cases. Why doesn't that work for your use case?

[delcypher]

@abrachet I left some comments/questions. When putting a patch up for review in phabricator please include the message that would be in the commit in the review to help reviewers understand your change and why it's being made.

[abrachet]

In D151057#4375370, @delcypher wrote:

The fact its only touching the TSan, SafeStack, and LibFuzzer test suites (and not others) should also be explained.

These just happen to be the only others that were breaking because they accidentally omitted adding config.target_cflags in their respective compiler substitutions.

compiler-rt/test/tsan/CMakeLists.txt

43 ↗

(On Diff #524523)

It only does this when "cross compiling" (I use quotes because the comment says this but the check is just ${arch} MATCHES "arm|aarch64|riscv32|riscv64". Other platforms won't get this. I've instead changed the way that target_cflags is set in lit.common.configured.in so COMPILER_RT_TEST_COMPILER_CFLAGS will always be appended to target_cflags. That solution also allows us to remove this workaround of adding COMPILER_RT_TEST_COMPILER_CFLAGS here. I didn't previously realize that setting *_TEST_TARGET_CFLAGS would override the default for config.target_cflags which is COMPILER_RT_TEST_COMPILER_CFLAGS

[delcypher]

@abrachet Thanks for addressing my feedback. I have an additional question but other than that I think the change looks reasonable. I'm going to add some other reviewers from Apple who work on compiler-rt more than me for visibility.

compiler-rt/test/lit.common.configured.in

70

Why was it necessary to remove the call to set_default and add this manual code here?

[delcypher]

@yln @thetruestblue @rsundahl @wrotki Adding you for visibility. I suspect this change is fine but I just wanted to make sure you know its happening.

[rsundahl]

In D151057#4362726, @phosek wrote:

...Concretely, on Apple platforms we use find_darwin_sdk_dir to find the right sysroot for each Darwin flavor, ignoring CMAKE_SYSROOT.

This seems important enough of a distinction to document early in say compiler-rt/CMakeLists.txt so we can refer back to it as rationale when you add "if (NOT APPLE AND (...)), else the "NOT APPLE" is just hanging there as a head scratcher.

[abrachet]

In D151057#4409312, @rsundahl wrote:

In D151057#4362726, @phosek wrote:

...Concretely, on Apple platforms we use find_darwin_sdk_dir to find the right sysroot for each Darwin flavor, ignoring CMAKE_SYSROOT.

This seems important enough of a distinction to document early in say compiler-rt/CMakeLists.txt so we can refer back to it as rationale when you add "if (NOT APPLE AND (...)), else the "NOT APPLE" is just hanging there as a head scratcher.

Added a comment

compiler-rt/test/lit.common.configured.in

70

set_default would only set target_cflags to @COMPILER_RT_TEST_COMPILER_CFLAGS@ if target_cflags isn't already set. Almost all of the local lit.site.cfg.py.in files have something like config.target_cflags = "@ASAN_TEST_TARGET_CFLAGS@". So if ASAN_TEST_TARGET_CFLAGS is set, the asan tests won't have the flags specified in COMPILER_RT_TEST_COMPILER_CFLAGS, which is surprising behavior. This was a problem specifically for tsan because TSAN_TEST_TARGET_CFLAGS was being set, see https://reviews.llvm.org/D151057?id=524523#inline-1465740

I've changed this here so COMPILER_RT_TEST_COMPILER_CFLAGS will be appended to target_cflags for all test suites regardless of setting sanitizer test specific flags via *_TEST_TARGET_CFLAGS.

compiler-rt/CMakeLists.txt

 list(APPEND COMPILER_RT_COMMON_CFLAGS ${stdlib_flag})
 list(APPEND COMPILER_RT_COMMON_LINK_FLAGS ${stdlib_flag})
 
 # TODO: There's a lot of duplication across lib/*/tests/CMakeLists.txt files,
 # move some of the common flags to COMPILER_RT_UNITTEST_CFLAGS.
 
 # Unittests need access to C++ standard library.
 string(APPEND COMPILER_RT_TEST_COMPILER_CFLAGS " ${stdlib_flag}")
+# CMAKE_SYSROOT instructs cmake to pass `--sysroot=${CMAKE_SYSROOT}` to compile
+# and link steps. For normal cmake targets we don't need to do anything, but for
+# lit tests we need to manually add this flag to the compilation options.
+# This isn't done here for Apple platforms where sysroot handling is more
+# nuanced. See `find_darwin_sdk_dir` and `darwin_add_builtin_library`.
+if (NOT APPLE AND (COMPILER_RT_HAS_SYSROOT_FLAG AND CMAKE_SYSROOT))
+  string(APPEND COMPILER_RT_TEST_COMPILER_CFLAGS " --sysroot=${CMAKE_SYSROOT}")
+endif()
 
 # When cross-compiling, COMPILER_RT_TEST_COMPILER_CFLAGS help in compilation
 # and linking of unittests.
 string(REPLACE " " ";" COMPILER_RT_UNITTEST_CFLAGS "${COMPILER_RT_TEST_COMPILER_CFLAGS}")
 set(COMPILER_RT_UNITTEST_LINK_FLAGS ${COMPILER_RT_UNITTEST_CFLAGS})
 
 if(COMPILER_RT_USE_LLVM_UNWINDER)
   # We're linking directly against the libunwind that we're building so don't

compiler-rt/lib/tsan/go/buildgo.sh

else
   FLAGS="$DEBUG_FLAGS"
 fi
 
 if [ "$SILENT" != "1" ]; then
   echo $CC gotsan.cpp -c -o $DIR/race_$SUFFIX.syso $FLAGS $CFLAGS
 fi
 $CC $DIR/gotsan.cpp -c -o $DIR/race_$SUFFIX.syso $FLAGS $CFLAGS
 
-$CC $OSCFLAGS $ARCHCFLAGS test.c $DIR/race_$SUFFIX.syso -g -o $DIR/test $OSLDFLAGS $LDFLAGS
+$CC $OSCFLAGS $ARCHCFLAGS test.c $DIR/race_$SUFFIX.syso -g -o $DIR/test $OSLDFLAGS $LDFLAGS $EXTRA_CFLAGS
 
 # Verify that no libc specific code is present.
 if [ "$DEPENDS_ON_LIBC" != "1" ]; then
   if nm $DIR/race_$SUFFIX.syso | grep -q __libc_; then
     printf -- '%s seems to link to libc\n' "race_$SUFFIX.syso"
     exit 1
   fi
 fi

compiler-rt/lib/tsan/rtl/CMakeLists.txt

set(TSAN_HEADERS
   )
 
 set(TSAN_RUNTIME_LIBRARIES)
 add_compiler_rt_component(tsan)
 
 if("${CMAKE_C_FLAGS}" MATCHES "-Wno-(error=)?unused-command-line-argument")
   set(EXTRA_CFLAGS "-Wno-error=unused-command-line-argument ${EXTRA_CFLAGS}")
 endif()
+# Make sure the flags in COMPILER_RT_TEST_COMPILER_CFLAGS are also used by buildgo.sh.
+set(EXTRA_CFLAGS "${COMPILER_RT_TEST_COMPILER_CFLAGS} ${EXTRA_CFLAGS}")
 
 if(APPLE)
   # Ideally we would check the SDK version for the actual platform we are
   # building for here.  To make our lifes easier we assume the host SDK setup is
   # sane and use the macOS SDK version as a proxy for aligned SDKs.
   find_darwin_sdk_version(macosx_sdk_version "macosx")
   if ("${macosx_sdk_version}" VERSION_LESS 10.12)
     message(FATAL_ERROR "Building the TSan runtime requires at least macOS SDK 10.12 (or aligned SDK on other platforms)")

compiler-rt/test/fuzzer/lit.cfg.py

 libfuzzer_src_root = os.path.join(config.compiler_rt_src_root, "lib", "fuzzer")
 config.substitutions.append(("%libfuzzer_src", libfuzzer_src_root))
 
 config.substitutions.append(("%python", '"%s"' % (sys.executable)))
 
 
 def generate_compiler_cmd(is_cpp=True, fuzzer_enabled=True, msan_enabled=False):
     compiler_cmd = config.clang
-    extra_cmd = config.target_flags
+    extra_cmd = config.target_flags + config.target_cflags
 
     if is_cpp:
         std_cmd = "--driver-mode=g++"
     else:
         std_cmd = ""
 
     if msan_enabled:
         sanitizers = ["memory"]

compiler-rt/test/lit.common.configured.in

 @LIT_SITE_CFG_IN_HEADER@
 
 # Set attribute value if it is unset.
 def set_default(attr, value):
   if not getattr(config, attr, None):
     setattr(config, attr, value)
 
 # Generic config options for all compiler-rt lit tests.
 set_default("target_triple", "@COMPILER_RT_DEFAULT_TARGET_TRIPLE@")
-set_default("target_cflags", "@COMPILER_RT_TEST_COMPILER_CFLAGS@")
 set_default("host_arch", "@HOST_ARCH@")
 set_default("target_arch", "@COMPILER_RT_DEFAULT_TARGET_ARCH@")
 set_default("host_os", "@HOST_OS@")
 set_default("llvm_build_mode", "@LLVM_BUILD_MODE@")
 set_default("llvm_src_root", "@LLVM_MAIN_SRC_DIR@")
 set_default("llvm_obj_root", "@LLVM_BINARY_DIR@")
 set_default("compiler_rt_src_root", "@COMPILER_RT_SOURCE_DIR@")
 set_default("compiler_rt_obj_root", "@COMPILER_RT_BINARY_DIR@")
elif config.android:
   else:
     set_default("target_suffix", "-%s-android" % config.target_arch)
 else:
   set_default("target_suffix", "-%s" % config.target_arch)
 
 set_default("have_zlib", @ZLIB_FOUND_PYBOOL@)
 set_default("libcxx_used", "@LLVM_LIBCXX_USED@")
 
+target_cflags = getattr(config, "target_cflags", "")
+setattr(config, "target_cflags", target_cflags + " @COMPILER_RT_TEST_COMPILER_CFLAGS@")
+
 # LLVM tools dir can be passed in lit parameters, so try to
 # apply substitution.
 config.llvm_tools_dir = lit_config.substitute(config.llvm_tools_dir)
 config.clang = lit_config.substitute(config.clang)
 config.compiler_rt_libdir = lit_config.substitute(config.compiler_rt_libdir)
 
 if not os.path.exists(config.clang):
   lit_config.fatal("Can't find compiler on path %r" % config.clang)
 
 import lit.llvm
 lit.llvm.initialize(lit_config, config)
 
 # Setup attributes common for all compiler-rt projects.
 lit_config.load_config(config, "@COMPILER_RT_SOURCE_DIR@/test/lit.common.cfg.py")

compiler-rt/test/safestack/lit.cfg.py

 # -*- Python -*-
 
 import os
 
 # Setup config name.
 config.name = "SafeStack"
 
 # Setup source root.
 config.test_source_root = os.path.dirname(__file__)
 
 # Test suffixes.
 config.suffixes = [".c", ".cpp", ".m", ".mm", ".ll", ".test"]
 
 # Add clang substitutions.
 config.substitutions.append(
-    ("%clang_nosafestack ", config.clang + " -O0 -fno-sanitize=safe-stack ")
+    (
+        "%clang_nosafestack ",
+        config.clang + config.target_cflags + " -O0 -fno-sanitize=safe-stack ",
+    )
 )
 config.substitutions.append(
-    ("%clang_safestack ", config.clang + " -O0 -fsanitize=safe-stack ")
+    (
+        "%clang_safestack ",
+        config.clang + config.target_cflags + " -O0 -fsanitize=safe-stack ",
+    )
 )
 
 if config.lto_supported:
     config.substitutions.append(
         (
             r"%clang_lto_safestack ",
             " ".join([config.clang] + config.lto_flags + ["-fsanitize=safe-stack "]),
         )
     )
 
 if config.host_os not in ["Linux", "FreeBSD", "NetBSD"]:
     config.unsupported = True