[libcxx-commits] [PATCH] D121123: [libc++] Add a way to enable lightweight assertions independent of the debug mode

Mon Mar 7 11:54:06 PST 2022

ldionne marked 8 inline comments as done.
ldionne added a comment.

In D121123#3364671 <https://reviews.llvm.org/D121123#3364671>, @Quuxplusone wrote:

> I have no particular opinion on whether this is a good direction that will eventually lead somewhere useful, or just a random walk. No particular objection, either.

It does lead somewhere useful **today**. Indeed, as you can see, it adds an assertion handler in the dylib under all configurations, and `_LIBCPP_ENABLE_ASSERTIONS` is such that it can be defined by the user regardless of how the library was built. This, alone, fixes most of the issues with the debug mode.

Another way to look at this patch is that it makes the bits of the debug mode that matter the most (i.e. non-complexity-modifying assertions) available to everyone, outside of the debug mode. This sets up the stage for either removing the remaining debug mode assertions (which are broken as everyone knows), or fixing them but keeping them separate from basic assertions. This is all cleanup / future work, but the basic assertions this allows us to enable is a real benefit that we get today.

================
Comment at: libcxx/CMakeLists.txt:681-685
 define_if(LIBCXX_DEBUG_BUILD -D_DEBUG)
 if (LIBCXX_ENABLE_ASSERTIONS AND NOT LIBCXX_DEBUG_BUILD)
   # MSVC doesn't like _DEBUG on release builds. See PR 4379.
   define_if_not(LIBCXX_TARGETING_MSVC -D_DEBUG)
 endif()
----------------
Quuxplusone wrote:
> I see you're no longer fiddling with `NDEBUG`. Why then are you //keeping// the fiddling with `_DEBUG`? What goes wrong if you remove these 5 lines too?
Because that is related to `LIBCXX_DEBUG_BUILD`, and that appears to still be used (in principle) on Windows (see for example https://github.com/llvm/llvm-project/blob/main/libcxx/utils/libcxx/test/config.py#L273).

Basically, I'm trying to clean some stuff up, but I'm also trying to be careful not to change anything unrelated to this PR specifically. I do definitely plan to look into it when I get to it.

================
Comment at: libcxx/docs/BuildingLibcxx.rst:219

-  Build libc++ with assertions enabled.
+  Build libc++ with assertions enabled in the library, and enable assertions by
+  default when building user code as well. Assertions can be turned off by users
----------------
Quuxplusone wrote:
> Here and several times below, where you talk about "the library," I think it's important to find a phrase that clarifies we're talking about the .so or .dylib (or I guess the .a, which is not a "shared library") — we're not just talking about code that happens to be inside "libc++," like in the headers or in the `std` namespace.
> Given that this phrase has to work for both .so //and// .a, though, I have no great suggestion. :(  The best I've thought of so far is "the compiled library."
Agreed, will change to "the compiled library".

================
Comment at: libcxx/docs/UsingLibcxx.rst:132
+exhaustive -- instead they aim to provide a good balance between safety and performance.
+In particular, these assertions do not change the complexity of algorithms. However, they
+might, in some cases, interfere with compiler optimizations.
----------------
Quuxplusone wrote:
> I thought some of them //do// majorly change the complexity of algorithms! E.g. `__do_compare_assert` in the debug comparator will definitely change some algorithms on vectors-of-vectors-of-vectors from linearish (each step does 1^k comparisons) to exponentialish (each step does 2^k comparisons).
> I might be wrong about this (e.g. if your new assert mode doesn't turn on debug comparators). But if this is an intended invariant, this needs to be called out //vocally and explicitly//, like maybe in a bulleted list of "things libc++ guarantees about our assertions," so that it's very clear to users //and maintainers// what'll count as a bug here.
You are right, however D121129 fixes that and makes `__do_compare_assert` a debug-mode assertion instead of a regular assertion.

IMO the current description is sufficient: it says that we aim not to change the complexity of algorithm, and I think that should be the criterion that we start with.

================
Comment at: libcxx/docs/UsingLibcxx.rst:158-161
+Replacing the default assertion handler is done by defining the ``_LIBCPP_ASSERTION_HANDLER`` macro,
+which is called with an instance of ``std::__libcpp_assertion_info``. Generally speaking,
+``_LIBCPP_ASSERTION_HANDLER(assertion_info)`` should expand to a fully qualified function call.
+For example:
----------------
Quuxplusone wrote:
> ```
> // In HelloWorldConfig.h (this must always be included before any other libc++ header)
> ```
> I think this has come up before, like in the past six months: We shouldn't encourage users to `#define _LIBCPP_WHATEVER` at the top of their file, because that will tend to get moved down, or not work when their file is a .h, or break when they move to modules, or whatever. We should encourage them to pass `-D_LIBCPP_WHATEVER` in their build system's C(XX)FLAGS instead.
> 
> (Yikes, you're even suggesting that they //do// put it in a .h, with a note that this header //must// be included first in any .cpp file that needs it. That's asking for trouble.)
I agree, this is error prone and not very ergonomic. It's also not simple to change this:

- The assertion handler needs to have been declared before any libc++ code is included. Otherwise, the compiler will complain that we are using `::HelloWorldAssertionHandler` in the code but it hasn't been declared yet.

- The assertion handler needs to be aware of the type `std::__libcpp_assertion_info`, which means that the declaration of `HelloWorldAssertionHandler` must come **after** including `<__assert>`, and **before** including anything else. That's why we have this weird include sandwich.

I think I can get rid of the second requirement by passing primitive types to the assertion handler, i.e. passing `char const* file, int line, char const* predicate, char const* message)` directly to the function. However, that prevents me from being backwards compatible with users that might have been setting `__libcpp_debug_function` (because I use the conversion operator in `__libcpp_assertion_info` to achieve that).

I'm not sure how to solve the first problem, though. In practice, I would expect that people use `-include <path-to-header-that-declares-the-assertion-handler>`.

To make progress, would you be OK with me changing the documentation to:

```
// In HelloWorldAssert.h
#define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
#include <__assert>

// This could also be defined out-of-line, but we're defining it inline
// for simplicity in this example.
inline void HelloWorldAssertionHandler(std::__libcpp_assertion_info info) {
  std::printf("Assertion %s failed at %s:%d, more info: %s",
              info.__predicate(), info.file(), info.__line(), info.__message());
  std::abort();
}

// In HelloWorld.cpp
// Make sure to pass `-include path/to/HelloWorldAssert.h` to the compiler
#include <vector>

int main() {
  std::vector<int> v;
  int& x = v[0]; // Your assertion handler will be called here if _LIBCPP_ENABLE_ASSERTIONS=1
}
```

If we decide to break backwards compatibility with the debug mode, we can simplify this to (I'm happy to do it, but I would prefer doing it separately since it's a breaking change):

```
// In HelloWorldAssert.h
#define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
// This could also be defined out-of-line, but we're defining it inline
// for simplicity in this example.
inline void HelloWorldAssertionHandler(char const* file, int line, char const* expression, char const* message) {
  std::printf("Assertion %s failed at %s:%d, more info: %s", expression, file, line, message);
  std::abort();
}

// Same story from HelloWorld.cpp, still need `-include path/to/HelloWorldAssert.h`
```

================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > philnik wrote:
> > > ldionne wrote:
> > > > I am not very happy about requiring users to include an internal header to customize the assertion handler. IMO that's kind of hacky. I thought about using `<version>` instead, but that would require `<version>` to include `<__assert>`, and unfortunately that's a no-go because `<__assert>` needs to include `<iosfwd>` right now (due to the legacy debug mode support).
> > > Why not put it in `<cassert>`? That would make the most sense IMO.
> > I agree it makes some sense, however we still have the problem that we need to include `<iosfwd>` from this header.
> > 
> > In other words, I don't think we can easily change this part of the user-facing API unless we agree to drop `<iosfwd>` from `<__assert>` (and in that case we can even include it from e.g. `<__config>`).
> Tangent FWIW: it seems like we could use a `<__stringfwd>` helper header.
I agree, and it's useful for e.g. D116950 anyways.

================
Comment at: libcxx/src/debug.cpp:43-46
+bool __libcpp_set_debug_function(__libcpp_debug_function_type __func) {
+  __libcpp_debug_function = __func;
+  return true;
+}
----------------
Quuxplusone wrote:
> I think it's weird that `__libcpp_set_debug_function` is a dylib function, when all it actually does is write to the extern-linkage global variable `__libcpp_debug_function` which is already accessed from many places in the header code. Consider making this an inline function in the header, instead?
> Or is there a benefit to having it in one place in the dylib where I guess someone could set a breakpoint on it or something?
Well, we can't really remove it from the dylib because some folks may depend on that symbol being defined. So it has to stay in the dylib, at the very least.

And as I've hinted at, I believe we should remove those from our headers in the future: `__libcpp_set_debug_function`, `__libcpp_debug_function` and `__libcpp_abort_debug_function`. Indeed, those are effectively supplanted by this new suggested assertion handler.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123