[Compiler-rt] Distinguish between testing just built runtime libraries and the libraries shipped with the compiler.

[delcypher]

Summary

The path to the runtime libraries used by the compiler under test
is normally identical to the path where just built libraries are
created. However, this is not necessarily the case when doing standalone
builds. This is because the external compiler used by tests may choose
to get its runtime libraries from somewhere else.

When doing standalone builds there are two types of testing we could be
doing:

• Test the just built runtime libraries.
• Test the runtime libraries shipped with the compile under test.

Both types of testing are valid but it confusingly turns out compiler-rt
actually did a mixture of these types of testing.

• The test/builtins/Unit/ test suite always tested the just built runtime libraries.
• All other testsuites implicitly use whatever runtime library the compiler decides to link.

There is no way for us to infer which type of testing the developer
wants so this patch introduces a new
COMPILER_RT_TEST_STANDALONE_BUILD_LIBS CMake
option which explicitly declares which runtime libraries should be
tested. If it is ON then the just built libraries should be tested,
otherwise the libraries in the external compiler should be tested.

When testing starts the lit test suite queries the compiler used for
testing to see where it will get its runtime libraries from. If these
paths are identical no action is taken (the common case). If the paths
are not identical then we check the value of
COMPILER_RT_TEST_STANDALONE_BUILD_LIBS (progated into the config as
test_standalone_build_libs) and check if the test suite supports testing in the
requested configuration.

• If we want to test just built libs and the test suite supports it (currently only test/builtins/Unit) then testing proceeds without any changes.
• If we want to test the just built libs and the test suite doesn't support it we emit a fatal error to prevent the developer from testing the wrong runtime libraries.
• If we are testing the compiler's built libs then we adjust config.compiler_rt_libdir to point at the compiler's runtime directory. This makes the test/builtins/Unit tests use the compiler's builtin library. No other changes are required because all other testsuites implicitly use the compiler's built libs.

To make the above work the
test_suite_supports_overriding_runtime_lib_path test suite config
option has been introduced so we can identify what each test suite
supports.

Note all of these checks have to be performed when lit runs.
We cannot run the checks at CMake generation time because
multi-configuration build systems prevent us from knowing what the
paths will be.

We could perhaps support COMPILER_RT_TEST_STANDALONE_BUILD_LIBS being
ON for most test suites (when the runtime library paths differs) in
the future by specifiying a custom compiler resource directory path.
Doing so is out of scope for this patch.

rdar://77182297

[delcypher]

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

54

@phosek I'd be curious how you think we should handle other platforms. If LLVM_ENABLE_PER_TARGET_RUNTIME_DIR wasn't a thing for Linux this would be just be something like.

if config.host_os == 'Linux':
  path = os.path.join(path, 'linux')

but LLVM_ENABLE_PER_TARGET_RUNTIME_DIR makes this more complicated.

[delcypher]

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

54

Hmm so doing something like clang --target <target_triple> -print-runtime-dir would actually help here. The problem is.

1. This flag is so new it doesn't exist in the compilers I'm currently trying to test.
2. It actually returns a completely incorrect path on Darwin. I've put up a patch for this https://reviews.llvm.org/D101682

I suppose I could have the current code as a fallback if -print-runtime-dir isn't supported, but otherwise use -print-runtime-dir.

[delcypher]

Use -print-runtime-dir to support non Darwin platforms.

[phosek]

Do we know for sure that people are using compiler-rt test suite to test runtime libraries shipped with their compiler? I'm a bit surprised by that since it's not a case I've ever considered. I'd prefer if we always used -nodefaultlibs to prevent that case altogether since compiler-rt build is already exceedingly complicated and we should ideally strive to reduce the number of modes and options we have to support.

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

54

I'd prefer to make LLVM_ENABLE_PER_TARGET_RUNTIME_DIR the default, and eventually the only mode. It's something we've already discussed with @ldionne and I'm planning on sending out an RFC proposing that. Deprecating non-LLVM_ENABLE_PER_TARGET_RUNTIME_DIR build might take a while so we may have to support both unfortunately for the foreseeable future.

[delcypher]

In D101681#2735499, @phosek wrote:

Do we know for sure that people are using compiler-rt test suite to test runtime libraries shipped with their compiler? I'm a bit surprised by that since it's not a case I've ever considered.

Yep. I'm using it. To make testing on various Apple platforms scale we are building the toolchain once and then distributing it to various testing machines that test the runtimes of the built toolchain. Each test machine is doing a standalone configure of compiler-rt without building anything and then running tests

I'd prefer if we always used -nodefaultlibs to prevent that case altogether since compiler-rt build is already exceedingly complicated and we should ideally strive to reduce the number of modes and options we have to support.

That's not how the test suites work today (apart from the builtins/Unit tests).

If you ignore the config.compiler_rt_libdir = compiler_libdir line added by this patch, then the remainder of this patch is actually restricting behaviour. If the compiler's runtime directory doesn't match the compiler-rt library directory then with this patch we actually prevent (by default) tests running with an error. Previously we just allowed testing to proceed even though we were testing the wrong runtime libraries. It just so happens that I do want to allow this "test the toolchain's runtime libraries" behaviour but I do not think it should be allowed by default. This is what the COMPILER_RT_TEST_BUILT_LIBS option does and why it is ON by default.

You're right that we could change the other test suites to pass -nodefaultlibs but I consider that out of scope for this patch. I am (mostly) trying to restrict behaviours that already existed. Should we need to support testing the just built libraries in a standalone build? Probably, but this doesn't work right now and I think it's much better for testing to immediately fail rather than silently testing the wrong thing. We could certainly look at changing the way the test suites find the runtime libraries in future patches.

Now the config.compiler_rt_libdir = compiler_libdir line is adding additional behaviour. This change

• Makes the builtin tests work when COMPILER_RT_TEST_BUILT_LIBS is set to OFF.
• Fixes an ASan test case when COMPILER_RT_TEST_BUILT_LIBS is set to OFF.

I technically could add this line as a separate patch but it was such a tiny change just doing that I thought it made more sense to do it in the same patch.

[delcypher]

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

54

As long as -print-runtime-dir continues to report the correct path when LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is on then I think the approach used in this patch should be okay.

I'm expecting we can remove the fallback for older Apple Clang in this patch in ~ a year from now.

Please CC me and @arphaman on that RFC when you send it. As long as the new path doesn't contain the architecture on Apple platforms then I guess the new path would be okay.

[delcypher]

Simplify patch because https://reviews.llvm.org/D101682 has landed.

[delcypher]

Ping.

[delcypher]

I tried out this patch on Linux with an in-tree build (tests run without failing the new check added) and an out of tree build (testing refuses to run as expected).

[aralisza]

compiler-rt/test/CMakeLists.txt

5

nit, is there a missing comma here? I had to read this a few times to parse it lol

When testing, use the just built runtime libraries instead of the compiler's.

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

30

do you think it'll be useful to include that the compiler is not supported yet here?

130

super nitpicky, but can you make it so the paths align to make it easier to compare? like

f'Compiler libdir:       {compiler_libdir}\n'
f'compiler-rt libdir:    {compiler_rt_libdir_real}')

136–143

I'm having a hard time understanding this message, does it mean something like

COMPILER_RT_TEST_BUILT_LIBS=ON, but this test suite does not support running using the just-built runtime libraries.
Either modify this test suite to support this test configuration, or set COMPILER_RT_TEST_BUILT_LIBS=OFF to test the runtime libraries included in the compiler instead.

147

should this be a warning? It seems like info to me

[delcypher]

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

30

I could add something in the message that includes config.compiler_id and mention that support isn't implemented.

130

Sure.

136–143

Yes that's exactly what it means. I'll update the patch to use your wording.

147

Oops. Yeah this shouldn't be a warning.

[phosek]

LGTM

[yln]

compiler-rt/test/CMakeLists.txt

5

Maybe name and phrase this as "test with standalone libs"?

I was initially confused what "test (with) built libs" meant. Usually (standard build), both the compiler and the runtime libs are "just built" and this distinction is only meaningful in the standalone case, right?

[delcypher]

Address @aralisza's feedback.

[delcypher]

compiler-rt/test/CMakeLists.txt

5

It's "just built" from the perspective if you had just run one of the testing targets (e.g.check-asan). That target would try to run tests but "just" before that the runtime libraries would be built. In the standalone case these are the libraries that would be most recently built compared to what the external compiler is using (they have to be older).

You're right though. This name isn't great. I was going to say this patch only matters for standalone builds but that's not quite accurate.

This patch matters when Clang and the compiler-rt disagree on the path to the runtime libraries. AFAIK this can only happen in standalone builds but it also needs to be a standalone build where Clang and compiler-rt disagree on the path. It turns out for the "llvm/runtimes" style standalone build provides a value for COMPILER_RT_OUTPUT_DIR that should result in Clang and compiler-rt actually agreeing. So saying this patch always matters in standalone builds isn't quite accurate.

Perhaps a name like COMPILER_RT_TEST_STANDALONE_BUILT_LIBS with a description like

When doing testing in a standalone build, test the runtime libraries built by this standalone build rather than the runtime libraries shipped with the compiler (used for testing). This option has no effect if the compiler and this build are configured to use the same runtime library path.

[delcypher]

Rename option name and lit variables to address @yln 's feedback.

[hubert.reinterpretcast]

I understand this patch is trying to expose unintended configurations (thanks!). I'll note that it might not be the friendliest patch to land on a Friday.
Please see the state of the sanitizer-ppc64le-linux bot: https://lab.llvm.org/buildbot/#/builders/19.

[delcypher]

In D101681#2761503, @hubert.reinterpretcast wrote:

I understand this patch is trying to expose unintended configurations (thanks!). I'll note that it might not be the friendliest patch to land on a Friday.
Please see the state of the sanitizer-ppc64le-linux bot: https://lab.llvm.org/buildbot/#/builders/19.

Sorry about this. I'm going to land a patch now to downgrade the fatal error to warning to unbreak the bots for now. Owner's of the bot should investigate this warning though because it points to the testing configuration being wrong.

[delcypher]

In D101681#2761522, @delcypher wrote:

In D101681#2761503, @hubert.reinterpretcast wrote:

I understand this patch is trying to expose unintended configurations (thanks!). I'll note that it might not be the friendliest patch to land on a Friday.
Please see the state of the sanitizer-ppc64le-linux bot: https://lab.llvm.org/buildbot/#/builders/19.

Sorry about this. I'm going to land a patch now to downgrade the fatal error to warning to unbreak the bots for now. Owner's of the bot should investigate this warning though because it points to the testing configuration being wrong.

@hubert.reinterpretcast Landed 7085cd2f2945b1e73e1c66f78080c48cba9112e8 which downgrades the check to a warning. Hopefully this should unbreak the sanitizer-ppc64le-linux bot.

[hubert.reinterpretcast]

In D101681#2761525, @delcypher wrote:

In D101681#2761522, @delcypher wrote:

In D101681#2761503, @hubert.reinterpretcast wrote:

I understand this patch is trying to expose unintended configurations (thanks!). I'll note that it might not be the friendliest patch to land on a Friday.
Please see the state of the sanitizer-ppc64le-linux bot: https://lab.llvm.org/buildbot/#/builders/19.

Sorry about this. I'm going to land a patch now to downgrade the fatal error to warning to unbreak the bots for now. Owner's of the bot should investigate this warning though because it points to the testing configuration being wrong.

Yes, I understand. I've contacted the people who I believe will be looking after the bot's configuration.

@hubert.reinterpretcast Landed 7085cd2f2945b1e73e1c66f78080c48cba9112e8 which downgrades the check to a warning. Hopefully this should unbreak the sanitizer-ppc64le-linux bot.

Thank you!

[delcypher]

I've landed b7f60d861ad7a8d03c09c0b72277eabaa53731fb to try to unbreak the likely misconfigured https://lab.llvm.org/buildbot/#/builders/119/builds/3801 bot.

[MaskRay]

I don't understand what COMPILER_RT_TEST_STANDALONE_BUILD_LIBS is, but find that this patch may be related to making
-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on clang -shared-libsan linked executables work on Linux: D108859, where I reuse the clang -print-runtime-dir logic.

compiler-rt/test/CMakeLists.txt

 # Needed for lit support in standalone builds.
 include(AddLLVM)
 
+option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
+  "When set to ON and testing in a standalone build, test the runtime \
+  libraries built by this standalone build rather than the runtime libraries \
+  shipped with the compiler (used for testing). When set to OFF and testing \
+  in a standalone build, test the runtime libraries shipped with the compiler \
+  (used for testing). This option has no effect if the compiler and this \
+  build are configured to use the same runtime library path."
+  ON)
+pythonize_bool(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS)
+
 pythonize_bool(LLVM_ENABLE_EXPENSIVE_CHECKS)
 configure_compiler_rt_lit_site_cfg(
   ${CMAKE_CURRENT_SOURCE_DIR}/lit.common.configured.in
   ${CMAKE_CURRENT_BINARY_DIR}/lit.common.configured)
 
 # BlocksRuntime (and most of builtins) testsuites are not yet ported to lit.
 # add_subdirectory(BlocksRuntime)
 

compiler-rt/test/builtins/Unit/lit.site.cfg.py.in

 @LIT_SITE_CFG_IN_HEADER@
 
 config.name_suffix = "@BUILTINS_TEST_CONFIG_SUFFIX@"
 config.builtins_lit_source_dir = "@BUILTINS_LIT_SOURCE_DIR@/Unit"
 config.target_cflags = "@BUILTINS_TEST_TARGET_CFLAGS@"
 config.target_arch = "@BUILTINS_TEST_TARGET_ARCH@"
 config.is_msvc = @MSVC_PYBOOL@
 config.builtins_is_msvc = @BUILTINS_IS_MSVC_PYBOOL@
 config.builtins_lit_source_features = "@BUILTINS_LIT_SOURCE_FEATURES@"
+config.test_suite_supports_overriding_runtime_lib_path = True
 
 # 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, "@BUILTINS_LIT_SOURCE_DIR@/Unit/lit.cfg.py")

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

 # it's not available.
 try:
   import shlex
   sh_quote = shlex.quote
 except:
   import pipes
   sh_quote = pipes.quote
 
+def find_compiler_libdir():
+  """
+    Returns the path to library resource directory used
+    by the compiler.
+  """
+  if config.compiler_id != 'Clang':
+    lit_config.warning(f'Determining compiler\'s runtime directory is not supported for {config.compiler_id}')
+    # TODO: Support other compilers.
+    return None
+  def get_path_from_clang(args, allow_failure):
+    clang_cmd = [
+      config.clang.strip(),
+      f'--target={config.target_triple}',
+    ]
+    clang_cmd.extend(args)
+    path = None
+    try:
+      result = subprocess.run(
+        clang_cmd,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        check=True
+      )
+      path = result.stdout.decode().strip()
+    except subprocess.CalledProcessError as e:
+      msg = f'Failed to run {clang_cmd}\nrc:{e.returncode}\nstdout:{e.stdout}\ne.stderr{e.stderr}'
+      if allow_failure:
+        lit_config.warning(msg)
+      else:
+        lit_config.fatal(msg)
+    return path
+
+  # Try using `-print-runtime-dir`. This is only supported by very new versions of Clang.
+  # so allow failure here.
+  runtime_dir = get_path_from_clang(['-print-runtime-dir'], allow_failure=True)
+  if runtime_dir:
+    if os.path.exists(runtime_dir):
+      return os.path.realpath(runtime_dir)
+    lit_config.fatal(f'Path reported by clang does not exist: {runtime_dir}')
+
+  # Fall back for older AppleClang that doesn't support `-print-runtime-dir`
+  # Note `-print-file-name=<path to compiler-rt lib>` was broken for Apple
+  # platforms so we can't use that approach here (see https://reviews.llvm.org/D101682).
+  if config.host_os == 'Darwin':
+    lib_dir = get_path_from_clang(['-print-file-name=lib'], allow_failure=False)
+    runtime_dir = os.path.join(lib_dir, 'darwin')
+    if not os.path.exists(runtime_dir):
+      lit_config.fatal(f'Path reported by clang does not exist: {runtime_dir}')
+    return os.path.realpath(runtime_dir)
+
+  lit_config.warning('Failed to determine compiler\'s runtime directory')
+  return None
+
+
 # Choose between lit's internal shell pipeline runner and a real shell.  If
 # LIT_USE_INTERNAL_SHELL is in the environment, we use that as an override.
 use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")
 if use_lit_shell:
     # 0 is external, "" is default, and everything else is internal.
     execute_external = (use_lit_shell == "0")
 else:
     # Otherwise we default to internal on Windows and external elsewhere, as

 elif compiler_id == 'GNU':
   config.cxx_mode_flags = ["-x c++"]
   config.debug_info_flags = ["-g"]
 else:
   lit_config.fatal("Unsupported compiler id: %r" % compiler_id)
 # Add compiler ID to the list of available features.
 config.available_features.add(compiler_id)
 
+# Ask the compiler for the path to libraries it is going to use. If this
+# doesn't match config.compiler_rt_libdir then it means we might be testing the
+# compiler's own runtime libraries rather than the ones we just built.
+# Warn about about this and handle appropriately.
+compiler_libdir = find_compiler_libdir()
+if compiler_libdir:
+  compiler_rt_libdir_real = os.path.realpath(config.compiler_rt_libdir)
+  if compiler_libdir != compiler_rt_libdir_real:
+    lit_config.warning(
+      'Compiler lib dir != compiler-rt lib dir\n'
+      f'Compiler libdir:     "{compiler_libdir}"\n'
+      f'compiler-rt libdir:  "{compiler_rt_libdir_real}"')
+    if config.test_standalone_build_libs:
+      # Use just built runtime libraries, i.e. the the libraries this built just built.
+      if not config.test_suite_supports_overriding_runtime_lib_path:
+        # Test suite doesn't support this configuration.
+        lit_config.fatal(
+            'COMPILER_RT_TEST_STANDALONE_BUILD_LIBS=ON, but this test suite '
+            'does not support testing the just-built runtime libraries '
+            'when the test compiler is configured to use different runtime '
+            'libraries. Either modify this test suite to support this test '
+            'configuration, or set COMPILER_RT_TEST_STANDALONE_BUILD_LIBS=OFF '
+            'to test the runtime libraries included in the compiler instead.'
+        )
+    else:
+      # Use Compiler's resource library directory instead.
+      config.compiler_rt_libdir = compiler_libdir
+    lit_config.note(f'Testing using libraries in "{config.compiler_rt_libdir}"')
+
 # If needed, add cflag for shadow scale.
 if config.asan_shadow_scale != '':
   config.target_cflags += " -mllvm -asan-mapping-scale=" + config.asan_shadow_scale
 if config.memprof_shadow_scale != '':
   config.target_cflags += " -mllvm -memprof-mapping-scale=" + config.memprof_shadow_scale
 
 config.environment = dict(os.environ)
 

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

 set_default("use_newpm", False)
 set_default("android", @ANDROID_PYBOOL@)
 set_default("android_api_level", "@ANDROID_API_LEVEL@")
 set_default("android_serial", "@ANDROID_SERIAL_FOR_TESTING@")
 set_default("android_files_to_push", [])
 set_default("have_rpc_xdr_h", @HAVE_RPC_XDR_H@)
 set_default("gwp_asan", @COMPILER_RT_HAS_GWP_ASAN_PYBOOL@)
 set_default("expensive_checks", @LLVM_ENABLE_EXPENSIVE_CHECKS_PYBOOL@)
+set_default("test_standalone_build_libs", @COMPILER_RT_TEST_STANDALONE_BUILD_LIBS_PYBOOL@)
+# True iff the test suite supports ignoring the test compiler's runtime library path
+# and using `config.compiler_rt_libdir` instead. This only matters when the runtime
+# library paths differ.
+set_default("test_suite_supports_overriding_runtime_lib_path", False)
 config.available_features.add('target-is-%s' % config.target_arch)
 
 if config.enable_per_target_runtime_dir:
   set_default("target_suffix", "")
 elif config.android:
   if config.target_arch == "i386":
     set_default("target_suffix", "-i686-android")
   else: