This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Avoid destructor call for error_category singletons
ClosedPublic

Authored by ldionne 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.

thakis added a subscriber: thakis.Aug 6 2019, 4:00 PM

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
202

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
202

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
202

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–86

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

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.

Quuxplusone added inline comments.
libcxx/src/system_error.cpp
222–230

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.

cebowleratibm added inline comments.Mar 1 2022, 5:03 AM
libcxx/src/system_error.cpp
222–230

I think the constexpr version will hit this error:

error: constexpr variable cannot have non-literal type 'const U'

Quuxplusone added inline comments.Mar 1 2022, 7:07 AM
libcxx/src/system_error.cpp
222–230

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
222–230

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'd like to take over this patch and rebase a revision if there are no objections.

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.

Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 7:14 AM
cebowleratibm commandeered this revision.Mar 29 2022, 10:03 AM
cebowleratibm added a reviewer: andusy.

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;
...
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 29 2022, 10:09 AM

Thanks for reviving this! I have a few comments and I also created D123519 to try and simplify this patch. I think i'm fine with this once it is rebased onto D123519.

libcxx/include/__config
760–764 ↗(On Diff #418912)

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.

cebowleratibm updated this revision to Diff 423779.EditedApr 19 2022, 6:26 PM

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.

ldionne added inline comments.Apr 24 2022, 9:37 AM
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.

cebowleratibm added inline comments.May 2 2022, 2:04 PM
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.

ldionne commandeered this revision.Jun 10 2022, 7:09 AM
ldionne added a reviewer: cebowleratibm.

Commandeering to explain what I mean -- sorry, I figured it was going to be easier since I had something specific in mind.

ldionne updated this revision to Diff 435906.Jun 10 2022, 7:10 AM

Rebase and implement my suggestion.

ldionne added inline comments.Jun 10 2022, 7:33 AM
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.

cebowleratibm added inline comments.Jun 23 2022, 2:08 PM
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.

ldionne marked 3 inline comments as done.Sep 1 2023, 8:27 AM
ldionne added inline comments.
libcxx/src/ios.cpp
90

Turns out there's no guard anyway since they are constinit.

ldionne updated this revision to Diff 555391.Sep 1 2023, 8:27 AM
ldionne marked an inline comment as done.

Rebase and remove the union trick which I don't think is necessary anymore.

ldionne accepted this revision.Sep 1 2023, 8:28 AM

I'll ship this if CI is green.

ldionne updated this revision to Diff 555427.Sep 1 2023, 10:11 AM

Run clang-format on new .cpp file.

ldionne updated this revision to Diff 555865.Sep 5 2023, 7:38 AM

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.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 5 2023, 10:45 AM
This revision was automatically updated to reflect the committed changes.