[libcxx-commits] [PATCH] D115398: [libcxx] Use LIBCXX_EXECUTOR, LIBCXX_TEST_COMPILER_FLAGS and LIBCXX_TEST_LINKER_FLAGS in new test configs

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/configs/cmake-bridge.cfg.in:33-34
+config.substitutions.append(('%{executor}', '@LIBCXX_EXECUTOR@'))
+config.substitutions.append(('%{user_compile_flags}', '@LIBCXX_TEST_COMPILER_FLAGS@'))
+config.substitutions.append(('%{user_link_flags}', '@LIBCXX_TEST_LINKER_FLAGS@'))
----------------
mstorsjo wrote:
> ldionne wrote:
> > About these two -- I would actually love to get rid of them. Instead, you could create your own simple `.cfg.in` file for MingGW where you set the required flags.
> > 
> > I loathe forwarding flags from CMake to the Lit config, IMO that's just unnecessary plumbing.
> I know you're not a fan of such settings, but I find them vital. I, on the other hand, loathe setups where it's only possible to build and run things in a finite, fixed set of configurations, instead of passing individual options for configuring one's build.
> 
> Sure we can only test and support a finite, fixed set of configurations, but it should be possible for vendors to build and test in other configs by just configuring things with additional flags on top of the existing configs, as long as it's not different enough warranting a separate config.
> 
> As a very concrete example; for the clang-cl builds, we currently build and test with setting `-DCMAKE_CXX_FLAGS="-D_LIBCPP_HAS_NO_INT128" -DLIBCXX_TEST_COMPILER_FLAGS="-D_LIBCPP_HAS_NO_INT128"`. I've considered extending it so we'd test both in the default configuration (with filesystem disabled) and this config (with int128 disabled so that msvc environments can test filesystem). That would require adding a separate test config file for that too.
> 
> And when such a detail is configured by adding things to `CMAKE_CXX_FLAGS`, it's best if the corresponding flag for tests can be passed in tandem right next to it, via `LIBCXX_TEST_COMPILER_FLAGS` to cmake, instead of using a separate test config for it.
Needing to pass additional compiler flags to the test suite is always a way to hack around some deficiency somewhere else. For instance, `_LIBCPP_HAS_NO_INT128` comes from:

```
# TODO: Clang-cl in MSVC configurations don't have access to compiler_rt
# builtins helpers for int128 division. See
# https://reviews.llvm.org/D91139#2429595 for a comment about longterm
# intent for handling the issue. In the meantime, define
# -D_LIBCPP_HAS_NO_INT128 (both when building the library itself and
# when building tests) to allow enabling filesystem for running tests,
# even if it uses a non-permanent ABI.
```

If we have a proper option that is not a hack, then it should be supported 1st class in `features.py` or in `params.py`. I don't want to bloat the number of base substitutions required to run the libc++ test suite in order to facilitate doing hacks -- I'm fine with these hacks remaining in the state of "being a temporary hack".



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115398