[PATCH v3 0/3] MR5035: Change SetLastError to RestoreLastError if appropriate

[Alfred Agrell (＠Alcaro)]

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") and not fn.endswith(".h"):
			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...

--
  v3: win32u: Use RtlRestoreLastWin32Error instead of RtlSetLastWin32Error if appropriate
      everywhere: Change SetLastError to RestoreLastError if appropriate
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5035

[PATCH v3 1/3] ntdll: Duplicate RtlSetLastWin32Error to RtlRestoreLastWin32Error

[Alfred Agrell]

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

[PATCH v3 2/3] everywhere: Change SetLastError to RestoreLastError if appropriate

[Alfred Agrell]

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 +-
 include/wine/test.h            | 2 +-
 16 files changed, 19 insertions(+), 19 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 )
diff --git a/include/wine/test.h b/include/wine/test.h
index 8f3f3bb0d50..84a721a71b0 100644
--- a/include/wine/test.h
+++ b/include/wine/test.h
@@ -637,7 +637,7 @@ struct winetest_thread_data *winetest_get_thread_data(void)
         data->str_pos = data->strings;
         TlsSetValue( tls_index, data );
     }
-    SetLastError( last_error );
+    RestoreLastError( last_error );
     return data;
 }
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5035

[PATCH v3 3/3] win32u: Use RtlRestoreLastWin32Error instead of RtlSetLastWin32Error if appropriate

[Alfred Agrell]

From: Alfred Agrell <floating(a)muncher.se>

---
 dlls/ntdll/unix/env.c   | 4 ++++
 dlls/win32u/clipboard.c | 2 +-
 dlls/win32u/spy.c       | 8 ++++----
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/dlls/ntdll/unix/env.c b/dlls/ntdll/unix/env.c
index ad9ab0dc220..65df38518c0 100644
--- a/dlls/ntdll/unix/env.c
+++ b/dlls/ntdll/unix/env.c
@@ -2467,3 +2467,7 @@ void WINAPI RtlSetLastWin32Error( DWORD err )
 #endif
     teb->LastErrorValue = err;
 }
+void WINAPI RtlRestoreLastWin32Error( DWORD err )
+{
+    RtlSetLastWin32Error(err);
+}
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

[PATCH v3 0/3] MR5035: Change SetLastError to RestoreLastError if appropriate - closed

[Alexandre Julliard (＠julliard)]

This merge request was closed by Alexandre Julliard.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5035

[Alexandre Julliard (＠julliard)]

> To me it looks that when used with __WINESRC__ this won't change anything at all.

Yes, that's not useful.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5035#note_60339