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

Workaround for issue #558 #714

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Jan 3, 2022

Signed-off-by: Vladislav Shchapov [email protected]

Description

test_malloc_shutdown_hang (Debug build): Assertion isMallocInitialized() failed

Fixes #558 (workaround)

  • - git commit message contains appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add respective label(s) to PR if you have permissions

  • bug fix - change which fixes an issue
  • new feature - change which adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and for some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

@alexey-katranov, @anton-potapov

Other information

@KFilipek
Copy link
Contributor

I've compiled this branch using W11 and VS 2022, and this issue is not reproducible.
My compiler version:

The CXX compiler identification is MSVC 19.34.31933.0

Since Jan 2023, Windows 7 and Windows 8.1 are not supported (https://learn.microsoft.com/en-us/lifecycle/products/windows-81)

@phprus
Copy link
Contributor Author

phprus commented Apr 21, 2023

@KFilipek

In any way, the initialization of an std::atomic variable is not atomic (https://en.cppreference.com/w/cpp/atomic/atomic/atomic) and because of this, there may be errors.
It's best solution to use std::atomic_ref and zero-initialized variable.

static std::atomic<intptr_t> mallocInitialized; - it's a hack. This code is not valid until C++20, but works on all tested compilers.

I think the best way would be a simple std::atomic_ref replacement wrapper based on __atomic_* or Interlocked* builtins.

@anton-potapov
Copy link
Contributor

@KFilipek

In any way, the initialization of an std::atomic variable is not atomic (https://en.cppreference.com/w/cpp/atomic/atomic/atomic) and because of this, there may be errors. It's best solution to use std::atomic_ref and zero-initialized variable.

static std::atomic<intptr_t> mallocInitialized; - it's a hack. This code is not valid until C++20, but works on all tested compilers.

IMHO in C++ 11 it is perfectly valid code, as in C++11 constructor of std::atomic is intentionally made trivial to allow correct (zero) initialization of global atomics. It is racy in C++20 as the constructor no longer trivial and will be called.

I think the best way would be a simple std::atomic_ref replacement wrapper based on __atomic_* or Interlocked* builtins.

Why not use std::atomic_ref when it is available (__cpp_lib_atomic_ref macro is to help)? e.g. :

#if __cpp_lib_atomic_ref
//intentionaly no init value. Guranteed to be zero-initilized by C++ standard. 
static intptr_t mallocInitializedStorage; 
static std::atomic_ref<intptr_t> mallocInitialized{mallocInitializedStorage};
#else
//intentionaly no init value. Guranteed to be zero-initilized by C++11 standard  (but not C++20)
static std::atomic<intptr_t> mallocInitialized; 
#endif  

@phprus
Copy link
Contributor Author

phprus commented Apr 25, 2023

IMHO in C++ 11 it is perfectly valid code, as in C++11 constructor of std::atomic is intentionally made trivial to allow correct (zero) initialization of global atomics.

Maybe not:

The default-initialized std::atomic does not contain a T object, and its only valid uses are destruction and initialization by std::atomic_init, see LWG 2334
(until C++20)

But I'm not sure.

The problem is the thread-safe of initialization in Windows.
As it turned out, the call oof static variable constructors and the call of doInitialization() can be executed in different threads at the same time.

I will suggest another solution (It does not depend on the thread-safety of the std::atomic_ref constructor):

#if __cpp_lib_atomic_ref
// intentionaly no init value. Guranteed to be zero-initilized by C++ standard. 
static intptr_t mallocInitializedStorage; 
#define mallocInitialized std::atomic_ref<intptr_t>(mallocInitializedStorage)
#else
// intentionaly no init value. Guranteed to be zero-initilized by C++11 standard  (but not C++20)
static std::atomic<intptr_t> mallocInitialized; 
#endif

What do you think about it?

@phprus
Copy link
Contributor Author

phprus commented Apr 25, 2023

PR updated.

@phprus
Copy link
Contributor Author

phprus commented May 22, 2023

Rebased and ping.

Signed-off-by: Vladislav Shchapov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_malloc_shutdown_hang (Debug build): Assertion isMallocInitialized() failed
3 participants