Based on a patch by Jinoh Kang:
https://gitlab.winehq.org/wine/wine/-/merge_requests/155
This patch simplifies the comment somewhat; I don't see it as necessary to fully explain the concept of memory barriers and acquire/release everywhere that they are used. In my opinion, it's necessary and also sufficient to explain why these specific places need an acquire/release pair, what that pair is protecting, and to cross-reference the parts of the pair with each other.
I've also removed the IOSB aliasing patches. Personally I think that there's no harm in making our code more endianness-conscious, for things like Winelib, but on the other hand Windows never supported big-endian, and we've resisted adding support for architectures Windows doesn't support (and see also [1]). Since it's not clear to me they're desirable, and they're orthogonal to the purpose of this patch set, I've removed them; they can be submitted later if there is a favourable consensus.
[1] https://www.winehq.org/pipermail/wine-devel/2021-July/191600.html
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/kernelbase/file.c | 6 +++++- dlls/ntdll/unix/unix_private.h | 26 ++++++++++++++++++++++---- dlls/ws2_32/socket.c | 6 +++++- 3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index 661bc0c2778..5ecf3049c4a 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -3156,7 +3156,9 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov
TRACE( "(%p %p %p %lu %d)\n", file, overlapped, result, timeout, alertable );
- status = overlapped->Internal; + /* Paired with the write-release in set_async_iosb() in ntdll; see the + * latter for details. */ + status = ReadAcquire( (LONG *)overlapped->Internal ); if (status == STATUS_PENDING) { if (!timeout) @@ -3173,6 +3175,8 @@ BOOL WINAPI DECLSPEC_HOTPATCH GetOverlappedResultEx( HANDLE file, OVERLAPPED *ov return FALSE; }
+ /* We don't need to give this load acquire semantics; the wait above + * already guarantees that the IOSB and output buffer are filled. */ status = overlapped->Internal; if (status == STATUS_PENDING) status = STATUS_SUCCESS; } diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index 73b9ed76de0..2ba5d1df295 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -363,6 +363,24 @@ static inline void set_async_iosb( client_ptr_t iosb, NTSTATUS status, ULONG_PTR { if (!iosb) return;
+ /* GetOverlappedResult() and WSAGetOverlappedResult() expect that if the + * status is written, that the information (and buffer, which was written + * earlier from the async callback) will be available. Hence we need to + * store the status last, with release semantics to ensure that those + * writes are visible. This release is paired with a read-acquire in + * GetOverlappedResult() and WSAGetOverlappedResult(): + * + * CPU 0 (set_async_iosb) CPU 1 (GetOverlappedResultEx) + * =========================== =========================== + * write buffer + * write Information + * WriteRelease(Status) <--------. + * | + * | + * (paired with) `-> ReadAcquire(Status) + * read Information + */ + if (in_wow64_call()) { struct iosb32 @@ -370,18 +388,18 @@ static inline void set_async_iosb( client_ptr_t iosb, NTSTATUS status, ULONG_PTR NTSTATUS Status; ULONG Information; } *io = wine_server_get_ptr( iosb ); - io->Status = status; io->Information = info; + WriteRelease( (LONG *)&io->Status, status ); } else { IO_STATUS_BLOCK *io = wine_server_get_ptr( iosb ); + io->Information = info; #ifdef NONAMELESSUNION - io->u.Status = status; + WriteRelease( (LONG *)&io->u.Status, status ); #else - io->Status = status; + WriteRelease( (LONG *)&io->Status, status ); #endif - io->Information = info; } }
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c index 9e447f29d8c..af7b45026ce 100644 --- a/dlls/ws2_32/socket.c +++ b/dlls/ws2_32/socket.c @@ -3710,7 +3710,9 @@ BOOL WINAPI WSAGetOverlappedResult( SOCKET s, LPWSAOVERLAPPED lpOverlapped, return FALSE; }
- status = lpOverlapped->Internal; + /* Paired with the write-release in set_async_iosb() in ntdll; see the + * latter for details. */ + status = ReadAcquire( (LONG *)lpOverlapped->Internal ); if (status == STATUS_PENDING) { if (!fWait) @@ -3722,6 +3724,8 @@ BOOL WINAPI WSAGetOverlappedResult( SOCKET s, LPWSAOVERLAPPED lpOverlapped, if (WaitForSingleObject( lpOverlapped->hEvent ? lpOverlapped->hEvent : SOCKET2HANDLE(s), INFINITE ) == WAIT_FAILED) return FALSE; + /* We don't need to give this load acquire semantics; the wait above + * already guarantees that the IOSB and output buffer are filled. */ status = lpOverlapped->Internal; }
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=126111
Your paranoid android.
=== debian11 (build log) ===
Error: Mount manager not running, most likely your WINEPREFIX wasn't created correctly. Task: WineTest did not produce the win32 report
=== debian11b (build log) ===
Error: Mount manager not running, most likely your WINEPREFIX wasn't created correctly. Task: WineTest did not produce the wow64 report
This merge request was approved by Zebediah Figura.
Jinoh Kang (@iamahuman) commented about dlls/kernelbase/file.c:
You forgot to take the address of `overlapped->Internal`.
```suggestion:-0+0 status = ReadAcquire( (LONG *)&overlapped->Internal ); ```
Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/unix_private.h:
Why `(LONG *)`? (same for other accesses)
Jinoh Kang (@iamahuman) commented about dlls/ws2_32/socket.c:
You forgot to take the address of `overlapped->Internal`.
```suggestion:-0+0 status = ReadAcquire( (LONG *)&lpOverlapped->Internal ); ```
This merge request was approved by Jinoh Kang.
So this basically brings us back to v9 of !155: https://gitlab.winehq.org/wine/wine/-/merge_requests/155/diffs?commit_id=354....
Note that this v9 patch was submitted *before* I eventually asked if we "need comments on read-acq side as well" on IRC; then, I interpreted your response "yes, both sides should have comments" in a way that requires both side to have detailed comments.
Perhaps I should have mentioned that I have made several modifications before asking there, so that you didn't have to work with outdated information.
Either way, thanks for your clarification!
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin -
Zebediah Figura (＠zfigura)