[libcxx-commits] [PATCH] D89353: Enable overriding `__libcpp_debug_function` invocation

[Chris Palmer via Phabricator via libcxx-commits]

palmer added inline comments.


================
Comment at: libcxx/docs/DesignDocs/DebugMode.rst:113
   * ``std::list``
+  * ``std::locale``
   * ``std::unordered_map``
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > In what sense does `std::locale` support iterators?
> I should add: Other than this `std::locale` diff which I don't understand (yet?), I love this documentation change; feel free to land it separately from the rest of this PR.
The implementation in include/__locale includes some iterators. If you have turned on this debugging mode, those iterators will be checked. Nothing directly API-exposed, but under the hood, debug checks could fire. I figure people might want to know.


================
Comment at: libcxx/include/__debug:42
 #   define _LIBCPP_DEBUG_ASSERT(x, m) _LIBCPP_ASSERT(x, m)
-#   define _LIBCPP_ASSERT_IMPL(x, m) ((x) ? (void)0 : _VSTD::__libcpp_debug_function(_VSTD::__libcpp_debug_info(__FILE__, __LINE__, #x, m)))
+#   define _LIBCPP_ASSERT_IMPL(x, m) ((x) ? (void)0 : _LIBCPP_DEBUG_FUNCTION_INVOCATION((x), m))
 #else
----------------
thakis wrote:
> Quuxplusone wrote:
> > (A) Please use `#x` here instead of `(x)` (and then `x` instead of `#x` in the macro above). This preserves the current assertion messages without extra parens. I don't think there's any reason it's a big deal; but we have a choice between "add an extra set of parens" or "don't", and I think "don't" is clearly if microscopically better.
> > 
> > (B) Even after reading D70343, I don't get the point of this new abstraction. Is it so that you can hard-code `#define _LIBCPP_DEBUG_FUNCTION_INVOCATION std::abort()` at compile time instead of setting `__libcpp_debug_function = std::abort;` at runtime? Why is the latter unacceptable to you?
> > Also, are you aware that libc++ compiles some `_LIBCPP_ASSERT` checks into its libc++.a binary? — Failures in those checks will respect the runtime setting of `__libcpp_debug_function`, but they will //not// respect macros set in the user's code. (I sent an email to libcxx-dev about this issue, specifically that debug iterators are broken because of it, on 2021-05-01, but nobody's replying to my email.)
> > 
> > (C) If you want libc++ to support this abstraction long-term, you absolutely need to "put a test on it." libcxx/test/libcxx/debug/ would probably be the right place for a new test that verifies the behavior of defining `_LIBCPP_DEBUG_FUNCTION_INVOCATION`.
> (For B: We build libc++ from source and statically link it, so that's fine for us.)
Why do we (Chromium) want this: in Chromium, we want these debugging/hardening checks in production. But, we are very sensitive to object code size, and the standard implementation has those print statements (which can get big). This macro allows us to use the smallest possible crash code (e.g. a `ud2` on Intel, or the like), and thus have the least impact on our overall object code size.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89353/new/

https://reviews.llvm.org/D89353