[libcxx-commits] [PATCH] D70343: Add a `_LIBCPP_HARDEN` define

[Chris Palmer via Phabricator via libcxx-commits]

palmer added a comment.

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


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

https://reviews.llvm.org/D70343