[libc++][test] fix redefinition of _LIBCPP_HARDENING_MODE

[Jannik2099]

-D_LIBCPP_HARDENING_MODE may be set in clang config files or through CXXFLAGS

This was the case for us on Gentoo.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Jannik Glückert (Jannik2099)

Changes

-D_LIBCPP_HARDENING_MODE may be set in clang config files or through CXXFLAGS

This was the case for us on Gentoo.

---

Full diff: https://github.com/llvm/llvm-project/pull/109330.diff

1 Files Affected:

• (modified) libcxx/utils/libcxx/test/params.py (+1)

diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index 13c7297fd7304f..732300232c79cf 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -392,6 +392,7 @@ def getSuitableClangTidy(cfg):
         actions=lambda hardening_mode: filter(
             None,
             [
+                AddCompileFlag("-U_LIBCPP_HARDENING_MODE")                                  if hardening_mode != "undefined" else None,
                 AddCompileFlag("-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_NONE")      if hardening_mode == "none" else None,
                 AddCompileFlag("-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_FAST")      if hardening_mode == "fast" else None,
                 AddCompileFlag("-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_EXTENSIVE") if hardening_mode == "extensive" else None,

[ldionne]

-D_LIBCPP_HARDENING_MODE may be set in clang config files

In what files?

or through CXXFLAGS

I don't understand, how would they make it in the test suite because of CXXFLAGS (I assume you're referring to the environment variable of that name)?

I don't fully understand why you need this change yet, but my immediate reaction is that we don't want to make this change. Indeed, we want to test as closely possible to the way users will use the library, and that makes us move away from that. For instance, this could potentially hide a bug where we incorrectly define _LIBCPP_HARDENING_MODE when hardening_mode is not specified, and we wouldn't find out about this.

[Jannik2099]

In what files?

/etc/clang/foo.cfg

This is how we apply -D_LIBCPP_HARDENING_MODE on gentoo.

how would they make it in the test suite because of CXXFLAGS

I had assumed that the compiler flags passed to cmake would also propagate to the lit tests, but admittedly I haven't tested that.

As it is now, tests are unrunnable out of the box with our system config.

[ldionne]

In what files?

/etc/clang/foo.cfg

This is how we apply -D_LIBCPP_HARDENING_MODE on gentoo.

How does that work? I am not familiar with these kinds of files. Is there a mechanism to tweak the default Clang flags using files like that?

It looks like what you're trying to do is enable hardening by default on Gentoo overall. Is that right? The correct way of doing that is to set -DLIBCXX_HARDENING_MODE=<mode> when configuring the library with CMake. That will end up with the appropriate definitions in __config_site and the test suite can then be run with e.g. hardening_mode=fast and everything should work.

[Jannik2099]

How does that work? I am not familiar with these kinds of files. Is there a mechanism to tweak the default Clang flags using files like that?

It just contains a string of arguments that get added to each clang invocation, with some rudimentary selection logic (e.g. clang --target=x86_64... will load x86_64....cfg). It's how we apply things like default-PIE, stack protector and whatnot.

The equivalent in gcc would be spec files, which are much more powerful in their conditional logic.

the correct way of doing that is to set -DLIBCXX_HARDENING_MODE= when configuring the library with CMake

I'm trying to find an answer to why we didn't do that. In general the sentiment around setting default behaviour has been "don't submit it as a new cmake toggle if a config file would do the job", perhaps -DLIBCXX_HARDENING_MODE predates this or has been added later.

I've pinged the people that configured this back then in Gentoo, either they have a good reason that's still valid today, or we'll just switch to the cmake toggle.

[MaskRay]

Historically, there are CMake options like CLANG_DEFAULT_RTLIB/CLANG_DEFAULT_PIE_ON_LINUX that customize certain default cflags/linker options. These options make CMake build system more complex and from the perspective of clangDriver maintenance, we are encouraging Clang configuration files. (There are several posts like https://blogs.gentoo.org/mgorny/2022/10/07/clang-in-gentoo-now-sets-default-runtimes-via-config-file/)

It seems that Jannik2099 has -DLIBCXX_HARDENING_MODE=<mode> in the default Clang configuration file, which conflicts with this libc++ setting.

One idea is to set the environment variable CLANG_NO_DEFAULT_CONFIG, used by clang/test/lit.cfg.py and other lit tests. The other is to set -U....

[ldionne]

Ack, thanks for the context. I'm not sure what to say other than repeat that the supported way to set the default hardening mode in libc++ is to set -DLIBCXX_HARDENING_MODE=<mode> when configuring the library, which ends up generating the __config_site accordingly. You could also generate __config_site yourself, but I'd recommend against that since that'll be one additional custom thing to maintain.

We could also look into moving the whole __config_site mechanism to a different mechanism like these configuration files used by Clang, but I'm not certain there's a lot of value to be gained (for libc++) from doing that.

I'm going to tentatively close this since I don't think we want to move forward with this approach as-is, but feel free to reopen if there's more discussion to be had. I'm mostly trying to keep our (long!) review queue tidy.

[Jannik2099]

I'm going to tentatively close this since I don't think we want to move forward with this approach as-is

Yeah, I agree with your rationale.

We'll probably just switch to the cmake option on our end, cheers!