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

[Chris Palmer via libcxx-commits]

Reviving this. Thanks for your patience.

I've updated the diff <https://reviews.llvm.org/D70343>, and I'll run some
object code size tests now and post what I find.

On Wed, Nov 27, 2019 at 7:43 AM Jan Wilken Dörrie <jdoerrie at google.com>
wrote:

>
>
> 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/20200508/a3f152ea/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3850 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20200508/a3f152ea/attachment.bin>