Enable overriding `__libcpp_debug_function` invocation

[ldionne]

Summary

Enable not just changing the handler function at run-time, but also
enable changing how it's invoked at compile time.

Document that, and update some other Debug Mode documentation as well.

[palmer]

This is following up on the discussion at https://reviews.llvm.org/D70343. Does this overall direction make sense to you? I have hacked it into Chromium and it seems to work.

[jdoerrie]

LGTM, no concerns from my side.

[palmer]

Thanks, jdoerrie! Does anyone else have thoughts?

[palmer]

Rebased against the new origin/main.

[EricWF]

Assuming this still merges, I don't have any issue with it.

There has been some work done by others on the debug mode. We don't need to wait for their approval, but please be receptive if they have comments.

[• Quuxplusone]

libcxx/docs/DesignDocs/DebugMode.rst

83

In what sense does std::locale support iterators?

libcxx/include/__debug

42

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

[• Quuxplusone]

libcxx/docs/DesignDocs/DebugMode.rst

83

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.

[thakis]

libcxx/include/__debug

42

(For B: We build libc++ from source and statically link it, so that's fine for us.)

[palmer]

libcxx/docs/DesignDocs/DebugMode.rst

83

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.

libcxx/include/__debug

42

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.

[• Quuxplusone]

libcxx/docs/DesignDocs/DebugMode.rst

83

Ah, that makes sense. But then I repeat: "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.)"
As of 2021-05-24, nobody is still replying to that email. I don't think we should pursue this PR D89353 until someone does.

[palmer]

libcxx/docs/DesignDocs/DebugMode.rst

83

In Chromium, we build libcxx entirely from source, so I don't think we have that problem.

[akhuang]

apparently i'll take over this patch from here!

[akhuang]

Remove parens from macro and add test case

[akhuang]

thought I'd updated this patch but apparently I hadn't--

[ldionne]

For some reason this went entirely under my radar.

I think we should have a broader discussion about what to do with the debug mode (top of thread: https://lists.llvm.org/pipermail/libcxx-dev/2021-June/001168.html) before we make this change.

If I understand correctly, the main problem being solved by this patch is that Chromium does not want to have those __FILE__ string constants stored in the compiled binaries (because it doesn't use that information anyway)?

[palmer]

If I understand correctly, the main problem being solved by this patch is that Chromium does not want to have those __FILE__ string constants stored in the compiled binaries (because it doesn't use that information anyway)?

It's not just the string constants, it's that we want everything about the code to be as small as possible:

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

[rnk]

In D89353#2811149, @ldionne wrote:

If I understand correctly, the main problem being solved by this patch is that Chromium does not want to have those __FILE__ string constants stored in the compiled binaries (because it doesn't use that information anyway)?

As I understand it, the plan is to actually ship with these checks enabled. This expands the scope of these debug mode checks from a testing tool to a security hardening tool. That's why the binary size matters: it will ship to users. It starts from the simple idea that std::vector<>::operator[] could have bounds checks, and one way to get there is to enable debug checks and add this override mechanism.

[jfb]

FWIW I support something along the lines of what Palmer is trying to do: many users of the STL would like to be able to handle particular types of library UB instead of performing potentially unbounded effects. Which ones is a bit of an art that we need to figure out, for example I wouldn't want to unduly affect performance (by e.g. changing algorithmic complexity) or code size (here, with diagnostic bloat). In a way we're experimenting on the edges of what contract checking would be for the STL, and I would approach it with this optic: could we have a mode of the STL which enforces contracts, and a mode where you get whatever. If so, what should be checked and what should remain unchecked, and what are the principles to distinguish these. I think in this discussion it's important to separate the invariants that are details of libc++'s implementation, from what are actual contracts at the STL API level.

[ldionne]

[Github PR Transition cleanup]

The Debug mode doesn't exist anymore, but this was done in D141326. I will close this to clean up the queue (which requires commandeering and abandoning, sorry if that seems rude but it's how Phabricator works).

[rnk]

+1, this use case has long been addressed by the assertion mode, and then the new safe and hardened modes.

libcxx/docs/DesignDocs/DebugMode.rst

.. code-block:: cpp
   int main(int, char**) {
     std::__libcpp_debug_function = &my_handler;
 
     std::string::iterator bad_it;
     std::string str("hello world");
     str.insert(bad_it, '!'); // causes debug assertion
     // control flow doesn't return
   }
+
+The above example overrides the bug handler at run-time, but it is also possible
+to override it at compile time using the ``_LIBCPP_DEBUG_FUNCTION_INVOCATION``
+macro. This macro takes 2 arguments: the expression ``x`` to trigger the
+assertion, and an error message ``m``. The default invocation uses these
+arguments to construct the ``std::__libcpp_debug_info`` argument, but you can do
+whatever you like with them (including nothing):
+
+.. code-block:: cpp
+
+  #define _LIBCPP_DEBUG_FUNCTION_INVOCATION(x, m) abort()
+  #include <string>
+  int main(int, char**) {
+    std::string::iterator bad_it;
+    std::string str("hello world");
+    str.insert(bad_it, '!'); // causes debug assertion
+    // control flow doesn't return
+  }
+
+Debug Mode Checks
+=================
+
+Libc++'s debug mode offers two levels of checking. The first enables various
+precondition checks throughout libc++. The second additionally enables
+"iterator debugging" which checks the validity of iterators used by the program.
+
+Basic Checks
+============
+
+These checks are enabled when ``_LIBCPP_DEBUG`` is defined to either 0 or 1.
+
+The following checks are enabled by ``_LIBCPP_DEBUG``:
+
+  * Many algorithms, such as ``binary_search``, ``merge``, ``next_permutation``, and ``sort``,
+    wrap the user-provided comparator to assert that `!comp(y, x)` whenever
+    `comp(x, y)`. This can cause the user-provided comparator to be evaluated
+    up to twice as many times as it would be without ``_LIBCPP_DEBUG``, and
+    causes the library to violate some of the Standard's complexity clauses.
+
+  * Bounds checks in containers (e.g ``operator[]``)
+
+  * Validity of dereferencing (e.g. ``operator*``, ``operator->``)
+
+  * Invariants and preconditions (e.g. ``has_value`` for ``std::optional``)
+
+Iterator Debugging Checks
+=========================
+
+These checks are enabled when ``_LIBCPP_DEBUG`` is defined to 1.
+
+The following containers and STL classes support iterator debugging:
+
+  * ``std::string``
+  * ``std::vector<T>`` (``T != bool``)
+  * ``std::list``
+  * ``std::locale``
+  * ``std::unordered_map``
+  * ``std::unordered_multimap``
+  * ``std::unordered_set``
+  * ``std::unordered_multiset``
+
+The remaining includes do not currently support iterator debugging.
+Patches welcome.

libcxx/include/__debug

 #endif
 
 #if _LIBCPP_DEBUG_LEVEL >= 1 || defined(_LIBCPP_BUILDING_LIBRARY)
 #   include <cstdlib>
 #   include <cstdio>
 #   include <cstddef>
 #endif
 
+#if !defined(_LIBCPP_DEBUG_FUNCTION_INVOCATION)
+#   define _LIBCPP_DEBUG_FUNCTION_INVOCATION(x, m) _VSTD::__libcpp_debug_function(_VSTD::__libcpp_debug_info(__FILE__, __LINE__, x, m))
+#endif
+
 #if _LIBCPP_DEBUG_LEVEL == 0
 #   define _LIBCPP_DEBUG_ASSERT(x, m) ((void)0)
 #   define _LIBCPP_ASSERT_IMPL(x, m) ((void)0)
 #elif _LIBCPP_DEBUG_LEVEL == 1
 #   define _LIBCPP_DEBUG_ASSERT(x, m) ((void)0)
-#   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))
 #elif _LIBCPP_DEBUG_LEVEL == 2
 #   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
 #   error _LIBCPP_DEBUG_LEVEL must be one of 0, 1, 2
 #endif
 
 #if !defined(_LIBCPP_ASSERT)
 #   define _LIBCPP_ASSERT(x, m) _LIBCPP_ASSERT_IMPL(x, m)
 #endif
 

libcxx/test/libcxx/debug/debug_function_invocation_macro.pass.cpp

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEBUG=1
+// UNSUPPORTED: libcxx-no-debug-mode
+
+// Test that the _LIBCPP_DEBUG_FUNCTION_INVOCATION macro works.
+
+// Define macro so that it doesn't abort.
+#define _LIBCPP_DEBUG_FUNCTION_INVOCATION(x, m) std::_Exit(EXIT_SUCCESS)
+
+#include <string>
+
+int main(int, char**) {
+  std::string::iterator bad_it;
+  std::string str("hello world");
+  str.insert(bad_it, '!');
+  return EXIT_FAILURE;
+}