Differential D70343
Add a `_LIBCPP_HARDEN` define Authored by palmer on Nov 15 2019, 2:54 PM.
Details This define allows builders to opt in to a version of _LIBCPP_ASSERT that Builders who do not define _LIBCPP_HARDEN, and builders who use Also, add an assert in constexpr string_view::operator[].
Diff Detail Event TimelineComment Actions What's the difference between _LIBCPP_HARDEN and simply defining _LIBCPP_DEBUG=0? That should turn on _LIBCPP_ASSERT in the same cases. Comment Actions I believe the intent of _LIBCPP_HARDEN is to turn asserts into program terminations. Ideally this happens as quickly as possible with as little overhead as possible. Thus we directly call __builtin_trap() instead of going through _VSTD::__libcpp_debug_function. Furthermore, I believe we were under the impression that defining _LIBCPP_DEBUG=0 turns on other checks as well, that we are not necessarily interested in. Please correct me if I'm wrong.
Comment Actions We had some discussion in email, but it didn't show up here. New patch coming up, using the return-expression form, as discussed. Comment Actions LGTM, but I would still like to know whether we actually need this in addition to _LIBCPP_DEBUG=0. As far as I can tell setting _LIBCPP_DEBUG to 0 makes _LIBCPP_ASSERT(x, m) be equivalent to ((x) ? (void)0 : abort_fun(x, m)), where abort_fun is effectively the following: void abort_fun(condition, message) {
std::fprintf(stderr, "__FILE__: __LINE__: _LIBCPP_ASSERT ${condition} failed. ${message}\n");
std::abort();
}Maybe that is good enough for our purposes? Comment Actions Thanks! Yes, I am still testing the binary size question. I hope to get back to you on that this week, and in any case won't land this until I have got some answer. There's also the question of the meaning of "harden" vs. "debug", and "debug = 0". I believe that "harden" has a more clear meaning than "debug = 0", which looks like it means "don't debug; optimize for production". or, at most, "do debug, but minimally". If I were a random build engineer setting up the build for some product, I would never guess that "debug = 0" means to turn on hardening checks.
Comment Actions Here are some binary size numbers, from a Chromium release build. Here is the build configuration (likely meaningful only to Chromium people; but this basically means "fully optimized except for the final PGO phase"): ~/chromium/src $ cat out/Release/args.gn origin/master: With libcxx-debug-level-0: With libcxx-debug-level-0 with __builtin_trap instead of printing: So the cost is still high, but significantly less with __builtin_trap. So I think this mode is valuable.
Comment Actions Thanks for the analysis, I agree that those args.gn make sense :)
Comment Actions I'm a bit confused by the build size numbers you gave compared to the patch. There's a confusing distinction in libc++ between _LIBCPP_DEBUG_LEVEL and _LIBCPP_DEBUG. ` Also, this patch has weird behavior? If I set _LIBCPP_DEBUG=0 and _LIBCPP_HARDEN I don't get the __builtin_trap implementation. Could we address the compile time by making __libcpp_debug_function overrideable as a macro?
Comment Actions
Yes; ignore the inaccurate name of my local git branch. That branch contains just: ~/chromium/src $ git show libcxx-debug-level-0
commit 7d791ab6746b1d6176407e735fd4727ff59c3b89 (libcxx-debug-level-0)
Author: Chris Palmer <palmer@chromium.org>
Date: Fri May 8 12:46:35 2020 -0700
DO NOT LAND. Turn on _LIBCPP_DEBUG=0 unconditionally.
diff --git a/build/config/c++/BUILD.gn b/build/config/c++/BUILD.gn
index 9787a0b982dd..d783d31232d9 100644
--- a/build/config/c++/BUILD.gn
+++ b/build/config/c++/BUILD.gn
@@ -67,11 +67,12 @@ config("runtime_library") {
# libc++ has two levels of debug mode. Setting _LIBCPP_DEBUG to zero
# enables most assertions. Setting it to one additionally enables iterator
# debugging. See https://libcxx.llvm.org/docs/DesignDocs/DebugMode.html
- if (enable_iterator_debugging) {
- defines += [ "_LIBCPP_DEBUG=1" ]
- } else if (is_debug || dcheck_always_on) {
- defines += [ "_LIBCPP_DEBUG=0" ]
- }
+ #if (enable_iterator_debugging) {
+ # defines += [ "_LIBCPP_DEBUG=1" ]
+ #} else if (is_debug || dcheck_always_on) {
+ # defines += [ "_LIBCPP_DEBUG=0" ]
+ #}
+ defines += [ "_LIBCPP_DEBUG=0" ]
}
if (is_win) {
That is what I expect. _LIBCPP_HARDEN should be appropriate for fully optimized release builds, which to me means leaving _LIBCPP_DEBUG undefined. Defining _LIBCPP_DEBUG means (to me) that the builder does want some debugging, i.e. some friendly printing.
It's not about compile time, it's about object code size. But, sure, anything that works to get the object code size down at least as much as this patch does is fine by me. I'm not married to this approach. (Although I would like to get something landed soonish, so that we can try using it in Chromium, so hopefully we don't need a fundamental re-think.) Comment Actions Overall I've been wanting to do something like this, and I agree that assert != hardening checks. Some assertions might be hardening checks, but many assertions are there to verify that the libc++ implementors didn't mess up, or to verify expensive things about usage of libc++. Hardening is only there to verify inexpensive problems which might lead to security issues. Therefore there's some overlap, but it's not the same. | ||||||||||||||||||||||||||||||
Checking that size() == 0 seems like a mistake, as otherwise you would allow arbitrary indices for an empty string_view. This should just be _LIBCPP_ASSERT(__pos < size(), "..."); unless I'm missing something obvious.
Given that string_view implies C++17 or newer, you should also be able to use multiple expressions in a constexpr function. IMO the following is easier to read:
_LIBCPP_CONSTEXPR _LIBCPP_INLINE_VISIBILITY const_reference operator[](size_type __pos) const _NOEXCEPT { _LIBCPP_ASSERT(__pos < size(), "string_view[] index out of bounds"); return __data[__pos]; }