Re: [boost] [release/atomic] request permission to merge 86144

Subject: Re: [boost] [release/atomic] request permission to merge 86144
From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2013-10-14 20:06:55

On Tuesday 15 October 2013 11:51:15 Gavin Lambert wrote:
> On 15/10/2013 03:07, Quoth Andrey Semashev:
> > I don't think the commit is correct.
> 
> I was the original author of the patch, and I can confirm that it is
> correct at least in the fact that DCAS (and other 64-bit atomic
> operations) did not work on VS2008 prior to the patch and does after
> applying it.

The support for 64-bit CAS on 32-bit x86 was indeed added after 1.54 was 
released (the initial commit that adds the support was 84695). After that the 
patch was not needed. The support should be tested by our testers since then, 
so I'm quite sure it should be working. However, I would appreciate if you 
tried out the current trunk sans the commit 86144 to verify it works.

> I did not test on other platforms or compilers but by inspection the
> changes in the patch should be skipped or otherwise harmless in those cases.
> > 1. 64-bit CAS for 32-bit x86 was already supported before. It is
> > controlled by the BOOST_ATOMIC_X86_HAS_CMPXCHG8B macro, which is
> > automatically defined when the compiler targets Pentium or later (may
> > need to be enabled by a compiler switch).
> 
> The patch was originally based on Boost 1.53 and 1.54, in which this
> macro is not defined at all.

I understand the patch might be helpful for these releases, and it might have 
been correct for that code. But the patch was eventually applied to trunk, 
which has a very different code now. It was a very wrong thing to do - to 
blindly apply the patch to the code it was not intended to. I wonder how it 
even applied without complaining.

> It does look like better support for 64-bit compare_exchange_* and
> load/store specifically has been added to trunk (and presumably 1.55)
> since then though.
> 
> But the patch should still add support for fetch_add, fetch_sub, and
> exchange operations, which still appear to be otherwise unsupported in
> trunk prior to the patch as far as I can tell.

No, the trunk code before 86144 had support for all 64-bit operations, both 
for 64 and 32-bit targets. In the latter case, they were implemented through 
DCAS (see atomic/detail/cas64strong.hpp).

> > 2. Your change modifies 64-bit branch, which always have 64-bit atomic ops
> > available (i.e. CAS-based path is not needed). This branch is not used by
> > 32- bit targets.
> 
> No, it's in the #else clause of the 64-bit branch, aka the 32-bit branch.

Are we looking at the same code? The change to windows.hpp starts at line 
1313, which is inside the "#if defined(_M_AMD64) || defined(_M_IA64)" block. 
The rest of the changes are also within this block. I'm looking at the current 
trunk now.

> Probably the changes in interlocked.hpp should now also be wrapped in a
> test for BOOST_ATOMIC_X86_HAS_CMPXCHG8B, since you're right that this
> would be more correct.

That macro is not defined when interlocked.hpp is included. The change to 
interlocked.hpp is not needed because windows.hpp doesn't use the 
_InterlockedCompareExchange64 intrinsic to implement cmpxchg8b.

> But the intrinsic *does* exist on x86 >= Pentium, so defining it here
> means that the assembly code in platform_cmpxchg64_strong is not
> required.  (Having said that, it would probably be harmless to
> completely omit the changes to interlocked.hpp.)
> 
> The changes in windows.hpp still seem relevant as noted above.

Given the above, I don't think so. I'm in favor of reverting that commit.