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

[Dan Liew via Phabricator via llvm-commits]

delcypher added inline comments.


================
Comment at: compiler-rt/test/lit.common.cfg.py:30
+  if config.compiler_id != 'Clang':
+    lit_config.warning('Failed to determine compiler\'s runtime directory')
+    # TODO: Support other compilers.
----------------
aralisza wrote:
> do you think it'll be useful to include that the compiler is not supported yet here?
I could add something in the message that includes `config.compiler_id` and mention that support isn't implemented.


================
Comment at: compiler-rt/test/lit.common.cfg.py:130
+      'Compiler lib dir != compiler-rt lib dir\n'
+      f'Compiler libdir: {compiler_libdir}\n'
+      f'compiler-rt lib dir: {compiler_rt_libdir_real}')
----------------
aralisza wrote:
> 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}')
> ```
Sure.


================
Comment at: compiler-rt/test/lit.common.cfg.py:136-143
+        lit_config.fatal(
+            'The compiler being used for testing is not using runtime '
+            'libraries created by this build but testing is configured to test '
+            'the built libraries (COMPILER_RT_TEST_BUILT_LIBS is ON). '
+            'Testing cannot proceed. To fix this modify this test suite to support this testing configuration.'
+            'Alternatively set COMPILER_RT_TEST_BUILT_LIBS to OFF to test the '
+            'runtime libraries of the compiler instead.'
----------------
aralisza wrote:
> 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.
Yes that's exactly what it means. I'll update the patch to use your wording.


================
Comment at: compiler-rt/test/lit.common.cfg.py:147
+      config.compiler_rt_libdir = compiler_libdir
+    lit_config.warning(f'Testing using libraries in {config.compiler_rt_libdir}')
+
----------------
aralisza wrote:
> should this be a warning? It seems like info to me
Oops. Yeah this shouldn't be a warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101681/new/

https://reviews.llvm.org/D101681