[aarch64-port-dev ] RFR(S): 8248657: Windows: strengthening in ThreadCritical regarding memory model

[David Holmes]

On 13/07/2020 12:25 am, Andrew Haley wrote:
> On 11/07/2020 15:15, Kim Barrett wrote:
> 
>  > This change seems to me to make the code noticeably simpler and
>  > easier to understand, while also making it platform-independent
>  > (eliminating a non-TSO race).
> 
> Which is, let us not forget, undefined behaviour. It's best to treat
> all such cases as bugs, even if they don't affect x86. But it's not
> always non-TSO machines that come out badly: this reminds me of
> JDK-8225716, a race condition which only showed on x86-32.
> 
>  > Those seem like good things, regardless of any aarch64 port that
>  > might be coming.
> 
> Indeed.
> 
> My opinion is that unnecessary platform dependencies are in effect
> technical debt. Another example is the use of non-portable integer
> types in AArch64 -- to a large extent my doing -- and it makes sense
> to do the cleanup in mainline. That way the Windows import patches
> will be as clean and simple as they can be.

We'll have to agree to disagree on which side of the "general cleanup" 
versus "part of the Windows-aarch64 port" fence this change sits. But I 
won't push further on that aspect.

But if we are dealing with non-TSO races then it would be good to get 
some guidance from Microsoft as to the memory ordering properties of 
various API's to ensure that we are maintaining correct ordering. For 
example, in the destructor we have:

81     lock_owner = 0;
82     // No lost wakeups, lock_event stays signaled until reset.
83     DWORD ret = SetEvent(lock_event);

but unless we are guaranteed that the store to lock_owner cannot be 
reordered by the compiler or the hardware, to appear to be after the 
SetEvent, then the logic is broken. Generally, because Windows only 
supported TSO systems, we have assumed that the compiler will not 
reorder code across these kind of API calls. But now we also need 
hardware guarantees.

Overall I do like the initialization cleanup to use the "init once" API.

Thanks,
David
-----