Page MenuHomePhabricator

[libc++] Make the Debug mode a configuration-time only option
ClosedPublic

Authored by ldionne on Apr 1 2022, 2:33 PM.

Details

Summary

The debug mode has been broken pretty much ever since it was shipped
because it was possible to enable the debug mode in user code without
actually enabling it in the dylib, leading to ODR violations that
caused various kinds of failures.

This commit makes the debug mode a knob that is configured when
building the library and which can't be changed afterwards. This is
less flexible for users, however it will actually work as intended
and it will allow us, in the future, to add various kinds of checks
that do not assume the same ABI as the normal library. Furthermore,
this will make the debug mode more robust, which means that vendors
might be more tempted to support it properly, which hasn't been the
case with the current debug mode.

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.

In the future, we should increase the granularity of the debug mode
checks so that we can cherry-pick which checks to enable. Furthermore,
a subset of checks that are currently marked as requiring a matching
dylib may be relaxed to not require that (such as the consistent
comparator tests).

Diff Detail

Event Timeline

ldionne created this revision.Apr 1 2022, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 2:33 PM
ldionne requested review of this revision.Apr 1 2022, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2022, 2:33 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

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.

libcxx/docs/TestingLibcxx.rst
161–167

Is there no way to enable assertions in the tests?

libcxx/include/__debug
22–24

Maybe we want to add a #warning for users who define _LIBCPP_DEBUG at all?

ldionne marked 2 inline comments as done.Apr 3 2022, 5:50 AM

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.

libcxx/docs/TestingLibcxx.rst
161–167

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.

libcxx/include/__debug
22–24

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.

ldionne updated this revision to Diff 420041.Apr 3 2022, 5:51 AM
ldionne marked 2 inline comments as done.

Address some CI failures.

Thanks for working on making debug mode more useful!

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.

I really like this direction!

libcxx/docs/DesignDocs/DebugMode.rst
23–26

Does this mean we want to use the unstable ABI in this mode?

philnik accepted this revision as: philnik.Apr 3 2022, 6:53 AM

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.

libcxx/docs/DesignDocs/DebugMode.rst
23–26

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.

libcxx/docs/TestingLibcxx.rst
161–167

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.

libcxx/include/__debug
22–24

Sounds reasonable.

ldionne updated this revision to Diff 420052.Apr 3 2022, 7:32 AM

Fix more CI failures

ldionne marked 2 inline comments as done.Apr 3 2022, 7:43 AM
ldionne added a subscriber: danlark.

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.

We are very much on the same page, it seems :-). Quoting from my in-progress design document:

  • The sanitized version of the library would have a different name, for example libc++-sanitized.dylib. A different __config_site file would also be provided to be used when compiling code against the sanitized library. Vendors could ship both the sanitized and the non-sanitized library side-by-side.
  • The sanitized version of the library would use a different inline namespace to make sure that code can’t accidentally link against the wrong version of the library (which would result in runtime errors due to ABI incompatibilities).
  • When -fsanitize=library is passed to the compiler, the compiler would automatically switch to using the sanitized version of the library. It would use the sanitized shared library as well as the sanitized version of the __config_site header, so that code in the headers would be built with sanitization enabled too.

I will also treat the topic of ABI compatibility between sanitized and non-sanitized versions, and also ABI stability between sanitized versions. Stay tuned, I should post it to Discourse in a couple of days.

@danlark
This patch will break users of the randomized unspecified behavior you added since you are most likely not shipping a debug-mode version of the library today. Has it been in used internally at Google?

libcxx/docs/DesignDocs/DebugMode.rst
23–26

I think we should use the unstable ABI in debug mode. ABI compatibility between GCC and Clang should not be an issue, because we would expect the debug mode to only be used through Clang's -fsanitize=library flag.

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.

We are very much on the same page, it seems :-). Quoting from my in-progress design document:

  • The sanitized version of the library would have a different name, for example libc++-sanitized.dylib. A different __config_site file would also be provided to be used when compiling code against the sanitized library. Vendors could ship both the sanitized and the non-sanitized library side-by-side.
  • The sanitized version of the library would use a different inline namespace to make sure that code can’t accidentally link against the wrong version of the library (which would result in runtime errors due to ABI incompatibilities).
  • When -fsanitize=library is passed to the compiler, the compiler would automatically switch to using the sanitized version of the library. It would use the sanitized shared library as well as the sanitized version of the __config_site header, so that code in the headers would be built with sanitization enabled too.

I will also treat the topic of ABI compatibility between sanitized and non-sanitized versions, and also ABI stability between sanitized versions. Stay tuned, I should post it to Discourse in a couple of days.

@danlark
This patch will break users of the randomized unspecified behavior you added since you are most likely not shipping a debug-mode version of the library today. Has it been in used internally at Google?

Thanks for adding me.

I confirm we haven't shipped anything with randomization to the (external) users and this patch should be fine with us as we build everything from source

ldionne marked an inline comment as done.Apr 3 2022, 8:20 AM

@danlark
This patch will break users of the randomized unspecified behavior you added since you are most likely not shipping a debug-mode version of the library today. Has it been in used internally at Google?

Thanks for adding me.

I confirm we haven't shipped anything with randomization to the (external) users and this patch should be fine with us as we build everything from source

Just to make sure we're on the same page, here's what this change entails if you've been using randomization (even just internally): Out-of-the-box, you'll get a compiler error saying that you're using unspecified behavior randomization with a library that has been built without the debug mode. You'll need to switch to building the library with the debug mode enabled, i.e. LIBCXX_ENABLE_DEBUG_MODE=ON when configuring in CMake. However, that will also cause all the other debug mode checks to be enabled, which you may or may not want. And there will certainly be a slowdown associated to this, too. Is that fine, or do we need to make it possible to use unspecified behavior randomization without using the rest of the debug mode from the get-go? If that's a requirement for you, I'll try to think about ways to make that possible in this version of the patch and keep you up-to-date. If not, that makes my life easier since I can land this and then relax requirements once we've agreed on a design document (aiming to make it happen this week).

@danlark
This patch will break users of the randomized unspecified behavior you added since you are most likely not shipping a debug-mode version of the library today. Has it been in used internally at Google?

Thanks for adding me.

I confirm we haven't shipped anything with randomization to the (external) users and this patch should be fine with us as we build everything from source

Just to make sure we're on the same page, here's what this change entails if you've been using randomization (even just internally): Out-of-the-box, you'll get a compiler error saying that you're using unspecified behavior randomization with a library that has been built without the debug mode. You'll need to switch to building the library with the debug mode enabled, i.e. LIBCXX_ENABLE_DEBUG_MODE=ON when configuring in CMake. However, that will also cause all the other debug mode checks to be enabled, which you may or may not want. And there will certainly be a slowdown associated to this, too. Is that fine, or do we need to make it possible to use unspecified behavior randomization without using the rest of the debug mode from the get-go? If that's a requirement for you, I'll try to think about ways to make that possible in this version of the patch and keep you up-to-date. If not, that makes my life easier since I can land this and then relax requirements once we've agreed on a design document (aiming to make it happen this week).

Hmm, from the exact snapshot of the patch I see that _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY can be used as a build flag without LIBCXX_ENABLE_DEBUG_MODE=on, is this going to be changed?

Ideally, yes, we would like to build without all checks being enabled at the same time as they increase the build size by substantial amount (especially things like vector subscription operator), we enabled randomization separately without all checks and that was our intention

Hmm, from the exact snapshot of the patch I see that _LIBCPP_DEBUG_RANDOMIZE_UNSPECIFIED_STABILITY can be used as a build flag without LIBCXX_ENABLE_DEBUG_MODE=on, is this going to be changed?

Actually, you're right. That wasn't the intent of the patch originally, but it works and I agree it should have been the intent of the patch. My next update will reflect that intent, and will ensure that it's tested.

ldionne updated this revision to Diff 434116.Fri, Jun 3, 12:52 PM

Rebase on top of D126993

philnik added inline comments.Fri, Jun 3, 1:20 PM
libcxx/CMakeLists.txt
76–80

Could we disable this by default in the unstable ABI?

libcxx/docs/DesignDocs/DebugMode.rst
25
libcxx/include/__debug
38
ldionne updated this revision to Diff 434502.Mon, Jun 6, 8:53 AM
ldionne marked 3 inline comments as done.

Rebase onto main and address comments.

ldionne updated this revision to Diff 434518.Mon, Jun 6, 10:09 AM

Fix merge conflict that broke the CI

ayzhao added a subscriber: ayzhao.Mon, Jun 6, 1:42 PM
ldionne updated this revision to Diff 434795.Tue, Jun 7, 6:09 AM

Rebase to trigger CI. I think the previous failure was a fluke.

ldionne updated this revision to Diff 434863.Tue, Jun 7, 10:13 AM

Fix CI with debug mode enabled.

ldionne accepted this revision as: Restricted Project.Tue, Jun 7, 1:31 PM

I'm going to land this. If you get to this review because your build started failing (you were defining _LIBCPP_DEBUG and you now get a compiler error), please note that this is intended. The debug mode was previously unsafe and broken, and you need to start using a library that was built with the debug mode enabled specifically.

This revision is now accepted and ready to land.Tue, Jun 7, 1:31 PM