[compiler-rt] [test] Disable sunrpc tests when rpc/xdr.h is missing

[mgorny]

Summary

Disable tests requiring sunrpc when the relevant headers are missing.
In order to accommodate that, move the header check
from sanitizer_common to base-config-ix, and define the check result
as a global variable there. Use it afterwards both for definition
needed by sanitizer_common, and to control 'sunrpc' test feature.

While at it, remove the append_have_file_definition macro that was used
only once, and no longer fits the split check-definition.

[mgorny]

Gentle ping.

[mgorny]

Ping. This is necessary to resolve https://github.com/google/sanitizers/issues/974 and fix tests on systems which don't enable all the backwards compatibility cruft (Arch, Gentoo). See also D47817 as necessary dependency.

[mgorny]

Rebased.

[mgorny]

Updated to use find_package().

[mgorny]

Fix bug in previous patch.

[mgorny]

Need to update this for the new idea of not using libtirpc at all.

[mgorny]

Following the changes to D47817, updated this one to disable relevant tests when sunrpc headers are not available.

[mgorny]

Ping. I'd really like to merge this before the branch.

[Lekensteyn]

The direction of this patch looks reasonable to me. Is it worth mentioning the issue (https://github.com/google/sanitizers/issues/974) in the commit message?

What environment have you tested this in, could you verify that the tests are indeed skipped on a system without these headers, and passed otherwise?

lib/sanitizer_common/CMakeLists.txt

180

The old code defined HAVE_xxx to 0 or 1, but in general wouldn't it be preferable to define it to 1 if available, and not define it at all if unavailable? This would match most other HAVE_xxx checks.

In order to make the lit config work in this case, you have to use something like pythonize_bool(HAVE_RPC_XDR_H).

test/lit.common.configured.in

39 ↗

(On Diff #181133)

This becomes 0 or 1 instead of True or False, not sure if that will cause issues later.

[mgorny]

In D47819#1356347, @Lekensteyn wrote:

The direction of this patch looks reasonable to me. Is it worth mentioning the issue (https://github.com/google/sanitizers/issues/974) in the commit message?

Yes, makes sense. I'll add it when committing.

What environment have you tested this in, could you verify that the tests are indeed skipped on a system without these headers, and passed otherwise?

I have only tests without those headers. I'll try to rebuild glibc with them enabled if it's still supported but it'd be preferable if somebody else tested it on 'natural' environment.

lib/sanitizer_common/CMakeLists.txt

180

Well, I suppose I could do that but given that the old code did it like this, the 0/1 definition seems to be a common practice in compiler-rt, and there is no apparent benefit from doing it the other way, I don't think it a good thing to do.

test/lit.common.configured.in

39 ↗

(On Diff #181133)

It shouldn't. 0/1 evaluates the same in boolean context, and other LLVM tests are relying on 0/1 being passed to lit already.

[mgorny]

PASS: MemorySanitizer-X86_64 :: Linux/sunrpc_bytes.cc (2932 of 6195)
PASS: MemorySanitizer-X86_64 :: Linux/sunrpc_string.cc (2935 of 6195)
PASS: MemorySanitizer-X86_64 :: Linux/sunrpc.cc (2974 of 6195)
PASS: ThreadSanitizer-x86_64 :: sunrpc.cc (5110 of 6195)

So yep, it works. Tested both ways now.

[Lekensteyn]

In D47819#1356498, @mgorny wrote:

PASS: MemorySanitizer-X86_64 :: Linux/sunrpc_bytes.cc (2932 of 6195)
PASS: MemorySanitizer-X86_64 :: Linux/sunrpc_string.cc (2935 of 6195)
PASS: MemorySanitizer-X86_64 :: Linux/sunrpc.cc (2974 of 6195)
PASS: ThreadSanitizer-x86_64 :: sunrpc.cc (5110 of 6195)

So yep, it works. Tested both ways now.

Thanks for testing! It looks good to me.

cmake/base-config-ix.cmake

 # The CompilerRT build system requires CMake version 2.8.8 or higher in order
 # to use its support for building convenience "libraries" as a collection of
 # .o files. This is particularly useful in producing larger, more complex
 # runtime libraries.
 
 include(CheckIncludeFile)
 include(CheckCXXSourceCompiles)
 
 check_include_file(unwind.h HAVE_UNWIND_H)
 
+# used in sanitizer_common and tests
+include(FindPkgConfig)
+pkg_check_modules(TIRPC libtirpc)
+
 # Top level target used to build all compiler-rt libraries.
 add_custom_target(compiler-rt ALL)
 add_custom_target(install-compiler-rt)
 add_custom_target(install-compiler-rt-stripped)
 set_target_properties(compiler-rt PROPERTIES FOLDER "Compiler-RT Misc")
 
 # Setting these variables from an LLVM build is sufficient that compiler-rt can
 # construct the output paths, so it can behave as if it were in-tree here.

lib/sanitizer_common/CMakeLists.txt

set(SANITIZER_HEADERS
   sanitizer_thread_registry.h
   sanitizer_vector.h
   sanitizer_win.h)
 
 include_directories(..)
 
 set(SANITIZER_COMMON_DEFINITIONS)
 
-include(FindPkgConfig)
-pkg_check_modules(TIRPC libtirpc)
 if (TIRPC_FOUND)
   include_directories(${TIRPC_INCLUDE_DIRS})
   set(CMAKE_REQUIRED_INCLUDES ${TIRPC_INCLUDE_DIRS})
 endif()
 
 include(CheckIncludeFile)
 cmake_push_check_state()
 string(REPLACE "-nodefaultlibs" "" CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
 append_have_file_definition(rpc/xdr.h HAVE_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
 cmake_pop_check_state()
 
 set(SANITIZER_CFLAGS ${SANITIZER_COMMON_CFLAGS})
 append_rtti_flag(OFF SANITIZER_CFLAGS)
 
 append_list_if(SANITIZER_LIMIT_FRAME_SIZE -Wframe-larger-than=570
                SANITIZER_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WGLOBAL_CONSTRUCTORS_FLAG -Wglobal-constructors
                SANITIZER_CFLAGS)

test/msan/lit.cfg

 # -*- Python -*-
 
 import os
 
 # Setup config name.
 config.name = 'MemorySanitizer' + getattr(config, 'name_suffix', 'default')
 
 # Setup source root.
 config.test_source_root = os.path.dirname(__file__)
 
 # Setup default compiler flags used with -fsanitize=memory option.
 clang_msan_cflags = (["-fsanitize=memory",
                       "-mno-omit-leaf-frame-pointer",
                       "-fno-omit-frame-pointer",
                       "-fno-optimize-sibling-calls"] +
                       [config.target_cflags] +
-                      config.debug_info_flags)
+                      config.debug_info_flags +
+                      [config.rpc_cflags])
 # Some Msan tests leverage backtrace() which requires libexecinfo on FreeBSD.
 if config.host_os == 'FreeBSD':
   clang_msan_cflags += ["-lexecinfo"]
 clang_msan_cxxflags = config.cxx_mode_flags + clang_msan_cflags
 
 def build_invocation(compile_flags):
   return " " + " ".join([config.clang] + compile_flags) + " "
 

test/msan/lit.site.cfg.in

 @LIT_SITE_CFG_IN_HEADER@
 
 # Tool-specific config options.
 config.name_suffix = "-@CONFIG_NAME@"
 config.target_cflags = "@MSAN_TEST_TARGET_CFLAGS@"
 config.target_arch = "@MSAN_TEST_TARGET_ARCH@"
 config.use_lld = @MSAN_TEST_USE_LLD@
 config.use_thinlto = @MSAN_TEST_USE_THINLTO@
+config.rpc_cflags = "@TIRPC_CFLAGS@"
 
 # Load common config for all compiler-rt lit tests.
 lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
 
 # Load tool-specific config that would do the real work.
 lit_config.load_config(config, "@MSAN_LIT_SOURCE_DIR@/lit.cfg")

test/tsan/lit.cfg

 
 tsan_incdir = config.test_source_root + "/../"
 # Setup default compiler flags used with -fsanitize=thread option.
 clang_tsan_cflags = (["-fsanitize=thread",
                       "-Wall"] +
                       [config.target_cflags] +
                       config.debug_info_flags +
                       extra_cflags +
-                      ["-I%s" % tsan_incdir])
+                      ["-I%s" % tsan_incdir] +
+                      [config.rpc_cflags])
 clang_tsan_cxxflags = config.cxx_mode_flags + clang_tsan_cflags + ["-std=c++11"] + ["-I%s" % tsan_incdir]
 # Add additional flags if we're using instrumented libc++.
 # Instrumented libcxx currently not supported on Darwin.
 if config.has_libcxx and config.host_os != 'Darwin':
   # FIXME: Dehardcode this path somehow.
   libcxx_path = os.path.join(config.compiler_rt_obj_root, "lib",
                              "tsan", "libcxx_tsan_%s" % config.target_arch)
   libcxx_incdir = os.path.join(libcxx_path, "include", "c++", "v1")

test/tsan/lit.site.cfg.in

 @LIT_SITE_CFG_IN_HEADER@
 
 config.name_suffix = "@TSAN_TEST_CONFIG_SUFFIX@"
 config.tsan_lit_source_dir = "@TSAN_LIT_SOURCE_DIR@"
 config.has_libcxx = @TSAN_HAS_LIBCXX@
 config.ios = @TSAN_TEST_IOS_PYBOOL@
 config.iossim = @TSAN_TEST_IOSSIM_PYBOOL@
 config.target_cflags = "@TSAN_TEST_TARGET_CFLAGS@"
 config.target_arch = "@TSAN_TEST_TARGET_ARCH@"
+config.rpc_cflags = "@TIRPC_CFLAGS@"
 
 # Load common config for all compiler-rt lit tests.
 lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
 
 # Load tool-specific config that would do the real work.
 lit_config.load_config(config, "@CMAKE_CURRENT_SOURCE_DIR@/lit.cfg")