[libcxx-commits] [PATCH] D81846: [libc++] Allow specifying custom Lit config files

Thu Jun 18 07:34:50 PDT 2020

ldionne added a comment.

In D81846#2097652 <https://reviews.llvm.org/D81846#2097652>, @gargaroff wrote:

> In D81846#2095851 <https://reviews.llvm.org/D81846#2095851>, @ldionne wrote:
>
> > 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81846