<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 27, 2019 at 12:57 AM Chris Palmer <<a href="mailto:palmer@google.com">palmer@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Sat, Nov 16, 2019 at 8:59 AM Eric Fiselier via libcxx-commits <<a href="mailto:libcxx-commits@lists.llvm.org" target="_blank">libcxx-commits@lists.llvm.org</a>> wrote:<br></div><div dir="ltr"><br></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div>Can you expand on why efficient termination is important?<br></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>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:</div><div><br></div><div>```</div><div>#if _LIBCPP_DEBUG >= 1<br><br>#define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : std::exit(0))<br></div><div>```</div><div><br></div><div>I.e. 0 would not do it.</div><div><br></div><div>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.</div></div></div></div></blockquote><div><br></div><div>Looking at the source code we can see the following: <a href="https://github.com/llvm/llvm-project/blob/703c26f03be74daf6e483380e6b23029a3851081/libcxx/include/__config#L873">__config</a> sets _LIBCPP_DEBUG_LEVEL to 1 once you pass _LIBCPP_DEBUG=0. This in turn makes <a href="https://github.com/llvm/llvm-project/blob/939544add98ee6463d6abd6c28fa6c9ac4b6e104/libcxx/include/__debug#L32">__debug</a> define _LIBCPP_ASSERT and invoke _VSTD::__libcpp_debug_function in the failure case. <a href="https://github.com/llvm/llvm-project/blob/939544add98ee6463d6abd6c28fa6c9ac4b6e104/libcxx/src/debug.cpp#L33-L39">debug.cpp</a> then sets this to __libcpp_abort_debug_function by default, which logs an error and invokes std::abort. Given that <a href="https://github.com/search?q=%22%23include+%5C%3C__debug%5C%3E%22+repo%3Allvm%2Fllvm-project+path%3Alibcxx%2Finclude%2F&type=Code">most headers</a> already #include <__debug>, the macro should already be effective in production code, assuming you pass _LIBCPP_DEBUG=0.</div><div><br></div><div>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.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div></div><div dir="auto">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. <br></div><div dir="auto"><br></div><div dir="auto">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.</div></div></blockquote><div><br></div><div>So should I update the patch to change the definition of __libcpp_debug_function when _LIBCPP_HARDEN is defined?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"> </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
================<br>
Comment at: libcxx/include/string_view:282<br>
+    const_reference operator[](size_type __pos) const _NOEXCEPT {<br>
+      return _LIBCPP_ASSERT(size() == 0 || __pos < size(), "string_view[] index out of bounds"), __data[__pos];<br>
+    }<br>
----------------<br>
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.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I agree. the size() == 0 check is incorrect. this should do the same bounds checking as at(pos).</div></div></blockquote><div><br></div><div>Did that, yep.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Given that string_view implies C++17 or newer, you should also be able to use multiple expressions in a constexpr function.</blockquote></div></div><div dir="auto"><br></div><div dir="auto">libc++ enable string view in all dialects.</div></div></blockquote><div><br></div><div>So should I go back to the previous ,-expression implementation? :)</div><div><br></div><div>Thanks for the review!</div></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>PS: These email conversations don't show up for me in the Differential UI, is this intended?  </div></div></div>