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

[Chris Palmer via libcxx-commits]

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.

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