[lockfree] _ENABLE_ATOMIC_ALIGNMENT_FIX for VS 2015 Update 2
Hi,
While running Boost.Lockfree's tests, one of our compiler devs encountered the following failure:
freelist_test.cpp
c:\wcfb01\binaries\x86chk\inc\atomic(659): error C2338: You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later).
c:\wcfb01\binaries\x86chk\inc\atomic(669): note: see reference to class template instantiation 'std::_Atomic_base<_Ty,4>' being compiled
with
[
_Ty=boost::lockfree::detail::tagged_index
]
e:\boost\boost\boost\lockfree\detail\freelist.hpp(608): note: see reference to class template instantiation 'std::atomicboost::lockfree::detail::tagged_index' being compiled
e:\boost\boost\boost\lockfree\detail\freelist.hpp(609): note: see reference to class template instantiation 'boost::lockfree::detail::fixed_size_freelist
On Tuesday, 10 May 2016 10:26:15 MSK Stephan T. Lavavej wrote:
Hi,
While running Boost.Lockfree's tests, one of our compiler devs encountered the following failure:
freelist_test.cpp c:\wcfb01\binaries\x86chk\inc\atomic(659): error C2338: You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later). c:\wcfb01\binaries\x86chk\inc\atomic(669): note: see reference to class template instantiation 'std::_Atomic_base<_Ty,4>' being compiled with [ _Ty=boost::lockfree::detail::tagged_index ] e:\boost\boost\boost\lockfree\detail\freelist.hpp(608): note: see reference to class template instantiation 'std::atomicboost::lockfree::detail::tagged_index' being compiled e:\boost\boost\boost\lockfree\detail\freelist.hpp(609): note: see reference to class template instantiation 'boost::lockfree::detail::fixed_size_freelist
' being compiled call "C:\WCFB01\binaries\x86chk\bin\i386\vcvarsall.bat" x86 >nul cl /Zm800 -nologo @"..\bin.v2\libs\lockfree\test\freelist_test.test\msvc-latest\debug\threadi ng-multi\freelist_test.obj.rsp"
...failed compile-c-c++ ..\bin.v2\libs\lockfree\test\freelist_test.test\msvc-latest\debug\threading -multi\freelist_test.obj...
with the following type:
class tagged_index { public: typedef boost::uint16_t tag_t; typedef boost::uint16_t index_t;
protected: index_t index; tag_t tag; };
This is by design - your tagged_index has align 2, size 4, triggering the need for my atomic alignment fix. As the static_assert advises, your code needs to define _ENABLE_ATOMIC_ALIGNMENT_FIX in order to pass.
If this has already been fixed in 1.61.0, then that's great (I haven't checked).
I think, it's not the library's prerogative to define such macros as this can affect user's code. I.e. the library cannot tell if breaking binary compatibility is ok for users. The code could have worked previously, if the atomic<> value was suitably aligned because of the surrounding code or natural alignment of the memory allocator or if the user specified alignment manually. I guess the test could be fixed by defining the macro for the test, but this won't fix other uses of the library. IMHO, it should be fixed in MSVC. The error should be opt-in, probably with an option of being a warning instead of an error. Or (more preferably) just fix the bug by default, with no errors or warnings, and provide a way to revert to the old behavior on request.
class tagged_index { public: typedef boost::uint16_t tag_t; typedef boost::uint16_t index_t;
protected: index_t index; tag_t tag; };
This is by design - your tagged_index has align 2, size 4, triggering the need for my atomic alignment fix.
boost lockfree ensure alignment of the atomics, so this warning is a false positive. sending a warning is good and nice, but a compile error is overkill imho. this code dates back before c++11 and still supports c++03 compilers. so yes, please make it a warning, but not an error.
Tim Blechmann wrote:
class tagged_index { public: typedef boost::uint16_t tag_t; typedef boost::uint16_t index_t;
protected: index_t index; tag_t tag; };
This is by design - your tagged_index has align 2, size 4, triggering the need for my atomic alignment fix.
boost lockfree ensure alignment of the atomics, so this warning is a false positive. sending a warning is good and nice, but a compile error is overkill imho. this code dates back before c++11 and still supports c++03 compilers.
You can in principle fix that by adding __declspec(align(4)) to tagged_index on MSVC.
On Tuesday, 10 May 2016 17:28:43 MSK Peter Dimov wrote:
Tim Blechmann wrote:
class tagged_index {
public: typedef boost::uint16_t tag_t; typedef boost::uint16_t index_t;
protected: index_t index; tag_t tag;
};
This is by design - your tagged_index has align 2, size 4, triggering the need for my atomic alignment fix.
boost lockfree ensure alignment of the atomics, so this warning is a false positive. sending a warning is good and nice, but a compile error is overkill imho. this code dates back before c++11 and still supports c++03 compilers.
You can in principle fix that by adding __declspec(align(4)) to tagged_index on MSVC.
...or a more portable BOOST_ALIGNMENT(4).
[Andrey Semashev]
IMHO, it should be fixed in MSVC. The error should be opt-in,
Users don't discover opt-in features.
Or (more preferably) just fix the bug by default, with no errors or warnings,
I initially planned to do that, but the fix affects layout, which breaks binary compatibility. [Tim Blechmann]
so yes, please make it a warning, but not an error.
Users ignore warnings. Faced with silent bad codegen for the majority of users (Lockfree is very unusual in that it's manually aligning), or breaking binary compatibility for users who mix RTM/Update 1 with Update 2 code (extremely common, especially when third-party libraries are involved), I felt that the best choice was to detect and emit the static_assert. [Peter Dimov]
You can in principle fix that by adding __declspec(align(4)) to tagged_index on MSVC. [Andrey Semashev] ...or a more portable BOOST_ALIGNMENT(4).
Yeah, that should be ideal - much better than defining the enable-fix macro in the library or the test. Thanks, STL
On Tuesday, 10 May 2016 19:07:27 MSK Stephan T. Lavavej wrote:
[Andrey Semashev]
IMHO, it should be fixed in MSVC. The error should be opt- in,
Users don't discover opt-in features.
It's a bugfix, not a feature. The bug is a nasty one, I understand. But I would have preferred to have the correct and silent behavior rather than having to deal with warnings and config macros to convince the compiler to do the right thing.
On Tuesday, 10 May 2016 19:12:12 MSK you wrote:
On Tuesday, 10 May 2016 19:07:27 MSK Stephan T. Lavavej
wrote:
[Andrey Semashev]
IMHO, it should be fixed in MSVC. The error should be opt-
in,
Users don't discover opt-in features.
It's a bugfix, not a feature. The bug is a nasty one, I understand. But I would have preferred to have the correct and silent behavior rather than having to deal with warnings and config macros to convince the compiler to do the right thing.
BTW, I thought MSVC did not guarantee ABI compatibility between different versions? Did this change?
Andrey Semashev wrote:
BTW, I thought MSVC did not guarantee ABI compatibility between different versions? Did this change?
It obviously depends on whether one considers 2015 update 2 a version different from 2015 update 1.
[STL]
Users don't discover opt-in features.
[Andrey Semashev]
It's a bugfix, not a feature.
I should have said: Users don't discover opt-in anything.
But I would have preferred to have the correct and silent behavior
The problem is that the correct behavior is a binary breaking change.
BTW, I thought MSVC did not guarantee ABI compatibility between different versions? Did this change?
We guarantee bincompat between Updates. We break it between major versions. STL
BTW, I thought MSVC did not guarantee ABI compatibility between different versions? Did this change?
We guarantee bincompat between Updates. We break it between major versions.
doesn't the compile error tells us that this is not the case here?
VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later).
in my interpretation of this the update breaks binary compatibility and by default rejects valid code.
doesn't the compile error tells us that this is not the case here?
If you are triggering that warning then the resulting code was broken (that is, not atomic) before. It's not a bincompat break if it never actually worked.
Billy O'Neal III
SDE - Visual C++ Libraries
________________________________________
From: Boost
BTW, I thought MSVC did not guarantee ABI compatibility between different versions? Did this change?
We guarantee bincompat between Updates. We break it between major versions.
doesn't the compile error tells us that this is not the case here?
VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later).
in my interpretation of this the update breaks binary compatibility and by default rejects valid code. _______________________________________________ Unsubscribe & other changes: https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2flists.boost.org%2fmailman%2flistinfo.cgi%2fboost&data=01%7c01%7cbion%40microsoft.com%7cadae602e54b74ea0d2cd08d3792bbf94%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=CS8gT%2fMRcG4NNqVHTlf6M24xtibfvnGU3iv8z79OFhg%3d
doesn't the compile error tells us that this is not the case here?
If you are triggering that warning then the resulting code was broken (that is, not atomic) before.
it is an error not a warning. i'm a bit confused: was it completely broken (as this statement suggest) or was the generated code correct when alignment was ensured by user code (as stl's statement suggested)?
It's not a bincompat break if it never actually worked.
well, isn't it an ABI change if you need to recompile everything with a specific compiler version? users will run into exactly the same issue, if they use structs as my tagged_index, which is not under their control.
[STL]
We guarantee bincompat between Updates. We break it between major versions.
[Tim Blechmann]
doesn't the compile error tells us that this is not the case here?
This is a special case, upholding the general principle. Because we don't want to silently break binary compatibility, when we're forced with a choice between doing that, or silent bad codegen (since we can't know whether the user is manually aligning, and almost nobody does), we avoid both by emitting a compiler error. This requires user action, either to avoid the scenario (e.g. by increasing the alignment of their type), or to tell the STL "go ahead and activate the fix, I'm not mixing layouts". I tried to explain this in the static_assert to the best of my ability. STL
While running Boost.Lockfree's tests, one of our compiler devs encountered the following failure:
freelist_test.cpp c:\wcfb01\binaries\x86chk\inc\atomic(659): error C2338: You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later). c:\wcfb01\binaries\x86chk\inc\atomic(669): note: see reference to class template instantiation 'std::_Atomic_base<_Ty,4>' being compiled with [ _Ty=boost::lockfree::detail::tagged_index ] e:\boost\boost\boost\lockfree\detail\freelist.hpp(608): note: see reference to class template instantiation 'std::atomicboost::lockfree::detail::tagged_index' being compiled e:\boost\boost\boost\lockfree\detail\freelist.hpp(609): note: see reference to class template instantiation 'boost::lockfree::detail::fixed_size_freelist
' being compiled call "C:\WCFB01\binaries\x86chk\bin\i386\vcvarsall.bat" x86 >nul cl /Zm800 -nologo @"..\bin.v2\libs\lockfree\test\freelist_test.test\msvc-latest\debug\threading-multi\freelist_test.obj.rsp"
...failed compile-c-c++ ..\bin.v2\libs\lockfree\test\freelist_test.test\msvc-latest\debug\threading-multi\freelist_test.obj...
with the following type:
class tagged_index { public: typedef boost::uint16_t tag_t; typedef boost::uint16_t index_t;
protected: index_t index; tag_t tag; };
This is by design - your tagged_index has align 2, size 4, triggering the need for my atomic alignment fix. As the static_assert advises, your code needs to define _ENABLE_ATOMIC_ALIGNMENT_FIX in order to pass.
If this has already been fixed in 1.61.0, then that's great (I haven't checked).
giving it a second thought, my interpretation of the standard suggests that that it is valid code that the compiler should accept. the standard (or n3290, which i have on my machine) tells me:
There is a generic class template atomic<T>. The type of the template argument T shall be trivially copy assignable and bitwise equality comparable.
i cannot find any wording which requires specific alignment requirements
for T. imho this is for a good reason: the type T might not be part of
your codebase, but coming from the API of a third-party library.
even if tagged_index is 2-byte aligned, it it would be perfectly valid
to use atomic
[Note: Operations that are lock-free should also be address-free. That is, atomic operations on the same memory location via two different addresses will communicate atomically. The implementation should not depend on any per-process state. This restriction enables communication by memory that is mapped into a process more than once and by memory that is shared between two processes. — end note ]
----- maybe something changed in the wording of the standard, but from what i can tell this change to msvc will reject valid code. so my suggestion is the following: * make atomic<T> lock-free depending on the address of the instance, use whatever fallback mechanism you have to emulate non-lock-free atomics. * using a spinlock-pool in the run-time library for emulating blocking atomic<>s to ensure that sizeof(T) == sizeof(atomic<T>). afaict the standard doesn't impose any restrictions on this, but leaves this open to the implementation. * issue a warning that T should be properly aligned to ensure that atomic<T> will always be lock-free. afaict you would then end up with a rather standard-compliant implementaton of atomic<T>. cheers, tim
participants (5)
-
Andrey Semashev
-
Billy O'Neal (VC LIBS)
-
Peter Dimov
-
Stephan T. Lavavej
-
Tim Blechmann