Skip to content

[libc++][hardening] Add a greppable prefix to assertion messages. #150560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 31, 2025
+4 −4
Merged

[libc++][hardening] Add a greppable prefix to assertion messages. #150560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libcxx/include/__assert
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
#define _LIBCPP_ASSERT(expression, message) \
(__builtin_expect(static_cast<bool>(expression), 1) \
? (void)0 \
: _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING(__LINE__) ": assertion " _LIBCPP_TOSTRING( \
expression) " failed: " message "\n"))
: _LIBCPP_ASSERTION_HANDLER(__FILE__ ":" _LIBCPP_TOSTRING( \
__LINE__) ": libc++ Hardening assertion " _LIBCPP_TOSTRING(expression) " failed: " message "\n"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very open to suggestions about how exactly to word the prefix. The ++ part in libc++ is slightly annoying since it needs escaping in the regex (no big deal, just a slight annoyance) -- personally, I'd lean towards using libcxx for that reason, but happy with anything, really.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess libc++ will be more familiar to end-users than libcxx. Would it also make sense to use h instead of H in Hardening? I can imagine most people will just type hardening assertion or hardening.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback!

To be clear about the use case, the main intent is to support people turning on the upcoming observe mode and deliberately "grepping" (whichever actual tool they use) their logs for libc++ Hardening assertions. I presume we will document the prefix and they will be searching for a specific string -- for me at least, the intent is not to make our Hardening stand out in general. Of course, we should still strive to make the prefix readable and reasonable.

I guess libc++ will be more familiar to end-users than libcxx.

This is a good point.

Would it also make sense to use h instead of H in Hardening?

I don't feel super strongly about it, but recently I've been deliberately trying to find a way to make it clearer that we use the word "hardening" in a very specific sense, as the name of our specific mechanism, and not just in the general sense of making things more robust and secure. Capitalization is supposed to stress that this refers to a specific feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we intend for _LIBCPP_ASSERT to only ever be used for hardening?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we intend for _LIBCPP_ASSERT to only ever be used for hardening?

I think it depends how you think about it. Many existing assertions are technically not really "hardening" in the sense of "security hardening". But since we've decided to classify all of the assertions and simply discriminate them based on the mode selected by the user, we consider all of them to be part of "Hardening" (as in, the libc++ extension).

So because of that, yes I think that _LIBCPP_ASSERT will always be used for "libc++ Hardening" related checks (since by definition any check we might add would fall into it), but not all of them are true "security hardening" in the general sense of it. True "security hardening" checks would be closer to the ones included in the fast (and maybe extensive) modes, probably without any check that is purely pedantic in nature.

Copy link
Contributor

@Zingam Zingam Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@var-const Maybe use LIBCPP as a prefix, which would be more consistent with libc++ naming conventions instead of your original "libcxx" alternative prefix? Just as the hardening macros' prefix themselves. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a consistent naming convention, unfortunately. Internal macros use the LIBCPP prefix like you said, but CMake variables (which are public-facing) use LIBCXX, and also the name of the directory within the LLVM repo is libcxx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re. libc++ vs. some other spelling: I'd prefer libc++. You can simply grep for Hardening assertion if the pluses are too much for you. I doubt there is much of a clash with other libraries, and if there is, you probably care about these other library diagnostics as well. If you don't then oh well, you might have to escape them. It's not the end of the world.

I think I'm happy with the current message if we rename _LIBCPP_ASSERT to make it clear that it's an implementation detail of hardening assertions (in a separate patch). That wasn't clear to me until now.

Copy link
Member

@ldionne ldionne Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, what I would do is something like this:

// This is the old _LIBCPP_ASSERT macro, we probably don't want to rename
// it but I'm using this new name here to clarify the intent
#define _LIBCPP_CONTRACT_VIOLATION(expression, message)                                                                \
  (__builtin_expect(static_cast<bool>(expression), 1)                                                                  \
       ? (void)0                                                                                                       \
       : _LIBCPP_ASSERTION_HANDLER(message))

#define _LIBCPP_HARDENING_ASSERT(expression, message)                                                                  \
     _LIBCPP_CONTRACT_VIOLATION(expression,                                                                            \
          __FILE__ ":" _LIBCPP_TOSTRING(__LINE__)                                                                      \
          ": assertion " _LIBCPP_TOSTRING(expression) " failed: " message "\n")

So basically, we have:

  1. A mechanism to set the "contract violation handler" (which we call the "assertion handler"). That is not tied to libc++ Hardening specifically
  2. A macro to trigger a "contract violation" (that is currently called _LIBCPP_ASSERT)
  3. A macro that triggers a contract violation originating from a libc++ Hardening assertion (that's what we'd now call _LIBCPP_HARDENING_ASSERT)

That way, we clarify what is a Hardening assertion but we still retain a general purpose mechanism for setting (and talking about) contract violations which should be forward compatible if we add non-Hardening related assertions.

Connecting the current vendor-controlled way of defining a custom "contract violation handler aka assertion handler" is going to be a bit challenging but I think it's doable, hopefully while staying backwards compatible. Otherwise it's possible that vendors will need to change their stuff a little bit but it should be pretty self-contained.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I really like this way of looking at it -- an assertion implements a contract violation, and Hardening happens to be the only part of the library that is currently using this. I'll do a follow-up to implement this.


// WARNING: __builtin_assume can currently inhibit optimizations. Only add assumptions with a clear
// optimization intent. See https://discourse.llvm.org/t/llvm-assume-blocks-optimization/71609 for a
Expand Down
4 changes: 2 additions & 2 deletions libcxx/test/support/check_assertion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ MatchResult MatchAssertionMessage(const std::string& text, std::string_view expe
// library.
std::string assertion_format_string = [&] {
if (use_marker)
return (".*###\\n(.*):(\\d+): assertion (.*) failed: (.*)\\n###");
return ("(.*):(\\d+): assertion (.*) failed: (.*)\\n");
return (".*###\\n(.*):(\\d+): libc\\+\\+ Hardening assertion (.*) failed: (.*)\\n###");
return ("(.*):(\\d+): libc\\+\\+ Hardening assertion (.*) failed: (.*)\\n");
}();
std::regex assertion_format(assertion_format_string);

Expand Down
Loading