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

[Jan Wilken Dörrie via libcxx-commits]

On Wed, Nov 27, 2019 at 12:57 AM Chris Palmer <palmer at google.com> wrote:

> On Sat, Nov 16, 2019 at 8:59 AM Eric Fiselier via libcxx-commits <
> libcxx-commits at lists.llvm.org> wrote:
>
> Can you expand on why efficient termination is important?
>>
>
> In the commit message I meant "fast" as in "fail fast"; the actual time
> taken for the process to terminate is not super important to us (Chromium).
> But we do care about object code size quite a bit. I think (perhaps I am
> wrong) that __builtin_trap gets us smaller code than std::exit.
>
> Also, just from poking around, it seems that _LIBCPP_DEBUG=0 does not turn
> on asserts in all places. Random example
> from ./libcxx/test/std/strings/basic.string/string.iterators/db_iterators_7.pass.cpp:
>
> ```
> #if _LIBCPP_DEBUG >= 1
>
> #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : std::exit(0))
> ```
>
> I.e. 0 would not do it.
>
> And, as far as I can tell, _LIBCPP_DEBUG only takes effect in files under
> libcxx/test/ — I take it this means the defines do nothing in production
> release builds. What I want is this callee hardening in production builds.
> If I understand correctly — and I'm still super new to this code base — the
> _LIBCPP_ASSERT calls are where I want them in production, and with
> _LIBCPP_HARDEN defined, they'll do their thing.
>

Looking at the source code we can see the following: __config
<https://github.com/llvm/llvm-project/blob/703c26f03be74daf6e483380e6b23029a3851081/libcxx/include/__config#L873>
sets _LIBCPP_DEBUG_LEVEL
to 1 once you pass _LIBCPP_DEBUG=0. This in turn makes __debug
<https://github.com/llvm/llvm-project/blob/939544add98ee6463d6abd6c28fa6c9ac4b6e104/libcxx/include/__debug#L32>
define _LIBCPP_ASSERT
and invoke _VSTD::__libcpp_debug_function in the failure case. debug.cpp
<https://github.com/llvm/llvm-project/blob/939544add98ee6463d6abd6c28fa6c9ac4b6e104/libcxx/src/debug.cpp#L33-L39>
then
sets this to __libcpp_abort_debug_function by default, which logs an error
and invokes std::abort. Given that most headers
<https://github.com/search?q=%22%23include+%5C%3C__debug%5C%3E%22+repo%3Allvm%2Fllvm-project+path%3Alibcxx%2Finclude%2F&type=Code>
already
#include <__debug>, the macro should already be effective in production
code, assuming you pass _LIBCPP_DEBUG=0.

I guess I'd be curious to know if there is a meaningful binary size win in
introducing  _LIBCPP_HARDEN, or if we could just rely on _LIBCPP_DEBUG=0.

>
> I've been meaning to add a mechanism to use `__builtin_trap` (or
>> `__builtin_unreachable` when UBSAN is enabled) instead of
>> `__libcpp_debug_function` for various reasons.
>>
>> However, for the purposes of testing, we need to maintain the
>> `__libcpp_debug_function` and ensure all _LIBCPP_ASSERT's can be made to
>> use it to report failure.
>>
>
> So should I update the patch to change the definition of
> __libcpp_debug_function when _LIBCPP_HARDEN is defined?
>
>
>>
>>
>> ================
>>> Comment at: libcxx/include/string_view:282
>>> +    const_reference operator[](size_type __pos) const _NOEXCEPT {
>>> +      return _LIBCPP_ASSERT(size() == 0 || __pos < size(),
>>> "string_view[] index out of bounds"), __data[__pos];
>>> +    }
>>> ----------------
>>> 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.
>>>
>>
>> I agree. the size() == 0 check is incorrect. this should do the same
>> bounds checking as at(pos).
>>
>
> Did that, yep.
>
>
>> Given that string_view implies C++17 or newer, you should also be able to
>>> use multiple expressions in a constexpr function.
>>
>>
>> libc++ enable string view in all dialects.
>>
>
> So should I go back to the previous ,-expression implementation? :)
>
> Thanks for the review!
>

Yes, given that C++11 only allows a single return expression in constexpr
functions you should revert it to the previous version. I didn't know that
libc++ defines string_view for all C++ dialects, since the standard only
defined it in C++17 and onwards this was surprising to me.

PS: These email conversations don't show up for me in the Differential UI,
is this intended?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20191127/03d6e3e2/attachment.html>