search

boostorg / lockfree

Boost.Lockfree
126 stars 86 forks source link

alignment change in 1.77 beta breaks VS 2017 builds #71

Open thebrandre opened 3 years ago

thebrandre commented 3 years ago

The underlying issue with the VS2017 compiler is shown here on godbolt.

The error was triggered in our code base (which works fine with boost-1.76) where we have boost::lockfree::queue in classes managed by shared_ptr. It is probably due to this commit.

Note that boost::lockfree::queue<int, boost::lockfree::capacity<N>>, which is already pretty large, increases by 60 bytes.

Of course, there is an easy fix by defining _ENABLE_EXTENDED_ALIGNED_STORAGE, but there is a reason they did not enable this by default.

This issue is obviously not a deal breaker but it will make upgrading boost from 1.76 to 1.77 troublesome for many users.

timblechmann commented 3 years ago

but there is a reason they did not enable this by default.

afaict they did not enable this by default, as it's a subtle change to the ABI.

i'd rather not change the code, though: it addressed an ubsan error

thebrandre commented 3 years ago

However ...

I am not sure if this is worth it.

Did you make sure that both changes are needed for ubsan compatibility? Only

struct BOOST_ALIGNMENT(BOOST_LOCKFREE_CACHELINE_BYTES) compiletime_sized_freelist_storage

causes problems.

Replacing std::allocator<T> by boost::alignment::aligned_allocator<T, BOOST_LOCKFREE_CACHELINE_BYTES> works fine ... (which is a shame because this one could be resolved more easily).

Can you maybe add a hint to the release notes? Because this issue is rather hard to track. The error message of VS2017 will point out only the struct/class that contains the boost::lockfree::queue and is created via std::make_shared.

timblechmann commented 3 years ago

it has a performance impact for all users due to the stack size increase

boost.lockfree structs use deliberate padding to prevent false sharing.

Did you make sure that both changes are needed for ubsan compatibility?

iirc one change was needed for compile-time sizeing the other for run-time sizing.