Workaround for issue #558

[phprus]

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

Description

test_malloc_shutdown_hang (Debug build): Assertion isMallocInitialized() failed

Fixes #558 (workaround)

• [x] - 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

• [x] 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
• [x] not needed

Documentation

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

Breaks backward compatibility

• [ ] Yes
• [x] No
• [ ] Unknown

Notify the following users

@alexey-katranov, @anton-potapov

Other information

[KFilipek]

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]

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

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

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<T> 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]

PR updated.

[phprus]

Rebased and ping.