[libcxx-commits] [PATCH] D122941: [libc++] Make the Debug mode a configuration-time only option

[Louis Dionne via Phabricator via libcxx-commits]

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

In D122941#3424161 <https://reviews.llvm.org/D122941#3424161>, @philnik wrote:

> Just as a thought: Do we want to give the debug-enabled library a different name? That way users could use `-stdlib=libc++` for the normal library and `-stdlib=libc++-debug` (or something like that) to build against the debug library. I guess vendors should either ship only a non-debug library or both, since having debug enabled has huge performance implications.

Yes, I agree, this is necessary if we want to ship both libraries side by side. In fact, I am planning to write a proposal for a new Clang/libc++ feature where one could basically use `-fsanitize=library` and Clang would automatically switch to the debug version of the library. I think it would make more sense to use a different library name once we lay out that framework -- this patch is still just groundwork to disentangle things.

> Another thought: Since we don't guarantee ABI-stability for the debug library AFAICT, do we want to enable a few ABI flags? Specifically things like `_LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON` or `_LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION`? These ABI changes don't affect the API, so they shouldn't be observable.

Yes, I think that would make sense. As it stands, this patch is currently backwards compatible despite updating the documentation to say that one shouldn't rely on ABI stability with the debug mode. I'd like to keep it that way and make any actual potentially breaking changes in separate patches, since those will be easier to revert and call out. I'm adding to my TODO list to enable these ABI breaks in the debug mode as a separate effort.



================
Comment at: libcxx/docs/TestingLibcxx.rst:161-167
-.. option:: debug_level=<level>
-
-  **Values**: 0, 1
-
-  Enable the use of debug mode. Level 0 enables assertions and level 1 enables
-  assertions and debugging of iterator misuse.
-
----------------
philnik wrote:
> Is there no way to enable assertions in the tests?
Yes, there's the `enable_assertions` Lit parameter. I am not a huge fan of documenting these options here because it leads to duplication and out-of-date documentation, instead I think we should point users to `libcxx/utils/libcxx/test/params.py` since it's pretty easy to figure out which options are available from the sources. Users wanting to test libc++ are actually vendors or libc++ developers, so I think it's reasonable to point them to the sources.

If you agree, I'll do that in a separate review.


================
Comment at: libcxx/include/__debug:22-24
+#if defined(_LIBCPP_DEBUG) && _LIBCPP_DEBUG != 0 && !defined(_LIBCPP_ENABLE_DEBUG_MODE)
+#   error "Enabling the debug mode now requires having configured the library with support for the debug mode"
 #endif
----------------
philnik wrote:
> Maybe we want to add a `#warning` for users who define `_LIBCPP_DEBUG` at all?
I went ahead and did that, however then I read my commit message and in particular this part:

```
This patch shouldn't break any user code, except folks who are building
against a library that doesn't have the debug mode enabled and who try
to enable the debug mode in their code. Such users will get a compile-time
error explaining that this configuration isn't supported anymore.
```

Adding a warning is going to potentially break a lot more users if they compile with `-Werror`. I think we want to do that, but I'd like to land it separately to keep the impact of this patch as small as possible, given it's a large patch and I really don't want to revert it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122941