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

[Eric Fiselier via libcxx-commits]

On Fri, Nov 15, 2019, 8:27 PM Jan Wilken Dörrie via Phabricator <
reviews at reviews.llvm.org> wrote:

> jdoerrie added a comment.
>
> In D70343#1748522 <https://reviews.llvm.org/D70343#1748522>, @EricWF
> wrote:
>
> > 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.


This patch will turn on all the same _LIBCPP_ASSERT spellings as
_LIBCPP_DEBUG=0. When set to '=1' it turns on additional iterator debugging
checks. That said, adding a new hardening level for the purposes of
security (instead of debugging) is OK by me.

Can you expand on why efficient termination is important?

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.

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


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


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];
>     }
> ```
>
>
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D70343/new/
>
> https://reviews.llvm.org/D70343
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20191115/79e0008c/attachment.html>