[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.
-- v2: msvcrt, ucrtbase: Introduce wine_i386_rewind. ucrtbase/tests: Test some details about rewind's ABI.
From: Victor Chiletto v@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 | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c index 069de0787b7..1371bc79b32 100644 --- a/dlls/ucrtbase/tests/misc.c +++ b/dlls/ucrtbase/tests/misc.c @@ -38,6 +38,10 @@ #include <winbase.h> #include "wine/test.h"
+#if defined(__i386__) && defined(__GNUC__) +#include "wine/asm.h" +#endif + #define DEFINE_EXPECT(func) \ static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
@@ -1591,6 +1595,41 @@ static void test_fopen_exclusive( void ) unlink(path); }
+#if defined(__i386__) +#if defined(__GNUC__) +extern CDECL FILE *test_rewind_wrapper(FILE *fp); +__ASM_GLOBAL_FUNC(test_rewind_wrapper, + "pushl 4(%esp)\n\t" + "call " __ASM_NAME("rewind") "\n\t" + "popl %eax\n\t" + "ret") +#else +FILE* CDECL test_rewind_wrapper(FILE* fp); +__declspec(naked) FILE* test_rewind_wrapper(FILE *fp) +{ + __asm + { + push [esp + 4]; + call rewind; + pop eax; + ret; + } +} +#endif + +static void test_rewind_i386_abi(void) +{ + FILE *fp_in, *fp_out; + + fp_in = fopen("rewind_abi.tst", "wb"); + fp_out = test_rewind_wrapper(fp_in); + todo_wine ok(fp_in == fp_out, "rewind modified the first argument in the stack\n"); + + fclose(fp_in); + unlink("rewind_abi.tst"); +} +#endif + START_TEST(misc) { int arg_c; @@ -1633,4 +1672,7 @@ START_TEST(misc) test_thread_storage(); test_fenv(); test_fopen_exclusive(); +#if defined(__i386__) + test_rewind_i386_abi(); +#endif }
From: Victor Chiletto v@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 1371bc79b32..1d978a2fb42 100644 --- a/dlls/ucrtbase/tests/misc.c +++ b/dlls/ucrtbase/tests/misc.c @@ -1623,7 +1623,7 @@ static void test_rewind_i386_abi(void)
fp_in = fopen("rewind_abi.tst", "wb"); fp_out = test_rewind_wrapper(fp_in); - todo_wine ok(fp_in == fp_out, "rewind modified the first argument in the stack\n"); + ok(fp_in == fp_out, "rewind modified the first argument in the stack\n");
fclose(fp_in); unlink("rewind_abi.tst"); 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
v2: Use basic asm for the tests.
Piotr Caban (@piotr) commented about dlls/ucrtbase/tests/misc.c:
- "call " __ASM_NAME("rewind") "\n\t"
- "popl %eax\n\t"
- "ret")
+#else +FILE* CDECL test_rewind_wrapper(FILE* fp); +__declspec(naked) FILE* test_rewind_wrapper(FILE *fp) +{
- __asm
- {
push [esp + 4];call rewind;pop eax;ret;- }
+} +#endif
I don't know if it's portable enough. How about changing it so it's done in similar way as in other tests? (see thiscall_thunk in dlls/msvcrt/tests/cpp.c for an example).
Piotr Caban (@piotr) commented about dlls/ucrtbase/tests/misc.c:
- {
push [esp + 4];call rewind;pop eax;ret;- }
+} +#endif
+static void test_rewind_i386_abi(void) +{
- FILE *fp_in, *fp_out;
- fp_in = fopen("rewind_abi.tst", "wb");
- fp_out = test_rewind_wrapper(fp_in);
- todo_wine ok(fp_in == fp_out, "rewind modified the first argument in the stack\n");
Please add the test after the fix (or in the same patch). It may fail depending on wine compiler/compilation flags.
Piotr Caban (@piotr) commented about dlls/msvcrt/file.c:
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
Please add CFI annotations and remove `INTERNAL: ` prefix from the comment. Since it's added for a broken app I think it's useful to add a short comment saying "needed for ...".
How about changing `wine_i386_rewind` function name to `rewind_preserve_stack`?
-
Piotr Caban (＠piotr) -
Victor Chiletto
-
Victor Hermann Chiletto (＠vitorhnn)