[PATCH 0/2] MR3397: Draft: ucrtbase: Do not modify rewind's stack arguments on i386.

[Victor Hermann Chiletto (＠vitorhnn)]

[A mod for Fallout: New Vegas relies on rewind not modifying the first argument passed to it.](https://github.com/jazzisparis/UIO-User-Interface-Organizer/blob/426d96391f9...) Since it only links to api-ms-win-crt-stdio I've kept the thunk usage to just ucrtbase.

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

[PATCH 1/2] ucrtbase/tests: Test some details about rewind's ABI.

[Victor Chiletto]

From: Victor Chiletto <v(a)hnn.net.br>

More specifically: a mod for Fallout: New Vegas relies on rewind not
modifying the first argument passed to it [1].

[1]: https://github.com/jazzisparis/UIO-User-Interface-Organizer/blob/426d96391f9...
---
 dlls/ucrtbase/tests/misc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c
index 069de0787b7..17b1dca4fcf 100644
--- a/dlls/ucrtbase/tests/misc.c
+++ b/dlls/ucrtbase/tests/misc.c
@@ -1591,6 +1591,31 @@ static void test_fopen_exclusive( void )
     unlink(path);
 }
 
+#if defined(__i386__)
+static void test_rewind_i386_abi(void)
+{
+    FILE *fp = fopen("rewind_abi.tst", "wb"), *fp2;
+
+    rewind(fp);
+
+#if defined(__GNUC__)
+    __asm__ volatile ("push %2; call %P1; movl (%%esp), %0; addl $4, %%esp;" : "=r" (fp2) : "i" (rewind), "r" (fp));
+#else
+    __asm
+    {
+        push fp;
+        call rewind;
+        mov fp2, [esp];
+        add esp, 4;
+    }
+#endif
+
+    todo_wine ok(fp == fp2, "rewind modified the first argument in the stack\n");
+
+    fclose(fp);
+}
+#endif
+
 START_TEST(misc)
 {
     int arg_c;
@@ -1633,4 +1658,7 @@ START_TEST(misc)
     test_thread_storage();
     test_fenv();
     test_fopen_exclusive();
+#if defined(__i386__)
+    test_rewind_i386_abi();
+#endif
 }
-- 
GitLab


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

[PATCH 2/2] msvcrt, ucrtbase: Introduce wine_i386_rewind.

[Victor Chiletto]

From: Victor Chiletto <v(a)hnn.net.br>

This is a small thunk to rewind that preserves the first argument
passed. This is needed because on some versions of GCC, rewind's call to
_unlock_file is tail-call optimized, which modifies the stack.
---
 dlls/msvcrt/file.c          | 13 +++++++++++++
 dlls/ucrtbase/tests/misc.c  |  2 +-
 dlls/ucrtbase/ucrtbase.spec |  3 ++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c
index e72784eef41..14018e7cda9 100644
--- a/dlls/msvcrt/file.c
+++ b/dlls/msvcrt/file.c
@@ -45,6 +45,10 @@
 
 #include "wine/debug.h"
 
+#if defined (__i386__)
+#include "wine/asm.h"
+#endif
+
 WINE_DEFAULT_DEBUG_CHANNEL(msvcrt);
 
 #undef _fstat
@@ -1658,6 +1662,15 @@ int CDECL clearerr_s(FILE* file)
   return 0;
 }
 
+#if defined(__i386__)
+/* INTERNAL: stack preserving thunk for rewind */
+__ASM_GLOBAL_FUNC(wine_i386_rewind,
+                  "pushl 4(%esp)\n\t"
+                  "call "__ASM_NAME("rewind") "\n\t"
+                  "popl %eax\n\t"
+                  "ret")
+#endif
+
 /*********************************************************************
  *		rewind (MSVCRT.@)
  */
diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c
index 17b1dca4fcf..67ba567472b 100644
--- a/dlls/ucrtbase/tests/misc.c
+++ b/dlls/ucrtbase/tests/misc.c
@@ -1610,7 +1610,7 @@ static void test_rewind_i386_abi(void)
     }
 #endif
 
-    todo_wine ok(fp == fp2, "rewind modified the first argument in the stack\n");
+    ok(fp == fp2, "rewind modified the first argument in the stack\n");
 
     fclose(fp);
 }
diff --git a/dlls/ucrtbase/ucrtbase.spec b/dlls/ucrtbase/ucrtbase.spec
index cad6cf1c381..088fafa8494 100644
--- a/dlls/ucrtbase/ucrtbase.spec
+++ b/dlls/ucrtbase/ucrtbase.spec
@@ -2470,7 +2470,8 @@
 @ cdecl remquof(float float ptr)
 @ cdecl remquol(double double ptr) remquo
 @ cdecl rename(str str)
-@ cdecl rewind(ptr)
+@ cdecl -arch=i386 rewind(ptr) wine_i386_rewind
+@ cdecl -arch=!i386 rewind(ptr) rewind
 @ cdecl rint(double) MSVCRT_rint
 @ cdecl rintf(float)
 @ cdecl rintl(double) MSVCRT_rint
-- 
GitLab

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

[PATCH 2/2] msvcrt, ucrtbase: Introduce wine_i386_rewind.

[Marvin]

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=135169

Your paranoid android.


=== debian11b (64 bit WoW report) ===

win32u.cwindows.devices.bluetooth:
win32u.c:1057: Test failed: buf = d
win32u.c:1060: Test failed: res = 0
win32u.c:1070: Test failed: res = 0

Report validation errors:
win32u.cwindows.devices.bluetooth:bluetooth has no done line (or it is garbled)