Page MenuHomePhabricator

[libc++] Allow specifying custom Lit config files
ClosedPublic

Authored by ldionne on Jun 15 2020, 8:47 AM.

Details

Summary

Before this patch, the libc++ test suite first loads lit.site.cfg
(generated by CMake), and then lit.cfg. It's also possible to load
lit.cfg before lit.site.cfg and to point to a custom lit.site.cfg
file using '--param=libcxx_site_config'. However, in that case, lit.cfg
still relies on the site configuration filling up the 'config' object
like the default lit.site.cfg file does, which isn't flexible enough.

This commit simplifies the setup by having just a single Lit site config
file per CMake configuration, and always loading exactly that config file.
However, the config file to use can be selected when setting up CMake via
the LIBCXX_TEST_CONFIG setting. Furthermore, the site configs are entirely
standalone, which means that a new site config can be added that doesn't
need to conform what's expected by config.py.

Diff Detail

Event Timeline

ldionne created this revision.Jun 15 2020, 8:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 8:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Adding reviewers whom I think might be impacted by this change.

jwakely resigned from this revision.Jun 15 2020, 4:19 PM

I don't think this impacts anything I do with lit (which isn't very much anyway).

Thanks for this patch. This definitely gets rid of one of the most annoying shortcomings we have at the moment, which is the need to run lit directly when we want to use a custom config.
So with this patch, if I want to use a custom config, I would simply copy-paste all the settings I care about from lit.site.cfg.in, add my own configuration logic and I'd be ready to go?

I would be okay with this as this solves my exact use-case, even though in a slightly different manner. I'm not a huge fan of having to copy-paste the config I'm interested in, because then I'd be concerned that the default config might change without me noticing. Would it be possible to load the default config from a custom config or is copy-pasting the stuff we're interested in the recommended way of setting up tests in this case?

By the way, I know this isn't directly related to this patch, but is there a minimum list of required configuration parameters for the new format?

Thanks for this patch. This definitely gets rid of one of the most annoying shortcomings we have at the moment, which is the need to run lit directly when we want to use a custom config.
So with this patch, if I want to use a custom config, I would simply copy-paste all the settings I care about from lit.site.cfg.in, add my own configuration logic and I'd be ready to go?

Basically. However, with the new format, you don't really need that much boilerplate. Look at e.g. libcxx/test/configs/libcxx-trunk-shared.cfg.in in this patch: https://reviews.llvm.org/D81866.

I would be okay with this as this solves my exact use-case, even though in a slightly different manner. I'm not a huge fan of having to copy-paste the config I'm interested in, because then I'd be concerned that the default config might change without me noticing. Would it be possible to load the default config from a custom config or is copy-pasting the stuff we're interested in the recommended way of setting up tests in this case?

Ideally we wouldn't need to copy-paste a lot of stuff. It would help me to better understand your specific setup. How do you configure the test suite? Is there some code I can see somewhere?

By the way, I know this isn't directly related to this patch, but is there a minimum list of required configuration parameters for the new format?

It's documented in the new test format here: https://github.com/llvm/llvm-project/blob/master/libcxx/utils/libcxx/test/newformat.py#L139. Basically, you need to provide the following Lit substitutions and you're all done:

%{cxx}           - A command that can be used to invoke the compiler
%{compile_flags} - Flags to use when compiling a test case
%{link_flags}    - Flags to use when linking a test case
%{flags}         - Flags to use either when compiling or linking a test case
%{exec}          - A command to prefix the execution of executables
STL_MSFT added inline comments.Jun 16 2020, 6:13 PM
libcxx/test/lit.cfg.py
8–11

I think we started running lit directly as of https://github.com/microsoft/STL/pull/710 so this will affect us - but I am not yet deeply familiar with this area. (Not an objection to your change, thanks for the heads up!)

Ideally we wouldn't need to copy-paste a lot of stuff. It would help me to better understand your specific setup. How do you configure the test suite? Is there some code I can see somewhere?

Unfortunately I can't show code (still need to convince management to open-source our backend 😄 ), but I should be able to go into just-enough details about our concrete use-case and challenges we are facing. Basically we have the following use-case: we have a downstream bare-metal target and we want to use the existing libcxx regression test suite to check whether our target-specific code (and obviously our compiler too) is working as intended. For that we need to cross-compile (managed to do that by creating a TargetInfo object) and remotely execute the tests on hardware. For executing a cross-compiled binary on hardware we have a script which does all of the heavy lifting for us.

So basically I simply need a minimum configuration which cross-compiles for my target and then uses our custom script to execute the cross-compiled binary on our hardware. We also have multiple architectures, but I can use the given target info object to determine which one and set compile and executions flags accordingly.

Since I didn't know which configuration parameters are actually required, I solved my use-case by implementing the ability to load an additional configuration, where I set up the substitutions and then it finally started working.

At this moment, our implementation doesn't support exception handling yet, so ideally I would like to disable all exception handling tests. Furthermore, I saw that there were some tests which rely on GNU extensions, which are not available in our C library. And also our C library has some quirks which makes it that we can't support some of the tests (e.g. include_as_c.sh.cpp or extern_c.pass.cpp) so we need to exclude all of those as well. I think that should be doable with a lit.local.cfg file which sets config.unsupported = True when running the tests for our target I guess? I'd rather not go through every single test and add a // UNSUPPORTED: ... to them.

Another challenge I'm facing at the moment is that we haven't implemented LLD support, so we are relying on an in-house linker, which we simply invoke from the driver. This linker is requires the libraries to be set in a certain order and with certain flags or otherwise we end up with undefined references. Is it problematic for the tests if I hard-code the linker flags, including libc++ and libc++abi in our configuration? I noticed that the test suite adds them to linker flags, but obviously not in a fashion that works for us.

Then, the final problem is that I have to disable warnings (config.enable_warnings = False) or otherwise the test suite adds a flag, which the compiler doesn't understand. I find it peculiar this happens as this flag is supposedly only added when it is supported by the compiler. So it would seem to me that either the wrong compiler is tested for this flag or that the check is not working. The flag in question is -Wno-aligned-allocation-unavailable and is added through this piece of code: self.cxx.addWarningFlagIfSupported('-Wno-aligned-allocation-unavailable').

By the way, I know this isn't directly related to this patch, but is there a minimum list of required configuration parameters for the new format?

It's documented in the new test format here: https://github.com/llvm/llvm-project/blob/master/libcxx/utils/libcxx/test/newformat.py#L139. Basically, you need to provide the following Lit substitutions and you're all done:

%{cxx}           - A command that can be used to invoke the compiler
%{compile_flags} - Flags to use when compiling a test case
%{link_flags}    - Flags to use when linking a test case
%{flags}         - Flags to use either when compiling or linking a test case
%{exec}          - A command to prefix the execution of executables

Oh, okay then! That is a lot less than I would have expected, given how much settings the old format required (or at least creates). In that case I really don't need the ability to load additional configurations along with the default one. When using a custom configuration, can the test suite still figure out to use the compiler that was used to build libc++ even if I don't add a substitution for %{cxx}? Or would I simply set it to "@LIBCXX_COMPILER@" and the CMake magic in this patch would fill in the right one?

STL_MSFT added inline comments.Jun 17 2020, 4:13 PM
libcxx/test/lit.cfg.py
8–11

We generate an stl-lit.py script so this shouldn't break microsoft/STL. Yay!

Ideally we wouldn't need to copy-paste a lot of stuff. It would help me to better understand your specific setup. How do you configure the test suite? Is there some code I can see somewhere?

Unfortunately I can't show code (still need to convince management to open-source our backend 😄 ), but I should be able to go into just-enough details about our concrete use-case and challenges we are facing. Basically we have the following use-case: we have a downstream bare-metal target and we want to use the existing libcxx regression test suite to check whether our target-specific code (and obviously our compiler too) is working as intended. For that we need to cross-compile (managed to do that by creating a TargetInfo object) and remotely execute the tests on hardware. For executing a cross-compiled binary on hardware we have a script which does all of the heavy lifting for us.

So basically I simply need a minimum configuration which cross-compiles for my target and then uses our custom script to execute the cross-compiled binary on our hardware. We also have multiple architectures, but I can use the given target info object to determine which one and set compile and executions flags accordingly.

Since I didn't know which configuration parameters are actually required, I solved my use-case by implementing the ability to load an additional configuration, where I set up the substitutions and then it finally started working.

Understood. The changes I made to the testing format were meant exactly to make this sort of use case easier, via the small number of configurations that are needed for the tests to run.

At this moment, our implementation doesn't support exception handling yet, so ideally I would like to disable all exception handling tests.

Does your compiler support -fno-exceptions? If so, just use --param enable_exceptions=False when running Lit and you're golden.

Furthermore, I saw that there were some tests which rely on GNU extensions, which are not available in our C library.

Which ones?

And also our C library has some quirks which makes it that we can't support some of the tests (e.g. include_as_c.sh.cpp or extern_c.pass.cpp) so we need to exclude all of those as well. I think that should be doable with a lit.local.cfg file which sets config.unsupported = True when running the tests for our target I guess? I'd rather not go through every single test and add a // UNSUPPORTED: ... to them.

I believe it might become useful to support specifying a set of UNSUPPORTED and/or XFAIL annotations externally to the test suite itself. This way, you could add additional annotations out of tree without risking merge conflicts or adding a lot of complexity. You'd have to maintain that out-of-tree list when we add new tests, but that's the case whichever way we decide to do it.

Another challenge I'm facing at the moment is that we haven't implemented LLD support, so we are relying on an in-house linker, which we simply invoke from the driver. This linker is requires the libraries to be set in a certain order and with certain flags or otherwise we end up with undefined references. Is it problematic for the tests if I hard-code the linker flags, including libc++ and libc++abi in our configuration? I noticed that the test suite adds them to linker flags, but obviously not in a fashion that works for us.

No, it shouldn't be an issue if you hardcode stuff. What's the problem you're seeing with adding your flags to %{link_flags}?

Then, the final problem is that I have to disable warnings (config.enable_warnings = False) or otherwise the test suite adds a flag, which the compiler doesn't understand. I find it peculiar this happens as this flag is supposedly only added when it is supported by the compiler. So it would seem to me that either the wrong compiler is tested for this flag or that the check is not working. The flag in question is -Wno-aligned-allocation-unavailable and is added through this piece of code: self.cxx.addWarningFlagIfSupported('-Wno-aligned-allocation-unavailable').

Your compiler doesn't support -Wno-aligned-allocation-unavailable? Does it fail when one tries to use that option?

By the way, I know this isn't directly related to this patch, but is there a minimum list of required configuration parameters for the new format?

It's documented in the new test format here: https://github.com/llvm/llvm-project/blob/master/libcxx/utils/libcxx/test/newformat.py#L139. Basically, you need to provide the following Lit substitutions and you're all done:

%{cxx}           - A command that can be used to invoke the compiler
%{compile_flags} - Flags to use when compiling a test case
%{link_flags}    - Flags to use when linking a test case
%{flags}         - Flags to use either when compiling or linking a test case
%{exec}          - A command to prefix the execution of executables

Oh, okay then! That is a lot less than I would have expected, given how much settings the old format required (or at least creates). In that case I really don't need the ability to load additional configurations along with the default one.

Nice.

When using a custom configuration, can the test suite still figure out to use the compiler that was used to build libc++ even if I don't add a substitution for %{cxx}? Or would I simply set it to "@LIBCXX_COMPILER@" and the CMake magic in this patch would fill in the right one?

A %{cxx} substitution needs to be provided to the test format. There's no magic. If you set %{cxx} to @CMAKE_CXX_COMPILER@, however, it'll get substituted at CMake generation time, like you would expect.

Since it looks like this is sufficient for your use case, I'll go forward with this patch.

ldionne accepted this revision as: Restricted Project.Jun 18 2020, 7:05 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 18 2020, 7:35 AM
This revision was automatically updated to reflect the committed changes.
gargaroff added a comment.EditedJun 19 2020, 12:26 AM

Does your compiler support -fno-exceptions? If so, just use --param enable_exceptions=False when running Lit and you're golden.

We have disabled exceptions through CMake (LIBCXX_ENABLE_EXCEPTIONS=OFF) which also sets config.enable_exceptions to False but we still see many many unresolved references errors (more than 500) for _Unwind_* functions (we don't provide libunwind yet). Also, at first we also disabled RTTI but still saw that all? most? just some select few? (didn't do a full investigation) of the RTTI tests were still running, so I was unsure whether the same was true for exception handling tests.

Furthermore, I saw that there were some tests which rely on GNU extensions, which are not available in our C library.

Which ones?

Sorry, I mixed up POSIX and GNU :)
Anyway, I see that some tests are using the debug_mode_helper.h which includes sys/wait.h. As far as I understand that's a POSIX header and is not available for our target (as well as all other POSIX-only headers).

Speaking of available features. There's another thing that just popped into my head: our C library uses the ISO-8859-X charmap and does not support UTF-8. Will we need to disable locale tests? When implementation our TargetInfo object I had to implement a function which tests whether some locales are supported and all of them where UTF-8 locales.

I believe it might become useful to support specifying a set of UNSUPPORTED and/or XFAIL annotations externally to the test suite itself. This way, you could add additional annotations out of tree without risking merge conflicts or adding a lot of complexity. You'd have to maintain that out-of-tree list when we add new tests, but that's the case whichever way we decide to do it.

Yes, that would indeed be a nice-to-have feature for us! I think it would make sense to have this as part of the config. I'm thinking something like config.unsupported_tests = [] and config.xfail_tests = [] which would take a list of paths (maybe even globs? or regex?). And since it would be part of the config, you could even make this dynamic.

No, it shouldn't be an issue if you hardcode stuff. What's the problem you're seeing with adding your flags to %{link_flags}?

No no, we didn't have any problem with adding it to %{link_flags}. We just wondered if it *would* be problematic for some tests if we did.

Your compiler doesn't support -Wno-aligned-allocation-unavailable? Does it fail when one tries to use that option?

Yes, clang complains about it being an unrecognized flag and exits right away.
By the way, setting config.enable_warnings = False has the nice side-effect that libcxx/selftest/newformat/pass.cpp/werror.pass.cpp and libcxx/selftest/newformat/sh.cpp/werror.sh.cpp are unexpectedly passing because -Werror is not passed as a flag anymore.

Since it looks like this is sufficient for your use case, I'll go forward with this patch.

Looking forward to using it! Thanks for the effort.

P.S. Since this is now quickly going to off-topic from this patch, we should probably move the discussion somewhere else again.

Is there any docs on the canonical way of excuting the libcxx testsuite against an installed compiler and its C++ standard library (without compiling libc++ at all)? Before this change, I had a custom lit.site.cfg for that, and just ran lit.py with LIBCXX_SITE_CONFIG set pointing at the config. My current workaround is a small custom script similar to the generated bin/llvm-lit, that bootstraps things - which also seems to be what @STL_MSFT pointed out that they do in their repo. It'd be great to have that usecase somewhat documented though, as I'm afraid the setup will break soon again as long as it's a non-documented setup.