Page MenuHomePhabricator

[libcxx] Avoid destructor call for error_category singletons
Needs RevisionPublic

Authored by andusy on Aug 2 2019, 11:16 AM.

Details

Summary

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.

Diff Detail

Event Timeline

andusy created this revision.Aug 2 2019, 11:16 AM
EricWF added a comment.Aug 2 2019, 5:22 PM

I think a better fix would be to apply [[clang::no_destroy]] to the category variable definitions instead.

EricWF added a comment.Aug 2 2019, 5:24 PM

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 requested changes to this revision.Aug 2 2019, 5:24 PM
This revision now requires changes to proceed.Aug 2 2019, 5:24 PM

@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

@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".

mclow.lists added a comment.EditedAug 5 2019, 10:28 AM

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 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.

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.)

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++.

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.

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?

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).

andusy updated this revision to Diff 215674.EditedAug 16 2019, 2:11 PM

I've updated the patch based on @EricWF's earlier comment.

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

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 believe the new patch no longer adds more code that runs before main.

mclow.lists added inline comments.Aug 16 2019, 2:43 PM
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.
It's clear what the intent is.

You have five #ifdefs in this patch.
I'd prefer to see only one ifdef, in <__config> where no one will ever look at it.

That's why we have macros like _LIBCPP_CONSTEXPR_AFTER_CXX11

mclow.lists added inline comments.Aug 16 2019, 2:47 PM
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?

hubert.reinterpretcast edited the summary of this revision. (Show Details)Aug 16 2019, 2:49 PM
hubert.reinterpretcast retitled this revision from [libcxx] Early-initialize error_category singletons to [libcxx] Avoid destructor call for error_category singletons.
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.

mclow.lists added inline comments.Aug 16 2019, 4:01 PM
libcxx/src/system_error.cpp
223

Sorry, I misread the union/struct nesting. You are correct.

andusy updated this revision to Diff 215975.Aug 19 2019, 1:46 PM
  • 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.

I'm not seeing unresolved comments. This patch LGTM.

EricWF requested changes to this revision.Aug 22 2019, 10:53 PM

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.

This revision now requires changes to proceed.Aug 22 2019, 10:53 PM

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.

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?

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.

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.

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.