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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Mar 7 10:24:28 PST 2022


Quuxplusone added a comment.

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.



================
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()
----------------
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?


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


================
Comment at: libcxx/docs/ReleaseNotes.rst:71-72
+  undefined behavior in user code. This new support is now separate from the old
+  (and incomplete) Debug Mode. It provides more flexibility, such as customizing
+  the assertion handler at compilation time, and vendors can select whether the
+  library they ship should include assertions or not by default. For details, see
----------------
More flexibility than what? Personally I'd cut this clause and just start the sentence at "Vendors can select..."


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


================
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:
----------------
```
// 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.)


================
Comment at: libcxx/docs/UsingLibcxx.rst:167
+  #define _LIBCPP_ASSERTION_HANDLER(info) ::HelloWorldAssertionHandler(info)
+  #include <__assert>
+
----------------
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.


================
Comment at: libcxx/src/assert.cpp:13
 #include <cstdlib>
 #include <string>
 
----------------
`<string>` is no longer needed.


================
Comment at: libcxx/src/debug.cpp:43-46
+bool __libcpp_set_debug_function(__libcpp_debug_function_type __func) {
+  __libcpp_debug_function = __func;
+  return true;
+}
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121123



More information about the libcxx-commits mailing list