This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Unconditionally provide the ability for users to enable assertions
AbandonedPublic

Authored by ldionne on Feb 16 2022, 12:47 PM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Summary

Previously, assertions could only be enabled if the library was configured
to include support for the Debug mode. This patch unconditionally adds the
very small amount of code needed to support assertions (and assertions
only, not the whole iterator debugging functionality) to the shared library.

This means that users can turn assertions on in their code regardless of
how the library was configured, which makes it a true user-facing knob.
I think this makes sense, just like we allow users to decide whether
assert() is enabled or not.

If they don't enable assertions in their code, nothing changes compared
to before this patch, except the shared library will now be a tiny bit
larger than it used to be. In my opinion, this is insignificant compared
to the benefit of being able to turn assertions on, and it's not even
worth the complexity to provide a knob to turn that part off.

Also note that technically, functions compiled inside the shared library
may or may not have assertions enabled, depending on whether the vendor
configured the library with LIBCXX_ENABLE_ASSERTIONS when building.
This can lead to ODR violations, however I think those are all benign.

Finally, since we're adding symbols to the shared library, this means
that this feature won't be available when linking against older dylibs,
hence the accompanying availability markup.

Diff Detail

Event Timeline

ldionne created this revision.Feb 16 2022, 12:47 PM
ldionne requested review of this revision.Feb 16 2022, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2022, 12:47 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Note that we could have interesting design discussions:

  1. Is it really necessary to allow setting the "assertion handler" at runtime, i.e. couldn't we get rid of __libcpp_set_debug_function and just always call abort()?
  2. Why don't we change the name of __libcpp_debug_function & friends to be more like __libcpp_assertion_handler?

Since the debug mode has been shipping in the LLVM release, my assumption is that people might have been enabling assertions and expecting ABI stability, which would be fair. If we make any of the changes above, we would break that.

As you might guess, my main goal is to eventually be able to enable assertions on Apple platforms, which isn't supported right now (we build our dylib with LIBCXX_ENABLE_DEBUG_MODE_SUPPORT=OFF). Technically, these ABI breaks would not impact us because we haven't been shipping those symbols, however they would impact users on other platforms. That's why I've decided to avoid going down that route so far.

ldionne updated this revision to Diff 409642.Feb 17 2022, 7:10 AM

Add ABI lists to try to fix the CI

ldionne added a subscriber: Restricted Project.Feb 28 2022, 2:21 PM

Does anyone want to chime in? Maybe libc++ vendors have an opinion about this? Vendors, have you been shipping the debug mode? With what stability guarantees? Unless told otherwise, my assumption is that folks have been shipping the library as-is, which means with support for the debug mode enabled (which unfortunately ties our hands a bit in this change).

We had never defined LIBCXX_ENABLE_ASSERTIONS and _LIBCPP_DEBUG explicitly in our cmake config for Chrome OS.

Running readelf shows following in our shipping shared libs.

 231: 00000000000c7db0     8 OBJECT  GLOBAL DEFAULT   23 _ZNSt3__123__libcpp_debug_functionE
 668: 0000000000075160    13 FUNC    GLOBAL DEFAULT   14 _ZNSt3__127__libcpp_set_debug_functionEPFvRKNS_19__libcpp_debug_infoEE
1438: 0000000000075110    79 FUNC    GLOBAL DEFAULT   14 _ZNSt3__129__libcpp_abort_debug_functionERKNS_19__libcpp_debug_infoE
1979: 0000000000074ef0   544 FUNC    GLOBAL DEFAULT   14 _ZNKSt3__119__libcpp_debug_info4whatEv

Running readelf shows following in our shipping shared libs.

 231: 00000000000c7db0     8 OBJECT  GLOBAL DEFAULT   23 _ZNSt3__123__libcpp_debug_functionE
 668: 0000000000075160    13 FUNC    GLOBAL DEFAULT   14 _ZNSt3__127__libcpp_set_debug_functionEPFvRKNS_19__libcpp_debug_infoEE
1438: 0000000000075110    79 FUNC    GLOBAL DEFAULT   14 _ZNSt3__129__libcpp_abort_debug_functionERKNS_19__libcpp_debug_infoE
1979: 0000000000074ef0   544 FUNC    GLOBAL DEFAULT   14 _ZNKSt3__119__libcpp_debug_info4whatEv

Okay, right -- so as expected, you've been shipping support for the debug mode as part of the dylib, and if any user has been enabling assertions with _LIBCPP_DEBUG=0, they are relying on those symbols. So I think my proposed direction stands (it's this patch as-is). It's not the most elegant design since we're reusing old __libcpp_debug_foo for assertions, but I think it's the best decision engineering-wise for the time being.

dim added subscribers: emaste, dim.Mar 1 2022, 7:35 AM

Running readelf shows following in our shipping shared libs.

 231: 00000000000c7db0     8 OBJECT  GLOBAL DEFAULT   23 _ZNSt3__123__libcpp_debug_functionE
 668: 0000000000075160    13 FUNC    GLOBAL DEFAULT   14 _ZNSt3__127__libcpp_set_debug_functionEPFvRKNS_19__libcpp_debug_infoEE
1438: 0000000000075110    79 FUNC    GLOBAL DEFAULT   14 _ZNSt3__129__libcpp_abort_debug_functionERKNS_19__libcpp_debug_infoE
1979: 0000000000074ef0   544 FUNC    GLOBAL DEFAULT   14 _ZNKSt3__119__libcpp_debug_info4whatEv

Okay, right -- so as expected, you've been shipping support for the debug mode as part of the dylib, and if any user has been enabling assertions with _LIBCPP_DEBUG=0, they are relying on those symbols. So I think my proposed direction stands (it's this patch as-is). It's not the most elegant design since we're reusing old __libcpp_debug_foo for assertions, but I think it's the best decision engineering-wise for the time being.

We've got something similar on FreeBSD:

% readelf -aW /lib/libc++.so.1
...
Relocation section '.rela.dyn' at offset 0x354f8 contains 2161 entries:
...
00000000000fc6e0  0000012c00000001 R_X86_64_64            0000000000092f60 std::__1::__libcpp_abort_debug_function(std::__1::__libcpp_debug_info const&) + 0
00000000000fb130  0000019b00000006 R_X86_64_GLOB_DAT      00000000000fc6e0 std::__1::__libcpp_debug_function + 0
...
Relocation section '.rela.plt' at offset 0x41f90 contains 572 entries:
...
00000000000fca38  0000085700000007 R_X86_64_JUMP_SLOT     0000000000092d10 std::__1::__libcpp_debug_info::what() const + 0
...
Symbol table '.dynsym' contains 2398 entries:
...
   300: 0000000000092f60    82 FUNC    GLOBAL DEFAULT   14 std::__1::__libcpp_abort_debug_function(std::__1::__libcpp_debug_info const&)
   411: 00000000000fc6e0     8 OBJECT  GLOBAL DEFAULT   25 std::__1::__libcpp_debug_function
  1468: 0000000000092fc0    18 FUNC    GLOBAL DEFAULT   14 std::__1::__libcpp_set_debug_function(void (*)(std::__1::__libcpp_debug_info const&))
  2135: 0000000000092d10   586 FUNC    GLOBAL DEFAULT   14 std::__1::__libcpp_debug_info::what() const

These all originate in debug.cpp. I think we have been shipping it like this since we switched from libstd++ to libc++ years ago.

So this change is fine from our perspective.

EricWF accepted this revision.Mar 1 2022, 9:25 AM
EricWF added a subscriber: EricWF.

Looks good to me.

This revision is now accepted and ready to land.Mar 1 2022, 9:25 AM
ldionne abandoned this revision.Mar 7 2022, 1:40 PM

I'm going to abandon this since I think we can do much better by providing an assertion handler that can be customized at compile-time instead of piggy-backing on the existing debug mode infrastructure, see D121123.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 1:40 PM