Page MenuHomePhabricator

Add a `_LIBCPP_HARDEN` define
AbandonedPublic

Authored by palmer on Nov 15 2019, 2:54 PM.

Details

Summary

This define allows builders to opt in to a version of _LIBCPP_ASSERT that
crashes fast in production builds, with the smallest possible object code.

Builders who do not define _LIBCPP_HARDEN, and builders who use
_LIBCPP_DEBUG_LEVEL >= 1, will see no change.

Also, add an assert in constexpr string_view::operator[].

Diff Detail

Event Timeline

palmer created this revision.Nov 15 2019, 2:54 PM

What's the difference between _LIBCPP_HARDEN and simply defining _LIBCPP_DEBUG=0? That should turn on _LIBCPP_ASSERT in the same cases.

What's the difference between _LIBCPP_HARDEN and simply defining _LIBCPP_DEBUG=0? That should turn on _LIBCPP_ASSERT in the same cases.

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.

libcxx/include/string_view
282

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];
}
palmer marked an inline comment as done.Nov 26 2019, 2:14 PM

Yes, jdoerrie has the motivation right. Thanks!

palmer updated this revision to Diff 231135.Nov 26 2019, 2:18 PM

Respond to comments.

We had some discussion in email, but it didn't show up here. New patch coming up, using the return-expression form, as discussed.

palmer updated this revision to Diff 262928.May 8 2020, 12:10 PM

Going back to the return-expression form.

jdoerrie accepted this revision.EditedMay 12 2020, 1:21 AM

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?

This revision is now accepted and ready to land.May 12 2020, 1:21 AM

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.

palmer edited the summary of this revision. (Show Details)May 13 2020, 10:05 AM
EricWF requested changes to this revision.May 13 2020, 10:35 AM
EricWF added inline comments.
libcxx/include/string_view
282

string_view doesn't imply C++17. We support it in all dialects.

This revision now requires changes to proceed.May 13 2020, 10:35 AM

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
is_debug = false
is_official_build = true
is_asan = false
use_goma = true
enable_nacl = false
is_component_build = false

origin/master:
-rwxr-xr-x 1 palmer primarygroup 162973344 May 12 17:30 out/Release/chrome

With libcxx-debug-level-0:
-rwxr-xr-x 1 palmer primarygroup 169825792 May 12 18:20 out/Release/chrome
Cost: 6,852,448 (4.2% of 162,973,344)

With libcxx-debug-level-0 with __builtin_trap instead of printing:
-rwxr-xr-x 1 palmer primarygroup 167307088 May 13 10:08 out/Release/chrome
Cost: 4,333,744 (2.6% of 162,973,344)

So the cost is still high, but significantly less with __builtin_trap. So I think this mode is valuable.

palmer marked an inline comment as done.May 13 2020, 11:07 AM
palmer added inline comments.
libcxx/include/string_view
282

So should I switch back to the previous 2-statement form, again? :)

jdoerrie marked an inline comment as done.May 13 2020, 11:40 AM

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
is_debug = false
is_official_build = true
is_asan = false
use_goma = true
enable_nacl = false
is_component_build = false

origin/master:
-rwxr-xr-x 1 palmer primarygroup 162973344 May 12 17:30 out/Release/chrome

With libcxx-debug-level-0:
-rwxr-xr-x 1 palmer primarygroup 169825792 May 12 18:20 out/Release/chrome
Cost: 6,852,448 (4.2% of 162,973,344)

With libcxx-debug-level-0 with __builtin_trap instead of printing:
-rwxr-xr-x 1 palmer primarygroup 167307088 May 13 10:08 out/Release/chrome
Cost: 4,333,744 (2.6% of 162,973,344)

So the cost is still high, but significantly less with __builtin_trap. So I think this mode is valuable.

Thanks for the analysis, I agree that those args.gn make sense :)

libcxx/include/string_view
282

No need, I believe Eric is referring to my initial 2 line suggestion. The one line solution is required to be compatible with C++11, where constexpr functions could only consist of a single return statement.

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. `
_LIBCPP_DEBUG_LEVEL is always one more that _LIBCPP_DEBUG. So libcpp-debug-level-0, shouldn't enable anything,
But I assume you correctly set _LIBCPP_DEBUG to zero.

Also, this patch has weird behavior? If I set _LIBCPP_DEBUG=0 and _LIBCPP_HARDEN I don't get the __builtin_trap implementation.
I think the idea of hardening and the want to make the assertion cheaper to compile should be dealt with separately.

Could we address the compile time by making __libcpp_debug_function overrideable as a macro?

libcxx/include/string_view
282

That works for me.

There's a confusing distinction in libc++ between _LIBCPP_DEBUG_LEVEL and _LIBCPP_DEBUG. `
_LIBCPP_DEBUG_LEVEL is always one more that _LIBCPP_DEBUG. So libcpp-debug-level-0, shouldn't enable anything,
But I assume you correctly set _LIBCPP_DEBUG to zero.

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) {

Also, this patch has weird behavior? If I set _LIBCPP_DEBUG=0 and _LIBCPP_HARDEN I don't get the __builtin_trap implementation.

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.

I think the idea of hardening and the want to make the assertion cheaper to compile should be dealt with separately.

Could we address the compile time by making __libcpp_debug_function overrideable as a macro?

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.)

jfb added a comment.May 26 2020, 9:21 AM

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.

palmer abandoned this revision.Mon, Oct 5, 5:07 PM

I'll start a new revision to add the missing string_view assert.