[ASan][Darwin] Fix `TestCases/Darwin/init_for_dlopen.cpp` when running in a standalone build.

[delcypher]

Summary

When compiler-rt is configured for a standalone build the
config.compiler_rt_libdir path isn't correct because the Clang used
for testing won't use the ASan runtime from this path.

During testing it seems that this bug only actually matters on Darwin for
the init_for_dlopen.cpp test case. The test fails because
the %shared_libasan expands to a path that doesn't exist. It doesn't
exist in this particular scenario because the build hasn't been run.
Even if the build were run %shared_libasan would still point to the
wrong path.

To fix this we query clang during lit setup to find out where the
clang being used for testing will get its ASan runtime from instead
of using config.compiler_rt_libdir.

Although this change could be made for other platforms we only make
it for Darwin for now to avoid accidently breaking testing on other
platforms.

This change should have no effect for in tree builds (the common case)
because config.compiler_rt_libdir and the output for `clang
-print-file-name=lib` agree.

rdar://77182297O

[aralisza]

a couple nits, but otherwise lgtm. deferring to Julian for review of functionality

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

57

how come this import is inside the function?

70

Is this python >= 3.6? What do you think about using f strings?

87

super nit picky, but what do you think about formatting this warning so it's easier to compare the two paths? something like

f"Path reported by clang != configured path\n\tclang path:\t{path}\n\tconfigured path:\t{config_crt_libdir_real}"

[delcypher]

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

57

To use the import only when it's needed. I don't have strong opinions here so I can make the import global if that's what you'd prefer.

70

According to https://llvm.org/docs/GettingStarted.html#software python 3.6 is the minimum version so we should be able to use f strings.

87

I'll try it.

[yln]

Would it be possible (and wouldn't it be better?) to set config.compiler_rt_libdir to the right path in the first place? This way we wouldn't get into the "path mismatch" state that we warn about.

I can see that compiler_rt_libdir is used in places (other than computing shared_libasan_path) as well.

First set by cmake compiler-rt/cmake/Modules/AddCompilerRT.cmake. Ideally, we would pick the right path there, right?

# Configure lit configuration files, including compiler-rt specific variables.
function(configure_compiler_rt_lit_site_cfg input output)
  set_llvm_build_mode()

  get_compiler_rt_output_dir(${COMPILER_RT_DEFAULT_TARGET_ARCH} output_dir)

  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} COMPILER_RT_RESOLVED_TEST_COMPILER ${COMPILER_RT_TEST_COMPILER})
  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} COMPILER_RT_RESOLVED_LIBRARY_OUTPUT_DIR ${output_dir})

  configure_lit_site_cfg(${input} ${output})
endfunction()

There also seems to be another config hook in compiler-rt/unittests/lit.common.unit.configured.in:

# LLVM tools dir and build mode can be passed in lit parameters,
# so try to apply substitution.
try:
  config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
  config.compiler_rt_libdir = config.compiler_rt_libdir % lit_config.params
  config.llvm_build_mode = config.llvm_build_mode % lit_config.params
except KeyError as e:
  key, = e.args
  lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key, key))

Can we use or change one of these existing mechanisms?

[delcypher]

In D101335#2720966, @yln wrote:

Would it be possible (and wouldn't it be better?) to set config.compiler_rt_libdir to the right path in the first place? This way we wouldn't get into the "path mismatch" state that we warn about.

I did look into this. I didn't go down that route because it's problematic.

I can see that compiler_rt_libdir is used in places (other than computing shared_libasan_path) as well.

First set by cmake compiler-rt/cmake/Modules/AddCompilerRT.cmake. Ideally, we would pick the right path there, right?

# Configure lit configuration files, including compiler-rt specific variables.
function(configure_compiler_rt_lit_site_cfg input output)
  set_llvm_build_mode()

  get_compiler_rt_output_dir(${COMPILER_RT_DEFAULT_TARGET_ARCH} output_dir)

  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} COMPILER_RT_RESOLVED_TEST_COMPILER ${COMPILER_RT_TEST_COMPILER})
  string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} COMPILER_RT_RESOLVED_LIBRARY_OUTPUT_DIR ${output_dir})

  configure_lit_site_cfg(${input} ${output})
endfunction()

So making the change at the CMake level has two problems.

1. If you're testing an externally built toolchain with a standalone compiler-rt configure (what I'm doing) then you risk accidentally borking the toolchain if you invoke a build. If COMPILER_RT_RESOLVED_LIBRARY_OUTPUT_DIR points into the externally built toolchain's resource directory then building the Sanitizers would overwrite the libraries in the toolchain. That doesn't seem desirable. What happens right now if that you can do a build of the Sanitizer runtimes and they just end up in the build directory but won't actually used by testing. This isn't great either but it at least it isn't modifying an external toolchain.

2. Changing this part of the CMake is incredibly risky because the way this code works differs between Apple and non-Apple platforms. Given that there's a much more targeted and lower risk option (i.e. this patch) I went for that instead.

There also seems to be another config hook in compiler-rt/unittests/lit.common.unit.configured.in:

# LLVM tools dir and build mode can be passed in lit parameters,
# so try to apply substitution.
try:
  config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
  config.compiler_rt_libdir = config.compiler_rt_libdir % lit_config.params
  config.llvm_build_mode = config.llvm_build_mode % lit_config.params
except KeyError as e:
  key, = e.args
  lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key, key))

Can we use or change one of these existing mechanisms?

I don't think we can use that mechanism. Looking at it more I don't know how anyone is supposed to use that. In order for this to work the strings like config.llvm_tools_dir would need to contain python string interpolation keys (e.g. %(some_key_name)s). These strings don't contain this so I'm not sure how that code ever worked.

Thinking about this more a more general solution would be to modify config.compiler_rt_libdir from test/lit.common.cfg.py if we are doing a standalone build and we are building using clang. We might still run into problems here though because some people might expect the tests to use libraries built as part of the standalone compiler-rt build (the builtins tests expect this) rather than the libraries in the external toolchain's resource directory (the sanitizer tests expect this).

So unfortunately going down this path is going to be messy.

[delcypher]

@yln Your comments have convinced me that I should try doing this a different way. I'll try the approach I outlined and I'll see how far I get.

[yln]

In D101335#2723210, @delcypher wrote:

@yln Your comments have convinced me that I should try doing this a different way. I'll try the approach I outlined and I'll see how far I get.

Thanks for considering other options. For the record: I don't know what this other path looks like. If it's not workable, then yes, let's move forward.
I just felt that "fixing up a path for a specific substitution" (when there are other uses) and the sense that we need a warning when the two config options diverged may give rise to future problems or confusion.

Changing this part of the CMake is incredibly risky because the way this code works differs between Apple and non-Apple platforms.

Maybe we can still guard the CMake change behind if (APPLE)?

[delcypher]

I'm abandoning this change. It is superseded by the much more general https://reviews.llvm.org/D101681

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

new_ld_library_path_32 = os.path.pathsep.join(
       (new_path, config.environment.get(dynamic_library_lookup_var, '')))
     config.environment[dynamic_library_lookup_var] = new_ld_library_path_32
 
     dynamic_library_lookup_var = 'LD_LIBRARY_PATH_64'
     new_ld_library_path_64 = os.path.pathsep.join(
       (new_path, config.environment.get(dynamic_library_lookup_var, '')))
     config.environment[dynamic_library_lookup_var] = new_ld_library_path_64
 
+def find_clang_compiler_rt_libdir(config):
+  import subprocess
+  clang_cmd = [
+    config.clang.strip(),
+    '-print-file-name=lib',
+  ]
+  try:
+    result = subprocess.run(
+      clang_cmd,
+      stdout=subprocess.PIPE,
+      stderr=subprocess.PIPE,
+      check=True
+    )
+  except subprocess.CalledProcessError as e:
+    msg = 'Failed to run {}\nrc:{}\nstdout:{}\nstderr:{}\n'.format(
+        clang_cmd,
+        e.returncode,
+        e.stdout,
+        e.stderr)
+    lit_config.fatal(msg)
+  path = result.stdout.decode().strip()
+  if not os.path.exists(path):
+    lit_config.fatal('Path reported by clang does not exist: {}'.format(path))
+  path = os.path.realpath(path)
+  if config.host_os == 'Darwin':
+    path = os.path.join(path, 'darwin')
+
+  config_crt_libdir_real = os.path.realpath(config.compiler_rt_libdir)
+  if path != config_crt_libdir_real:
+    # Note it's possible for this mismatch to occur when doing a
+    # Standalone build of compiler-rt.
+    lit_config.warning('Path reported by clang ({}) != configured path ({})'.format(
+      path, config_crt_libdir_real))
+  return path
+
 # Setup config name.
 config.name = 'AddressSanitizer' + config.name_suffix
 
 # Platform-specific default ASAN_OPTIONS for lit tests.
 default_asan_opts = list(config.default_sanitizer_opts)
 
 # On Darwin, leak checking is not enabled by default. Enable for x86_64
 # tests to prevent regressions

 config.substitutions.append( ("%clang ", build_invocation(target_cflags)) )
 config.substitutions.append( ("%clangxx ", build_invocation(target_cxxflags)) )
 config.substitutions.append( ("%clang_asan ", build_invocation(clang_asan_cflags)) )
 config.substitutions.append( ("%clangxx_asan ", build_invocation(clang_asan_cxxflags)) )
 if config.asan_dynamic:
   if config.host_os in ['Linux', 'FreeBSD', 'NetBSD', 'SunOS']:
     shared_libasan_path = os.path.join(config.compiler_rt_libdir, "libclang_rt.asan{}.so".format(config.target_suffix))
   elif config.host_os == 'Darwin':
-    shared_libasan_path = os.path.join(config.compiler_rt_libdir, 'libclang_rt.asan_{}_dynamic.dylib'.format(config.apple_platform))
+    shared_libasan_path = os.path.join(
+      find_clang_compiler_rt_libdir(config),
+      'libclang_rt.asan_{}_dynamic.dylib'.format(config.apple_platform))
   else:
     lit_config.warning('%shared_libasan substitution not set but dynamic ASan is available.')
     shared_libasan_path = None
 
   if shared_libasan_path is not None:
     config.substitutions.append( ("%shared_libasan", shared_libasan_path) )
   config.substitutions.append( ("%clang_asan_static ", build_invocation(clang_asan_static_cflags)) )
   config.substitutions.append( ("%clangxx_asan_static ", build_invocation(clang_asan_static_cxxflags)) )