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

[David Holmes]

Hi Thomas,

On 13/07/2020 2:41 pm, Thomas Stüfe wrote:
> On Mon, Jul 13, 2020 at 3:55 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     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.
> 
> 
> Can a compiler reorder system calls and stores? How would it determine 
> if this is safe to do?

A compiler can reorder anything it likes if it can determine it is safe 
to do so. :) In general as it is impractical, if not impossible, to 
make such a determination, the response to such queries has typically 
been "the compiler would never do that". And for gcc and clang we have 
some very knowledgeable folk that can attest to that. For VS, as I 
stated (now below) we assume this is also the case.

> I'd be surprised if Microsoft loosened up reordering since this would 
> mean existing software cannot just be recompiled for arm and expected to 
> work. But this is just a guess of course.

It's an interesting point because I would expect there to be a lot of 
software written for Windows that contains assumptions of TSO that would 
in fact fail when run on Aarch64. I don't know if there are any special 
mechanisms to force a binary to run in TSO mode on Aarch64 under Windows 
(or build flags), that would allow for ease of migration. But unless all 
Windows software will run in such a mode there is a need for MS to 
document what the memory consistency properties of various APIs are (as 
POSIX does [1]). This may already exist in the Windows-Aarch64 "SDK" but 
I have no knowledge of that and so can only see the general win32 
documentation available online.

Cheers,
David

[1] 
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_12

>     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
>     -----
> 
> 
> Thomas