Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes std::call_once reentrancy problem #2743

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Conversation

avtolstoy
Copy link
Member

@avtolstoy avtolstoy commented Feb 21, 2024

Problem

std::call_once implementation in libstdc++ is not re-entrant and uses a common shared mutex:

// XXX: This is not defined in our toolchain, no thread local storage (TLS)
#undef _GLIBCXX_HAVE_TLS
// XXX

  template<typename _Callable, typename... _Args>
    void
    call_once(once_flag& __once, _Callable&& __f, _Args&&... __args)
    {
      // _GLIBCXX_RESOLVE_LIB_DEFECTS
      // 2442. call_once() shouldn't DECAY_COPY()
      auto __callable = [&] {
	  std::__invoke(std::forward<_Callable>(__f),
			std::forward<_Args>(__args)...);
      };
#ifdef _GLIBCXX_HAVE_TLS
      __once_callable = std::__addressof(__callable);
      __once_call = []{ (*(decltype(__callable)*)__once_callable)(); };
#else
      unique_lock<mutex> __functor_lock(__get_once_mutex());
      __once_functor = __callable;
      __set_once_functor_lock_ptr(&__functor_lock);
#endif

      int __e = __gthread_once(&__once._M_once, &__once_proxy);

#ifndef _GLIBCXX_HAVE_TLS
      if (__functor_lock)
        __set_once_functor_lock_ptr(0);
#endif

#ifdef __clang_analyzer__
      // PR libstdc++/82481
      __once_callable = nullptr;
      __once_call = nullptr;
#endif

      if (__e)
	__throw_system_error(__e);
    }

Solution

A workaround for that. Going forward we should ideally get rid of std::call_once usage altogether and replace with our own implementation. Supporting __gthread_xxx implementations for std internals which constantly change between GCC releases is not trivial.

Steps to Test

Not adding tests as std::call_once platform-specific implementations are not exposed to the application anyway.

M SoM build out of this branch should correctly boot in to the application.

Example App

N/A

References

N/A


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@avtolstoy avtolstoy added this to the 5.8.0 milestone Feb 21, 2024
Copy link
Member

@scott-brust scott-brust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked some questions to make sure I understand the behavior

GTHREAD_ONCE_STATE_INITIALIZED = 2
} __gthread_once_state_t;

typedef std::atomic<__gthread_once_state_t> __gthread_once_t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __gthread_once_t type is an atomic type with the above enum.
We use the standard atomic operations to load / compare the once state value and use that to decide if we need to call the once function or not.

Is this doing the same thing as once_flag?
Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an actual underlying implementation of std::once_flag. It's platform-specific, so we have to provide it. This is not new one, I just moved some things around. See deleted gtrh-default.h

extern "C" int __gthread_once (__gthread_once_t* once, void (*func) (void))
{
if (once->load(std::memory_order_acquire) == GTHREAD_ONCE_STATE_INITIALIZED) {
// Short path, already initialized
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case where func has already been called once and nothing needs to be done?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

// Make a local copy, this is a workaround for a non-entrant implementation in libstdc++
auto callable = std::__once_functor;
auto lock = std::__get_once_functor_lock_ptr();
if (lock) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What case would __get_once_functor_lock_ptr return a non zero value?
I see below we set it back to null with __set_once_functor_lock_ptr(0)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a precaution, but this should always be not null. This is a pointer to unique_lock<mutex> __functor_lock(__get_once_mutex()) created right before call into this function.

if (once->compare_exchange_strong(state, GTHREAD_ONCE_STATE_RUNNING,
std::memory_order_acq_rel)) {
// Switched to initializing/running state
callable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the actual one time function is called right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

once->store(GTHREAD_ONCE_STATE_INITIALIZED, std::memory_order_release);
} else {
// Wait for some other thread to complete initialization
while (once->load(std::memory_order_acquire) == GTHREAD_ONCE_STATE_RUNNING) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else condition here is when the once state is something other than GTHREAD_ONCE_STATE_NOT_INITIALIZED ?
This can only happen if another thread is calling __gthread_once with the same once state object?
In this scenario we just yield until the other thread completes and the state variable is equal, then return?
We dont need to do any kind of atomic compare since we are polling it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is for RUNNING state: both threads fell through the first UNINITIALIZED check, the other thread managed to 'acquire' the atomic and set it to RUNNING + run callable(), our thread failed to acquire the atomic at that time and just waits for the second thread to complete running callable() and set state to INITIALIZED

@avtolstoy avtolstoy merged commit ac77bcd into develop Feb 22, 2024
13 checks passed
@avtolstoy avtolstoy deleted the fix/std-once-reentrancy branch February 22, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants