When a handle to an error_category singleton object is used during the termination phase of a program, the destruction of the error_category object may have occurred prior to execution of the current destructor or function registered with atexit, because the singleton object may have been constructed after the corresponding initialization or call to atexit. For example, the updated tests from this patch will fail if using a libc++ built using a compiler that updates the vtable of the object on destruction. This patch attempts to avoid the issue by causing the destructor to not be called in the style of ResourceInitHelper in src/experimental/memory_resource.cpp. This approach might not work if object lifetime is strictly enforced.
Details
- Reviewers
hubert.reinterpretcast jasonliu mclow.lists EricWF andusy cebowleratibm ldionne - Group Reviewers
Restricted Project - Commits
- rGfbdf684fae52: [libc++] Avoid destructor call for error_category singletons
Diff Detail
Event Timeline
I think a better fix would be to apply [[clang::no_destroy]] to the category variable definitions instead.
Or alternatively, there's an example of how to fix this in memory_resource.cpp https://github.com/llvm-mirror/libcxx/blob/master/src/experimental/memory_resource.cpp#L67-L96
@EricWF pointed out to me that it is UB to access standard library objects of static storage duration from an atexit handler: http://eel.is/c++draft/basic.start.term#6
I don't read the paragraph that way. The access during evaluation of an atexit handler must, in the absence of multithreading, happen before "completion of destruction of objects with static storage duration and execution of std::atexit registered functions".
When a handle to an error_category singleton object is used during the termination phase of a program, the destruction of the error_category object may have occurred prior to execution of the current destructor or function registered with atexit, because the singleton object may have been constructed after the corresponding initialization or call to atexit.
This sounds right, and that's exactly what http://eel.is/c++draft/basic.start.term#5 says (second sentence):
If a call to std::atexit strongly happens before the completion of the initialization of an object with static storage duration, the call to the destructor for the object is sequenced before the call to the function passed to std::atexit.
The good news is that the constructor of the singleton object in libc++ is under your control. It can be constructed whenever you choose, just by calling generic_category().
The lifetime of the object returned by generic_category() seems underspecified to me. Is there an objection to treating the behaviour as undesirable? It seems unfriendly to require users to collect possible error_category instances during initialization in order to keep error_codes around during program termination.
Hubert - I'm not following what you're saying.
Where did error_code come from? As far as I can tell, your comment is the first mention of error_code in this conversation.
Yes, you're correct it is. We encountered this lifetime issue in an application where the reference to the out-of-lifetime object was the cat_ exposition-only member of error_code. The status quo with libc++ is that error_codes are likely unsafe to use during program termination.
To chime in from the sidelines: We strive to have zero code that runs before main in Chromium, so we're not excited about a patch that adds more such code to libc++. (In fact, we're hoping to contribute a build mode that removes the existing static initializer for cin/cout/cerr as well.)
I'll pass along a comment from Howard (made to me privately, but he agreed that I could share it here):
There's a FILO guarantee on lifetime here: http://eel.is/c++draft/basic.start.term#3
A careful client would take advantage of that guarantee by ensuring that the lifetime of the object returned by std::generic_category() starts before the lifetime of a. One easy way to do this is in the default constructor for A:
A() {std::generic_category();}Failure to go to such effort is where the UB lies. The suggested "fix" which attempts to remove this responsibility from this client imposes a cost on every other client of libc++, whether or not one uses <system_error>. The more namespace-scope static objects there are in libc++, the longer it takes every program that links to libc++ to launch and reach main(). Apple considered this such a large problem that they forbade namespace-scope static objects and I had to get special permission for the std streams: cin, cout, etc.
Not only would acceptance of this patch penalize all users, it would still not absolutely guarantee a fix. Platforms are free to initialize namespace-scope objects in main prior to initializing namespace-scope objects within libc++.
Thanks for the clarification.
So the situation is:
- You register an atexit handler.
- During program termination, your handler is called, and you do something that creates an error_code object.
- You try to use the error_category of that error code, and discover that it has been destroyed?
Is that correct?
We did not hit the atexit handler case. We had a static lifetime object (the one returned by llvm::errs()) that holds an error_code (llvm::raw_fd_ostream::EC) as a member. The error_code is assigned to during the course of the program. When the destructor for the llvm::raw_fd_ostream is called, we discover that we can't use the error_category of the error code (because it has been destroyed).
libcxx/src/future.cpp | ||
---|---|---|
83 | Please don't write code like this. It's hard to read, and it's easy to have the attribute get disconnected from the thing it's referring to. Instead, define a new macro _LIBCPP_SAFE_STATIC_AFTER_CXX11 and use that. You have five #ifdefs in this patch. That's why we have macros like _LIBCPP_CONSTEXPR_AFTER_CXX11 |
libcxx/src/system_error.cpp | ||
---|---|---|
223 | Why do you overlay both of these in a single structure? Is your intent that a program only calls either generic_category or system_category in any particular run of the program? Otherwise, don't you get UB? |
libcxx/src/system_error.cpp | ||
---|---|---|
223 | These aren't overlaid, but it does mean that there would be an unused instance of one of the types for each helper we create. We should probably avoid that. |
libcxx/src/system_error.cpp | ||
---|---|---|
223 | Sorry, I misread the union/struct nesting. You are correct. |
- Added definition for _LIBCPP_SAFE_STATIC_AFTER_CXX11 in <__config>.
- Split up GenericAndSystemErrorHelper to GenericErrorHelper and SystemErrorHelper.
libcxx/src/future.cpp | ||
---|---|---|
82 | @mclow.lists, not sure if you meant to have this on one line; otherwise, I think all of the comments have been addressed. |
Sorry for being late to the party, I've looked at the codege of this and other approachs. what we want is a single global than initializes all of objects we need. Right now we have function local static then may may not have make guards and that's less than ideal.
I'm having to help the original author fix this patch, I also have a verified intestine patch of my own.
In other words, we only want to pay the cost of the destructor registration once for the entire library? I think this might need a new file and is a much larger scope than what this patch is attempting. @EricWF, is this patch being subsumed into a bigger effort?
That doesn't need to happen now. But I want to see these changed to be global statics not function local. At least that way we omit the guards entirely. and accessing the variable becomes a single instruction.
This goes back to the "paying for something you don't use" with regards to the up-front library loading cost. If people are actually agreed on this course of action, then we can create that version of the patch, but it does not seem clear to me.
This patch has sat idle for 2+ years but I think the current patch is an improvement. Currently, the error_category singletons are implemented as static locals for which there is no guarantee that the destructors won't run prematurely. The proposed patch fixes that problem.
Perhaps it is desirable to coalesce these singletons to static globals in a single module but there appears to be contention on the thread as to whether or not that is desirable. The static local solution has the advantage that it does not cause initialization if it is unused, but the accessor functions will be slower. My suggestion is to commit this patch and if anyone feels it's important to move to file scope globals then that can be addressed in a future patch.
libcxx/src/system_error.cpp | ||
---|---|---|
232–238 | After rebasing, I would expect this would end up looking like const error_category& system_category() _NOEXCEPT { union U { __system_error_category s; _LIBCPP_CONSTEXPR explicit U() : s() {} ~U() {} // Too bad this can't be trivial... or can it? }; static _LIBCPP_CONSTINIT U u; return u.s; } i.e. a very small diff against the left-hand side. |
libcxx/src/system_error.cpp | ||
---|---|---|
232–238 | I think the constexpr version will hit this error: error: constexpr variable cannot have non-literal type 'const U' |
libcxx/src/system_error.cpp | ||
---|---|---|
232–238 | There's no constexpr variable here, but you're right, the constexpr constructor hits src/system_error.cpp:212:37: error: constexpr constructor never produces a constant expression [-Winvalid-constexpr] _LIBCPP_CONSTEXPR explicit U() : s() {} ^ That's also a problem with the current PR. (Maybe that Clang diagnostic didn't exist when the PR was originally made; but it does exist now; I just tried it locally.) |
libcxx/src/system_error.cpp | ||
---|---|---|
232–238 | We probably want to apply [[no_destroy]] in conjunction with the above. That is, guarantee the functionality with the above and suppress the registration of the destructor for compilers that support it. |
I was writing a revision of this patch but hit a problem:
class _LIBCPP_TYPE_VIS error_category { public: virtual ~error_category() _NOEXCEPT; #if defined(_LIBCPP_BUILDING_LIBRARY) && \ defined(_LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS) error_category() _NOEXCEPT; #else _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11 error_category() _NOEXCEPT = default; #endif ...
When _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS is defined the non-constexpr error_category constructor causes a problem with the _LIBCPP_CONSTINIT singleton objects:
llvm-project/libcxx/src/ios.cpp:59:25: error: constexpr constructor never produces a constant expression [-Winvalid-constexpr] constexpr explicit IostreamErrorHelper()
It seems we would need a new CONSTINIT macro that doesn't get defined when _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS is defined and when libc++ is stuck on these legacy definitions then the singleton ctor calls can't be bypassed. It's a shame given that we know the constructor doesn't do anything meaningful.
I updated the patch to use the previous union pattern to bypass the singleton destructors but added constinit, constexpr and attribute((no_destroy)) as possible to avoid the runtime overhead of using a static local.
Note I had to introduce some new macros in __config to guard application of constinit and constexpr because one of the base classes error_category does not provide a constexpr constructor according to:
#if defined(_LIBCPP_BUILDING_LIBRARY) && \ defined(_LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS) error_category() _NOEXCEPT; ...
libcxx/include/__config | ||
---|---|---|
760–764 | I think we can get rid of those definitions once D123519 lands. | |
libcxx/test/std/input.output/iostreams.base/std.ios.manip/error.reporting/iostream_category.pass.cpp | ||
25 | Can you make this: struct A { same-as-before }; static A a; I initially missed the fact that you were defining A *and* defining a at the same time. I think it's a bit easier to read if you make it explicit. This applies to other patches as well. |
Revised after D123519.
I think _LIBCPP_ERROR_CATEGORY_DEFINE_LEGACY_INLINE_FUNCTIONS belongs in __config now, though I just followed suit with what was done in D123519. I need to preprocess out the constexpr/constinit singleton pattern in each of the derived error_category compilation units.
I chose to keep the _LIBCPP_NO_DESTROY and the union pattern even with _LIBCPP_ERROR_CATEGORY_DEFINE_LEGACY_INLINE_FUNCTIONS as it's still useful to bypass calling the destructor and bypass registering the destructor when _LIBCPP_NO_DESTROY is supported. Unfortunately there's no way to bypass the initialization runtime guard with the old ABI, but an improvement nonetheless and the ideal solution moving forward.
libcxx/src/future.cpp | ||
---|---|---|
65 | Is there a reason why you can't simply use the #else branch all the time, and never define _LIBCPP_ERROR_CATEGORY_DEFINE_LEGACY_INLINE_FUNCTIONS in this translation unit (the same applies to other changes)? My memory is blurry from being on vacation, but I think the intent of my D123519 was that you could always assume that error_category is constexpr friendly when building the library. |
libcxx/src/future.cpp | ||
---|---|---|
65 | If that is the case then I think I'm misunderstanding how things work. From __config: #elif _LIBCPP_ABI_VERSION == 1 # if !defined(_LIBCPP_OBJECT_FORMAT_COFF) // Enable compiling copies of now inline methods into the dylib to support // applications compiled against older libraries. This is unnecessary with // COFF dllexport semantics, since dllexport forces a non-inline definition // of inline functions to be emitted anyway. Our own non-inline copy would // conflict with the dllexport-emitted copy, so we disable it. # define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS # endif so there are at least some library builds that will define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS, and when it is, the definition of error_category will not have a constexpr constructor: class _LIBCPP_TYPE_VIS error_category { public: virtual ~error_category() _NOEXCEPT; #if defined(_LIBCPP_ERROR_CATEGORY_DEFINE_LEGACY_INLINE_FUNCTIONS) error_category() noexcept; #else _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11 error_category() _NOEXCEPT = default; #endif So the #if / #else I added are meant to be symmetric to the #if / #else in the error_category definition. |
Commandeering to explain what I mean -- sorry, I figured it was going to be easier since I had something specific in mind.
libcxx/src/ios.cpp | ||
---|---|---|
90 | Actually, since those are constinit, I think we could even define them at file scope and skip the function-local static. Thoughts? |
libcxx/src/ios.cpp | ||
---|---|---|
90 | That makes sense. It does have an advantage in cases where _LIBCPP_NO_DESTROY is not effective. |
libcxx/src/ios.cpp | ||
---|---|---|
90 | If _LIBCPP_NO_DESTROY is not effective, doesn't that mean that the user will pay the startup to register the atexit destruction, even if the category singleton is never used? I think the benefit of the static local remains: that you're not paying for it unless it's used. If _LIBCPP_NO_DESTROY is supported then there's no benefit to using static locals. Are we ok imposing the load time cost when the library is built without _LIBCPP_NO_DESTROY support? |
libcxx/src/ios.cpp | ||
---|---|---|
90 | It's a trade-off between one-time startup cost versus dealing with the guard for every call. |
libcxx/src/ios.cpp | ||
---|---|---|
90 | Turns out there's no guard anyway since they are constinit. |
Re-introduce the union trick which is needed to implement nodestroy semantics on compilers that don't support the nodestroy attribute.
While we're at it, drop the nodestroy attribute since the union trick is enough.
I think we can get rid of those definitions once D123519 lands.