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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision as: philnik.
philnik added a comment.

Then this LGTM.

Another thought: Should we change the ABI namespace to force link time errors if someone tries to link against the wrong library? - This should probably be done in the same patch where we enable some ABI flags.



================
Comment at: libcxx/docs/DesignDocs/DebugMode.rst:23
 
-Also note that while the debug mode has no effect on libc++'s ABI, it does have broad ODR
-implications. Users should compile their whole program at the same debugging level.
+Furthermore, users should not rely on a stable ABI being provided when the debug mode is
+enabled -- we reserve the right to change the ABI at any time. If you need a stable ABI
----------------
Mordante wrote:
> Does this mean we want to use the unstable ABI in this mode?
I don't think so. I think we should still guarantee ABI stability between GCC and clang, which excludes the `[[clang::trivial_abi]]` optimizations. I also wouldn't expect the size of `tuple` to change between debug and non-debug library, but that one is more debatable I think.


================
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.
-
----------------
ldionne wrote:
> 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.
Yes, that sounds reasonable. When we do that we should probably check if there is any extra information here and put it in a comment if there is any.


================
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
----------------
ldionne wrote:
> 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.
Sounds reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122941