[PATCH v2 0/3] MR5035: Change SetLastError to RestoreLastError if appropriate
Theoretically, this allows debuggers to tell them apart. https://devblogs.microsoft.com/oldnewthing/20110429-00/?p=10803 But in practice, this is more about cleanliness than anything else; I'm not aware of anyone running SetLastError-aware debuggers in Wine. Feel free to reject if this is a waste of time. The changeable SetLastError calls were found by this script: ```python #!/usr/bin/env python3 import os import re for (dirpath, dirnames, filenames) in os.walk("."): for fn in filenames: if not fn.endswith(".c"): continue text = open(dirpath+"/"+fn,"rt").read() text = text.replace("RtlSetLastWin32Error","SetLastError") text = text.replace("RtlGetLastWin32Error","GetLastError") if "SetLastError" not in text: continue varname = None for line in text.split("\n"): if "GetLastError" in line: if m := re.search(r"([A-Za-z0-9_]+) *= *GetLastError", line): prevline = line varname = m[1] if line == "}": varname = None if varname is not None and "SetLastError" in line: if m := re.search(r"SetLastError\( *([A-Za-z0-9_]+) *\)", line): if m[1] == varname: print(dirpath+"/"+fn, prevline, line) ``` The script also flags https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/mpr/wnet.c#L2811, which is half set and half restore; I chose to left it unchanged. Idea from https://gitlab.winehq.org/wine/wine/-/merge_requests/5020/diffs#6c845445f68f... -- v2: win32u: Use RtlRestoreLastWin32Error instead of RtlSetLastWin32Error if appropriate everywhere: Change SetLastError to RestoreLastError if appropriate ntdll: Duplicate RtlSetLastWin32Error to RtlRestoreLastWin32Error https://gitlab.winehq.org/wine/wine/-/merge_requests/5035
From: Alfred Agrell <floating(a)muncher.se> This allows debuggers to tell them apart --- dlls/ntdll/error.c | 18 +++++++++++++++++- dlls/ntdll/ntdll.spec | 2 +- include/winbase.h | 5 +++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/error.c b/dlls/ntdll/error.c index 5412b19907c..6b914f226e5 100644 --- a/dlls/ntdll/error.c +++ b/dlls/ntdll/error.c @@ -109,7 +109,6 @@ DWORD WINAPI RtlGetLastWin32Error(void) /*********************************************************************** * RtlSetLastWin32Error (NTDLL.@) - * RtlRestoreLastWin32Error (NTDLL.@) * * Set the per-thread error value. * @@ -124,6 +123,23 @@ void WINAPI RtlSetLastWin32Error( DWORD err ) NtCurrentTeb()->LastErrorValue = err; } +/*********************************************************************** + * RtlRestoreLastWin32Error (NTDLL.@) + * + * Set the per-thread error value. + * Identical to the above; duplicated to allow debugger breakpoints to tell them apart. + * + * PARAMS + * err [I] The new error value to set + * + * RETURNS + * Nothing. + */ +void WINAPI RtlRestoreLastWin32Error( DWORD err ) +{ + NtCurrentTeb()->LastErrorValue = err; +} + /*********************************************************************** * RtlSetLastWin32ErrorAndNtStatusFromNtStatus (NTDLL.@) * diff --git a/dlls/ntdll/ntdll.spec b/dlls/ntdll/ntdll.spec index 64fdb6fdb24..b3650d7113d 100644 --- a/dlls/ntdll/ntdll.spec +++ b/dlls/ntdll/ntdll.spec @@ -968,7 +968,7 @@ @ stdcall RtlRemoveVectoredExceptionHandler(ptr) @ stdcall RtlResetRtlTranslations(ptr) @ cdecl RtlRestoreContext(ptr ptr) -@ stdcall RtlRestoreLastWin32Error(long) RtlSetLastWin32Error +@ stdcall RtlRestoreLastWin32Error(long) @ stub RtlRevertMemoryStream @ stub RtlRunDecodeUnicodeString @ stub RtlRunEncodeUnicodeString diff --git a/include/winbase.h b/include/winbase.h index fe143cc7a61..92a4416de88 100644 --- a/include/winbase.h +++ b/include/winbase.h @@ -3029,6 +3029,11 @@ static FORCEINLINE void WINAPI SetLastError( DWORD err ) *(DWORD *)((void **)NtCurrentTeb() + 13) = err; } +static FORCEINLINE void WINAPI RestoreLastError( DWORD err ) +{ + SetLastError(err); +} + #else /* __WINESRC__ */ WINBASEAPI HANDLE WINAPI GetCurrentProcess(void); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5035
From: Alfred Agrell <floating(a)muncher.se> --- dlls/advapi32/tests/security.c | 2 +- dlls/comctl32/tests/edit.c | 2 +- dlls/crypt32/protectdata.c | 2 +- dlls/dbghelp/tests/dbghelp.c | 2 +- dlls/mciwave/mciwave.c | 2 +- dlls/mspatcha/pa19.c | 4 ++-- dlls/msvcrt/thread.c | 2 +- dlls/riched20/tests/editor.c | 2 +- dlls/riched20/tests/richole.c | 2 +- dlls/setupapi/misc.c | 2 +- dlls/user32/clipboard.c | 2 +- dlls/user32/tests/clipboard.c | 6 +++--- dlls/user32/tests/edit.c | 2 +- dlls/user32/tests/msg.c | 2 +- dlls/winecrt0/debug.c | 2 +- 15 files changed, 18 insertions(+), 18 deletions(-) diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c index 2840f6bd75a..0ce1e515387 100644 --- a/dlls/advapi32/tests/security.c +++ b/dlls/advapi32/tests/security.c @@ -133,7 +133,7 @@ static const char* debugstr_sid(PSID sid) LocalFree(sidstr); } /* Restore the last error in case ConvertSidToStringSidA() modified it */ - SetLastError(le); + RestoreLastError(le); return res; } diff --git a/dlls/comctl32/tests/edit.c b/dlls/comctl32/tests/edit.c index bce0215ef19..cab5c918b67 100644 --- a/dlls/comctl32/tests/edit.c +++ b/dlls/comctl32/tests/edit.c @@ -94,7 +94,7 @@ static BOOL open_clipboard(HWND hwnd) */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return ret; } Sleep(15); diff --git a/dlls/crypt32/protectdata.c b/dlls/crypt32/protectdata.c index f7d9f439872..3a5997ea1a8 100644 --- a/dlls/crypt32/protectdata.c +++ b/dlls/crypt32/protectdata.c @@ -729,7 +729,7 @@ BOOL load_encryption_key(HCRYPTPROV hProv, DWORD key_len, const DATA_BLOB *salt, szUsername[0]='\0'; GetUserNameA( szUsername, &dwUsernameLen ); } - SetLastError(dwError); + RestoreLastError(dwError); /* salt the hash with: * - the user id diff --git a/dlls/dbghelp/tests/dbghelp.c b/dlls/dbghelp/tests/dbghelp.c index d1c2f219533..7c79609def1 100644 --- a/dlls/dbghelp/tests/dbghelp.c +++ b/dlls/dbghelp/tests/dbghelp.c @@ -748,7 +748,7 @@ static enum process_kind get_process_kind(HANDLE process) { DWORD gle = GetLastError(); enum process_kind pcskind = get_process_kind_internal(process); - SetLastError(gle); + RestoreLastError(gle); return pcskind; } diff --git a/dlls/mciwave/mciwave.c b/dlls/mciwave/mciwave.c index f2af38b8762..e373d1c093e 100644 --- a/dlls/mciwave/mciwave.c +++ b/dlls/mciwave/mciwave.c @@ -1445,7 +1445,7 @@ static DWORD WAVE_mciSave(MCIDEVICEID wDevID, DWORD dwFlags, LPMCI_SAVE_PARMSW l */ tmpRet = GetLastError(); DeleteFileW (lpParms->lpfilename); - SetLastError(tmpRet); + RestoreLastError(tmpRet); /* FIXME: Open file.wav; Save; must not rename the original file. * Nor must Save a.wav; Save b.wav rename a. */ diff --git a/dlls/mspatcha/pa19.c b/dlls/mspatcha/pa19.c index 4b21b199ed8..93abf45366f 100644 --- a/dlls/mspatcha/pa19.c +++ b/dlls/mspatcha/pa19.c @@ -903,7 +903,7 @@ close_old_map: close_patch_map: CloseHandle(patch_map); - SetLastError(err); + RestoreLastError(err); return res; } @@ -965,7 +965,7 @@ close_patch_file: CloseHandle(patch_hndl); /* set last error even on success as per windows */ - SetLastError(err); + RestoreLastError(err); return res; } diff --git a/dlls/msvcrt/thread.c b/dlls/msvcrt/thread.c index 6aad9a22d47..1462340303d 100644 --- a/dlls/msvcrt/thread.c +++ b/dlls/msvcrt/thread.c @@ -61,7 +61,7 @@ thread_data_t *CDECL msvcrt_get_thread_data(void) ptr->module = NULL; #endif } - SetLastError( err ); + RestoreLastError( err ); return ptr; } diff --git a/dlls/riched20/tests/editor.c b/dlls/riched20/tests/editor.c index f8abb7a3e65..49662207d69 100644 --- a/dlls/riched20/tests/editor.c +++ b/dlls/riched20/tests/editor.c @@ -4835,7 +4835,7 @@ static BOOL open_clipboard_(int line, HWND hwnd) */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace_(__FILE__, line)("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return ret; } Sleep(15); diff --git a/dlls/riched20/tests/richole.c b/dlls/riched20/tests/richole.c index fb5177b83e9..c5ace5f589c 100644 --- a/dlls/riched20/tests/richole.c +++ b/dlls/riched20/tests/richole.c @@ -4955,7 +4955,7 @@ static BOOL open_clipboard(HWND hwnd) */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return ret; } Sleep(15); diff --git a/dlls/setupapi/misc.c b/dlls/setupapi/misc.c index b1083fca665..9d03787c385 100644 --- a/dlls/setupapi/misc.c +++ b/dlls/setupapi/misc.c @@ -456,7 +456,7 @@ BOOL WINAPI FileExists(LPCWSTR lpFileName, LPWIN32_FIND_DATAW lpFileFindData) { dwError = GetLastError(); SetErrorMode(uErrorMode); - SetLastError(dwError); + RestoreLastError(dwError); return FALSE; } diff --git a/dlls/user32/clipboard.c b/dlls/user32/clipboard.c index 72c7720dc17..d6cb3e88dae 100644 --- a/dlls/user32/clipboard.c +++ b/dlls/user32/clipboard.c @@ -56,7 +56,7 @@ static const char *debugstr_format( UINT id ) WCHAR buffer[256]; DWORD le = GetLastError(); BOOL r = NtUserGetClipboardFormatName( id, buffer, 256 ); - SetLastError(le); + RestoreLastError(le); if (r) return wine_dbg_sprintf( "%04x %s", id, debugstr_w(buffer) ); diff --git a/dlls/user32/tests/clipboard.c b/dlls/user32/tests/clipboard.c index 87df06b6621..90d9f301721 100644 --- a/dlls/user32/tests/clipboard.c +++ b/dlls/user32/tests/clipboard.c @@ -57,7 +57,7 @@ static BOOL open_clipboard_(int line, HWND hwnd) */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace_(__FILE__, line)("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return ret; } Sleep(15); @@ -80,11 +80,11 @@ static BOOL has_no_open_wnd_(int line) /* See open_clipboard() */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace_(__FILE__, line)("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return FALSE; } Sleep(15); - SetLastError(le); + RestoreLastError(le); } } diff --git a/dlls/user32/tests/edit.c b/dlls/user32/tests/edit.c index bbde9b16616..aff286907c1 100644 --- a/dlls/user32/tests/edit.c +++ b/dlls/user32/tests/edit.c @@ -54,7 +54,7 @@ static BOOL open_clipboard(HWND hwnd) */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return ret; } Sleep(15); diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 3414dda6e5a..406d714e0e5 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -17965,7 +17965,7 @@ static BOOL open_clipboard_(int line, HWND hwnd) */ GetClassNameA(clipwnd, classname, ARRAY_SIZE(classname)); trace_(__FILE__, line)("%p (%s) opened the clipboard\n", clipwnd, classname); - SetLastError(le); + RestoreLastError(le); return ret; } Sleep(15); diff --git a/dlls/winecrt0/debug.c b/dlls/winecrt0/debug.c index 2ac4505fb85..e150aa20289 100644 --- a/dlls/winecrt0/debug.c +++ b/dlls/winecrt0/debug.c @@ -54,7 +54,7 @@ static void load_func( void **func, const char *name, void *def ) HMODULE module = GetModuleHandleW( L"ntdll.dll" ); void *proc = GetProcAddress( module, name ); InterlockedExchangePointer( func, proc ? proc : def ); - SetLastError( err ); + RestoreLastError( err ); } } #define LOAD_FUNC(name) load_func( (void **)&p ## name, #name, fallback ## name ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5035
From: Alfred Agrell <floating(a)muncher.se> --- dlls/win32u/clipboard.c | 2 +- dlls/win32u/spy.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/win32u/clipboard.c b/dlls/win32u/clipboard.c index 6cf484a56ca..8b8c5f0ea2d 100644 --- a/dlls/win32u/clipboard.c +++ b/dlls/win32u/clipboard.c @@ -58,7 +58,7 @@ static const char *debugstr_format( UINT id ) WCHAR buffer[256]; DWORD le = RtlGetLastWin32Error(); BOOL r = NtUserGetClipboardFormatName( id, buffer, ARRAYSIZE(buffer) ); - RtlSetLastWin32Error(le); + RtlRestoreLastWin32Error(le); if (r) return wine_dbg_sprintf( "%04x %s", id, debugstr_w(buffer) ); diff --git a/dlls/win32u/spy.c b/dlls/win32u/spy.c index 20a57803cb6..a0422d69434 100644 --- a/dlls/win32u/spy.c +++ b/dlls/win32u/spy.c @@ -2220,7 +2220,7 @@ const char *debugstr_msg_name( UINT msg, HWND hWnd ) ext_sp_e.wParam = 0; ext_sp_e.wnd_class[0] = 0; SPY_GetMsgStuff(&ext_sp_e); - RtlSetLastWin32Error( save_error ); + RtlRestoreLastWin32Error( save_error ); return wine_dbg_sprintf("%s", ext_sp_e.msg_name); } @@ -2505,7 +2505,7 @@ static void SPY_DumpStructure(const SPY_INSTANCE *sp_e, BOOL enter) /* save and restore error code over the next call */ save_error = RtlGetLastWin32Error(); NtUserGetClassName( pnmh->hwndFrom, FALSE, &str ); - RtlSetLastWin32Error(save_error); + RtlRestoreLastWin32Error(save_error); if (wcscmp(TOOLBARCLASSNAMEW, from_class) == 0) dumplen = sizeof(NMTBCUSTOMDRAW)-sizeof(NMHDR); } else if ( pnmh->code >= HDN_ENDDRAG @@ -2639,7 +2639,7 @@ void spy_enter_message( INT iFlag, HWND hWnd, UINT msg, WPARAM wParam, LPARAM lP break; } set_indent_level( indent + SPY_INDENT_UNIT ); - RtlSetLastWin32Error( save_error ); + RtlRestoreLastWin32Error( save_error ); } @@ -2684,5 +2684,5 @@ void spy_exit_message( INT iFlag, HWND hWnd, UINT msg, LRESULT lReturn, SPY_DumpStructure(&sp_e, FALSE); break; } - RtlSetLastWin32Error( save_error ); + RtlRestoreLastWin32Error( save_error ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5035
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=142836 Your paranoid android. === debian11 (build log) === /home/winetest/tools/testbot/var/wine-win32/../wine/dlls/win32u/clipboard.c:61: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-win32/../wine/dlls/win32u/spy.c:2508: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-win32/../wine/dlls/win32u/spy.c:2223: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-win32/../wine/dlls/win32u/spy.c:2642: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-win32/../wine/dlls/win32u/spy.c:2687: undefined reference to `RtlRestoreLastWin32Error' collect2: error: ld returned 1 exit status Task: The win32 Wine build failed === debian11b (build log) === /home/winetest/tools/testbot/var/wine-wow64/../wine/dlls/win32u/clipboard.c:61: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-wow64/../wine/dlls/win32u/spy.c:2508: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-wow64/../wine/dlls/win32u/spy.c:2223: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-wow64/../wine/dlls/win32u/spy.c:2642: undefined reference to `RtlRestoreLastWin32Error' /home/winetest/tools/testbot/var/wine-wow64/../wine/dlls/win32u/spy.c:2687: undefined reference to `RtlRestoreLastWin32Error' collect2: error: ld returned 1 exit status Task: The wow64 Wine build failed
To me it looks that when used with `__WINESRC__` this won't change anything at all. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5035#note_60260
participants (4)
-
Alfred Agrell -
Alfred Agrell (＠Alcaro)
-
Marvin -
Nikolay Sivov (＠nsivov)