Page MenuHomePhabricator

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

Authored by delcypher on Apr 30 2021, 5:17 PM.

Details

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

Diff Detail

Event Timeline

delcypher created this revision.Apr 30 2021, 5:17 PM
delcypher requested review of this revision.Apr 30 2021, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 5:17 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
delcypher added inline comments.Apr 30 2021, 6:07 PM
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 added inline comments.Apr 30 2021, 7:28 PM
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 updated this revision to Diff 342503.May 3 2021, 12:17 PM

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

phosek added a subscriber: ldionne.May 3 2021, 10:24 PM

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.

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 added inline comments.
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 updated this revision to Diff 344575.May 11 2021, 3:31 PM

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

delcypher edited the summary of this revision. (Show Details)May 11 2021, 3:32 PM

Ping.

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 added inline comments.May 13 2021, 12:57 PM
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 added inline comments.May 13 2021, 1:03 PM
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 accepted this revision.May 13 2021, 2:06 PM

LGTM

This revision is now accepted and ready to land.May 13 2021, 2:06 PM
yln accepted this revision.May 13 2021, 3:27 PM
yln added inline comments.
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 updated this revision to Diff 345580.May 14 2021, 3:48 PM
delcypher edited the summary of this revision. (Show Details)

Address @aralisza's feedback.

delcypher marked 3 inline comments as done.May 14 2021, 3:49 PM
delcypher added inline comments.May 14 2021, 4:21 PM
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 updated this revision to Diff 345593.May 14 2021, 5:58 PM

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

delcypher edited the summary of this revision. (Show Details)May 14 2021, 5:58 PM
delcypher marked 3 inline comments as done.May 14 2021, 6:01 PM
This revision was landed with ongoing or failed builds.May 14 2021, 6:11 PM
This revision was automatically updated to reflect the committed changes.

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.

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.

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.

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!