[PATCH v4 0/1] MR364: kernel32/tests: Test module refcounting with forwarded exports.
Test module references created by resolving forwarded exports. The following diagram explains what happens in bug #52094 under the hood: ```mermaid graph TB subgraph state_00["Initial"] direction LR s00_ida[ida64.dll] ~~~ s00_idapy[idapython64.dll] ~~~ s00_py3[python3.dll] -. "(forwards to)" .-> s00_py311[python311.dll] end state_00 -. "1. <strong><code><span style='color:#28f'>Load</span>Library("</code></strong>configured Python version (e.g., python311.dll)<strong><code>")</code></strong>" .-> state_01 subgraph state_01["python311.dll loaded"] direction LR s01_ida[ida64.dll] ~~~ s01_idapy[idapython64.dll] ~~~ s01_py3[python3.dll] -. "(forwards to)" .-> s01_py311[python311.dll] s01_ida -- "<em>dynamic</em> ref" --> s01_py311 end state_01 -. "2. <strong><code><span style='color:#28f'>Load</span>Library("</code></strong>idapython64.dll<strong><code>")</code></strong>" .-> state_02 subgraph state_02["idapython64.dll loaded"] direction LR s02_ida[ida64.dll] -- "<em>dynamic</em> ref" --> s02_idapy[idapython64.dll] -- "<em><strong>static</strong></em> ref" --> s02_py3[python3.dll] -. "(forwards to)" .-> s02_py311[python311.dll] s02_ida -- "<em>dynamic</em> ref" --> s02_py311 s02_idapy -- "<span style='color:red'><em><strong>static forwarded</strong></em> ref</span>" --> s02_py311 end state_02 -. "3. <strong><code><span style='color:#d70'>Free</span>Library("</code></strong>configured Python version (e.g., python311.dll)<strong><code>")</code></strong>" .-> state_03 subgraph state_03["python311.dll reference removed"] direction LR style s03_py311 color:#f00,stroke:#f00 s03_ida[ida64.dll] -- "<em>dynamic</em> ref" --> s03_idapy[idapython64.dll] -- "<em><strong>static</strong></em> ref" --> s03_py3[python3.dll] -. "(forwards to)" .-> s03_py311[python311.dll] s03_ida -. "<del><em>dynamic</em> ref (deleted)</del>" .-> s03_py311 s03_idapy -- "<span style='color:red'><em><strong>static forwarded</strong></em> ref</span>" --> s03_py311 end ``` On Windows, the <em><strong>static forwarded</strong></em> ref keeps `python3.dll` from being unloaded; however, Wine is missing this behavior, causing it to be unloaded and cause segfault immediately (since further initialization happens just after FreeLibrary). The tests aim to reproduce this condition. -- v4: kernel32/tests: Test module refcounting with forwarded exports. https://gitlab.winehq.org/wine/wine/-/merge_requests/364
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/icmp/Makefile.in | 1 + dlls/kernel32/tests/Makefile.in | 14 ++- dlls/kernel32/tests/forward1.c | 43 +++++++ dlls/kernel32/tests/forward1.spec | 2 + dlls/kernel32/tests/forward2.c | 33 ++++++ dlls/kernel32/tests/forward2.spec | 2 + dlls/kernel32/tests/forward3.c | 33 ++++++ dlls/kernel32/tests/forward3.spec | 2 + dlls/kernel32/tests/loader.c | 184 ++++++++++++++++++++++++++++++ dlls/kernel32/tests/sforward.c | 42 +++++++ dlls/kernel32/tests/sforward.spec | 1 + 11 files changed, 355 insertions(+), 2 deletions(-) create mode 100644 dlls/kernel32/tests/forward1.c create mode 100644 dlls/kernel32/tests/forward1.spec create mode 100644 dlls/kernel32/tests/forward2.c create mode 100644 dlls/kernel32/tests/forward2.spec create mode 100644 dlls/kernel32/tests/forward3.c create mode 100644 dlls/kernel32/tests/forward3.spec create mode 100644 dlls/kernel32/tests/sforward.c create mode 100644 dlls/kernel32/tests/sforward.spec diff --git a/dlls/icmp/Makefile.in b/dlls/icmp/Makefile.in index 42d30b63d8d..1eb9f8a4662 100644 --- a/dlls/icmp/Makefile.in +++ b/dlls/icmp/Makefile.in @@ -1,4 +1,5 @@ MODULE = icmp.dll +IMPORTLIB = icmp EXTRADLLFLAGS = -Wb,--data-only diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in index e9516603ce9..67775e78a83 100644 --- a/dlls/kernel32/tests/Makefile.in +++ b/dlls/kernel32/tests/Makefile.in @@ -1,5 +1,7 @@ TESTDLL = kernel32.dll -IMPORTS = user32 advapi32 + +# icmp is for testing export forwarding (to iphlpapi) +IMPORTS = user32 advapi32 icmp SOURCES = \ actctx.c \ @@ -37,4 +39,12 @@ SOURCES = \ toolhelp.c \ version.c \ virtual.c \ - volume.c + volume.c \ + forward1.c \ + forward1.spec \ + forward2.c \ + forward2.spec \ + forward3.c \ + forward3.spec \ + sforward.c \ + sforward.spec diff --git a/dlls/kernel32/tests/forward1.c b/dlls/kernel32/tests/forward1.c new file mode 100644 index 00000000000..2dbdc4fb978 --- /dev/null +++ b/dlls/kernel32/tests/forward1.c @@ -0,0 +1,43 @@ +/* + * Dummy forwarder test DLL + * + * Copyright 2024 Jinoh Kang + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#define WIN32_LEAN_AND_MEAN +#include <windows.h> + +BOOL WINAPI DllMain(HINSTANCE instance_new, DWORD reason, LPVOID reserved) +{ + if (reason == DLL_PROCESS_ATTACH) + DisableThreadLibraryCalls( instance_new ); + return TRUE; +} + +unsigned long forward_test_func(void) +{ + return 0x00005678UL; +} + +unsigned long forward_test_func2(void) +{ + return 0x12340000UL; +} diff --git a/dlls/kernel32/tests/forward1.spec b/dlls/kernel32/tests/forward1.spec new file mode 100644 index 00000000000..bf19fa7e011 --- /dev/null +++ b/dlls/kernel32/tests/forward1.spec @@ -0,0 +1,2 @@ +1 cdecl forward_test_func() +2 cdecl -noname forward_test_func2() diff --git a/dlls/kernel32/tests/forward2.c b/dlls/kernel32/tests/forward2.c new file mode 100644 index 00000000000..a7b53ee553c --- /dev/null +++ b/dlls/kernel32/tests/forward2.c @@ -0,0 +1,33 @@ +/* + * Dummy forwarder test DLL + * + * Copyright 2024 Jinoh Kang + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#define WIN32_LEAN_AND_MEAN +#include <windows.h> + +BOOL WINAPI DllMain(HINSTANCE instance_new, DWORD reason, LPVOID reserved) +{ + if (reason == DLL_PROCESS_ATTACH) + DisableThreadLibraryCalls( instance_new ); + return TRUE; +} diff --git a/dlls/kernel32/tests/forward2.spec b/dlls/kernel32/tests/forward2.spec new file mode 100644 index 00000000000..374156d8d06 --- /dev/null +++ b/dlls/kernel32/tests/forward2.spec @@ -0,0 +1,2 @@ +1 cdecl forward_test_func() forward1.forward_test_func +2 cdecl -noname forward_test_func2() forward1.#2 diff --git a/dlls/kernel32/tests/forward3.c b/dlls/kernel32/tests/forward3.c new file mode 100644 index 00000000000..a7b53ee553c --- /dev/null +++ b/dlls/kernel32/tests/forward3.c @@ -0,0 +1,33 @@ +/* + * Dummy forwarder test DLL + * + * Copyright 2024 Jinoh Kang + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#define WIN32_LEAN_AND_MEAN +#include <windows.h> + +BOOL WINAPI DllMain(HINSTANCE instance_new, DWORD reason, LPVOID reserved) +{ + if (reason == DLL_PROCESS_ATTACH) + DisableThreadLibraryCalls( instance_new ); + return TRUE; +} diff --git a/dlls/kernel32/tests/forward3.spec b/dlls/kernel32/tests/forward3.spec new file mode 100644 index 00000000000..31d019aa071 --- /dev/null +++ b/dlls/kernel32/tests/forward3.spec @@ -0,0 +1,2 @@ +1 cdecl forward_test_func() forward2.forward_test_func +2 cdecl -noname forward_test_func2() forward2.#2 diff --git a/dlls/kernel32/tests/loader.c b/dlls/kernel32/tests/loader.c index 8f418ef09a9..0e81baa9e4d 100644 --- a/dlls/kernel32/tests/loader.c +++ b/dlls/kernel32/tests/loader.c @@ -1613,6 +1613,187 @@ static void test_ImportDescriptors(void) } } +static void extract_resource(const char *name, const char *type, const char *path) +{ + HMODULE module; + DWORD written; + HANDLE file; + HRSRC res; + void *ptr; + + file = CreateFileA(path, GENERIC_READ|GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0); + ok( file != INVALID_HANDLE_VALUE, "file creation failed, at %s, error %lu\n", path, GetLastError() ); + + module = GetModuleHandleA( NULL ); + res = FindResourceA( module, name, type ); + ok( res != 0, "couldn't find resource\n" ); + ptr = LockResource( LoadResource( module, res )); + WriteFile( file, ptr, SizeofResource( module, res ), &written, NULL ); + ok( written == SizeofResource( module, res ), "couldn't write resource\n" ); + CloseHandle( file ); +} + +static void test_static_forwarded_import_refs(void) +{ + CHAR temp_path[MAX_PATH], dir_path[MAX_PATH], sforward_path[MAX_PATH]; + HMODULE iphlpapi, icmp, sforward; + FARPROC test_func_stub; + + if (GetModuleHandleA( "iphlpapi.dll" )) + { + win_skip( "iphlpapi.dll already loaded, skipping\n" ); + return; + } + + ok( !GetModuleHandleA( "icmp.dll" ), "icmp.dll shall not have already been loaded\n" ); + + GetTempPathA( ARRAY_SIZE(temp_path), temp_path ); + GetTempFileNameA( temp_path, "ldr", GetTickCount() | 1UL, dir_path ); + ok( CreateDirectoryA( dir_path, NULL ), "failed to create dir %s, error %lu\n", + dir_path, GetLastError() ); + + snprintf( sforward_path, MAX_PATH, "%s\\sforward.dll", dir_path ); + extract_resource( "sforward.dll", "TESTDLL", sforward_path ); + + iphlpapi = LoadLibraryA( "iphlpapi.dll" ); + ok( !!iphlpapi, "couldn't find iphlpapi.dll: %lu\n", GetLastError() ); + icmp = LoadLibraryA( "icmp.dll" ); + ok( !!icmp, "couldn't find icmp.dll: %lu\n", GetLastError() ); + sforward = LoadLibraryA( sforward_path ); + ok( !!sforward, "couldn't find %s: %lu\n", sforward_path, GetLastError() ); + + test_func_stub = GetProcAddress( sforward, "test_func_stub" ); + ok( !!test_func_stub, "sforward!test_func_stub not found\n" ); + + /* When a DLL imports a forwarded export, the loader introduces a module + * dependency from the importer DLL to the target DLL of the forwarded + * export. This keeps iphlpapi and icmp from being unloaded. + */ + FreeLibrary( iphlpapi ); + FreeLibrary( icmp ); + todo_wine + ok( !!GetModuleHandleA( "iphlpapi.dll" ), "iphlpapi.dll unexpectedly unloaded\n" ); + ok( !!GetModuleHandleA( "icmp.dll" ), "icmp.dll unexpectedly unloaded\n" ); + + FreeLibrary( sforward ); + ok( !GetModuleHandleA( "iphlpapi.dll" ), "iphlpapi.dll unexpectedly kept open\n" ); + ok( !GetModuleHandleA( "icmp.dll" ), "icmp.dll unexpectedly kept open\n" ); + ok( !GetModuleHandleA( "sforward.dll" ), "sforward.dll unexpectedly kept open\n" ); + + DeleteFileA( sforward_path ); + RemoveDirectoryA( dir_path ); +} + +static void test_dynamic_forwarded_import_refs(void) +{ + CHAR temp_path[MAX_PATH], dir_path[MAX_PATH]; + CHAR forward1_path[MAX_PATH]; + CHAR forward2_path[MAX_PATH]; + CHAR forward3_path[MAX_PATH]; + HMODULE forward1, forward2, forward3; + FARPROC proc1, proc2, proc3, oproc1, oproc2, oproc3; + + GetTempPathA( ARRAY_SIZE(temp_path), temp_path ); + GetTempFileNameA( temp_path, "ldr", GetTickCount() | 1UL, dir_path ); + ok( CreateDirectoryA( dir_path, NULL ), "failed to create dir %s, error %lu\n", + dir_path, GetLastError() ); + + snprintf( forward1_path, MAX_PATH, "%s\\forward1.dll", dir_path ); + snprintf( forward2_path, MAX_PATH, "%s\\forward2.dll", dir_path ); + snprintf( forward3_path, MAX_PATH, "%s\\forward3.dll", dir_path ); + extract_resource( "forward1.dll", "TESTDLL", forward1_path ); + extract_resource( "forward2.dll", "TESTDLL", forward2_path ); + extract_resource( "forward3.dll", "TESTDLL", forward3_path ); + + forward1 = LoadLibraryA( forward1_path ); + ok( !!forward1, "couldn't find %s: %lu\n", forward1_path, GetLastError() ); + forward2 = LoadLibraryA( forward2_path ); + ok( !!forward2, "couldn't find %s: %lu\n", forward2_path, GetLastError() ); + forward3 = LoadLibraryA( forward3_path ); + ok( !!forward3, "couldn't find %s: %lu\n", forward3_path, GetLastError() ); + + proc1 = GetProcAddress(forward1, "forward_test_func"); + ok( !!proc1, "cannot resolve forward1!forward_test_func\n"); + proc2 = GetProcAddress(forward2, "forward_test_func"); + ok( !!proc2, "cannot resolve forward2!forward_test_func\n"); + proc3 = GetProcAddress(forward3, "forward_test_func"); + ok( !!proc3, "cannot resolve forward3!forward_test_func\n"); + ok( proc1 == proc3, "forward1!forward_test_func is not equal to forward3!forward_test_func\n"); + ok( proc2 == proc3, "forward2!forward_test_func is not equal to forward3!forward_test_func\n"); + + oproc1 = GetProcAddress(forward1, (LPSTR)2); + ok( !!oproc1, "cannot resolve forward1!#2 (forward_test_func2)\n"); + oproc2 = GetProcAddress(forward2, (LPSTR)2); + ok( !!oproc2, "cannot resolve forward2!#2 (forward_test_func2)\n"); + oproc3 = GetProcAddress(forward3, (LPSTR)2); + ok( !!oproc3, "cannot resolve forward3!#2 (forward_test_func2)\n"); + ok( oproc1 == oproc3, "forward1!forward_test_func2 is not equal to forward3!forward_test_func2\n"); + ok( oproc2 == oproc3, "forward2!forward_test_func2 is not equal to forward3!forward_test_func2\n"); + + /* GetProcAddress, when called on a forwarded export, has a side effect of + * introducing a module dependency from the source forwarder DLL to the + * target DLL. This keeps forward1 and forward2 from being unloaded. + */ + FreeLibrary( forward1 ); + FreeLibrary( forward2 ); + todo_wine + ok( !!GetModuleHandleA( "forward1.dll" ), "forward1.dll unexpectedly unloaded\n" ); + todo_wine + ok( !!GetModuleHandleA( "forward2.dll" ), "forward2.dll unexpectedly unloaded\n" ); + + FreeLibrary( forward3 ); + ok( !GetModuleHandleA( "forward1.dll" ), "forward1.dll unexpectedly kept open\n" ); + ok( !GetModuleHandleA( "forward2.dll" ), "forward2.dll unexpectedly kept open\n" ); + ok( !GetModuleHandleA( "forward3.dll" ), "forward3.dll unexpectedly kept open\n" ); + + DeleteFileA( forward1_path ); + DeleteFileA( forward2_path ); + DeleteFileA( forward3_path ); + RemoveDirectoryA( dir_path ); +} + +static void test_dynamic_forward_export_norefs(void) +{ + CHAR temp_path[MAX_PATH], dir_path[MAX_PATH]; + CHAR forward1_path[MAX_PATH]; + CHAR forward2_path[MAX_PATH]; + CHAR forward3_path[MAX_PATH]; + HMODULE forward1, forward2, forward3; + + GetTempPathA( ARRAY_SIZE(temp_path), temp_path ); + GetTempFileNameA( temp_path, "ldr", GetTickCount() | 1UL, dir_path ); + ok( CreateDirectoryA( dir_path, NULL ), "failed to create dir %s, error %lu\n", + dir_path, GetLastError() ); + + snprintf( forward1_path, MAX_PATH, "%s\\forward1.dll", dir_path ); + snprintf( forward2_path, MAX_PATH, "%s\\forward2.dll", dir_path ); + snprintf( forward3_path, MAX_PATH, "%s\\forward3.dll", dir_path ); + extract_resource( "forward1.dll", "TESTDLL", forward1_path ); + extract_resource( "forward2.dll", "TESTDLL", forward2_path ); + extract_resource( "forward3.dll", "TESTDLL", forward3_path ); + + forward1 = LoadLibraryA( forward1_path ); + ok( !!forward1, "couldn't find %s: %lu\n", forward1_path, GetLastError() ); + forward2 = LoadLibraryA( forward2_path ); + ok( !!forward2, "couldn't find %s: %lu\n", forward2_path, GetLastError() ); + forward3 = LoadLibraryA( forward3_path ); + ok( !!forward3, "couldn't find %s: %lu\n", forward3_path, GetLastError() ); + + /* The mere existence of a forwarded export shall not count as a reference by itself. */ + FreeLibrary( forward1 ); + FreeLibrary( forward3 ); + ok( !GetModuleHandleA( "forward1.dll" ), "forward1.dll unexpectedly kept open\n" ); + ok( !GetModuleHandleA( "forward3.dll" ), "forward3.dll unexpectedly kept open\n" ); + + FreeLibrary( forward2 ); + ok( !GetModuleHandleA( "forward2.dll" ), "forward2.dll unexpectedly kept open\n" ); + + DeleteFileA( forward1_path ); + DeleteFileA( forward2_path ); + DeleteFileA( forward3_path ); + RemoveDirectoryA( dir_path ); +} + static void test_image_mapping(const char *dll_name, DWORD scn_page_access, BOOL is_dll) { HANDLE hfile, hmap; @@ -4277,9 +4458,12 @@ START_TEST(loader) ok(len && len < ARRAY_SIZE(syswow_dir), "Couldn't get wow directory: %lu\n", GetLastError()); } + test_static_forwarded_import_refs(); /* Must be first; other tests may load iphlpapi.dll */ test_filenames(); test_ResolveDelayLoadedAPI(); test_ImportDescriptors(); + test_dynamic_forwarded_import_refs(); + test_dynamic_forward_export_norefs(); test_section_access(); test_import_resolution(); test_ExitProcess(); diff --git a/dlls/kernel32/tests/sforward.c b/dlls/kernel32/tests/sforward.c new file mode 100644 index 00000000000..b8d09931cb2 --- /dev/null +++ b/dlls/kernel32/tests/sforward.c @@ -0,0 +1,42 @@ +/* + * Dummy forwarder test DLL + * + * Copyright 2024 Jinoh Kang + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#define WIN32_LEAN_AND_MEAN +#include <windows.h> +#include <ws2tcpip.h> +#include <iphlpapi.h> +#include <icmpapi.h> + +void test_func_stub(void) +{ + HANDLE file = IcmpCreateFile(); + if (file != INVALID_HANDLE_VALUE) IcmpCloseHandle( file ); +} + +BOOL WINAPI DllMain(HINSTANCE instance_new, DWORD reason, LPVOID reserved) +{ + if (reason == DLL_PROCESS_ATTACH) + DisableThreadLibraryCalls( instance_new ); + return TRUE; +} diff --git a/dlls/kernel32/tests/sforward.spec b/dlls/kernel32/tests/sforward.spec new file mode 100644 index 00000000000..cb6d4add796 --- /dev/null +++ b/dlls/kernel32/tests/sforward.spec @@ -0,0 +1 @@ +@ cdecl test_func_stub() -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/364
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=143187 Your paranoid android. === w10pro64_en_AE_u8 (32 bit report) === kernel32: heap.c:3314: Test failed: HeapValidate failed heap.c:3323: Test failed: HeapValidate failed === w10pro64 (32 bit report) === kernel32: loader.c:3150: Test failed: attached thread count should be 2 loader.c:3361: Test failed: expected thread exit code 196, got 259 loader.c:3361: Test failed: expected thread exit code 196, got 259 loader.c:2854: Test failed: expected thread exit code 196, got 195 loader.c:2854: Test failed: expected thread exit code 196, got 195 === w1064_adm (64 bit report) === kernel32: loader.c:713: Test failed: 1415: wrong status c000007b/0 loader.c:718: Test failed: 1415: failed with c000007b expected fallback loader.c:713: Test failed: 1421: wrong status c000007b/0 loader.c:718: Test failed: 1421: failed with c000007b expected fallback
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/forward1.c:
+ * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#define WIN32_LEAN_AND_MEAN +#include <windows.h> I think we often use this set of minimal headers:
```suggestion:-1+0 #include <stdarg.h> #include "windef.h" #include "winbase.h" ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61838
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/forward1.c:
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#if 0 +#pragma makedep testdll +#endif + +#define WIN32_LEAN_AND_MEAN +#include <windows.h> + +BOOL WINAPI DllMain(HINSTANCE instance_new, DWORD reason, LPVOID reserved) +{ + if (reason == DLL_PROCESS_ATTACH) + DisableThreadLibraryCalls( instance_new ); + return TRUE; +} Style is inconsistent, same in various places in the other files. With `space-in-paren` style, only case where function declaration takes no space is `(void)`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61839
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/Makefile.in:
TESTDLL = kernel32.dll -IMPORTS = user32 advapi32 + +# icmp is for testing export forwarding (to iphlpapi) +IMPORTS = user32 advapi32 icmp Removing this doesn't seem to make any difference to the test on Windows? Is this even necessary?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61840
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/loader.c:
+ WriteFile( file, ptr, SizeofResource( module, res ), &written, NULL ); + ok( written == SizeofResource( module, res ), "couldn't write resource\n" ); + CloseHandle( file ); +} + +static void test_static_forwarded_import_refs(void) +{ + CHAR temp_path[MAX_PATH], dir_path[MAX_PATH], sforward_path[MAX_PATH]; + HMODULE iphlpapi, icmp, sforward; + FARPROC test_func_stub; + + if (GetModuleHandleA( "iphlpapi.dll" )) + { + win_skip( "iphlpapi.dll already loaded, skipping\n" ); + return; + } Imo it'd be better to avoid the skip unless there's actually cases where iphlpapi is already loaded. Otherwise we might end up breaking the test without noticing it (even though there's a comment, who trusts them anyway?).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61841
On Mon Feb 19 07:23:20 2024 +0000, Rémi Bernon wrote:
Imo it'd be better to avoid the skip unless there's actually cases where iphlpapi is already loaded. Otherwise we might end up breaking the test without noticing it (even though there's a comment, who trusts them anyway?). Would `ok( !!GetModuleHandleA( "iphlpapi.dll" ), "iphlpapi.dll shouldn't be already loaded\n" );` work?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61856
On Mon Feb 19 10:57:32 2024 +0000, Jinoh Kang wrote:
Would `ok( !!GetModuleHandleA( "iphlpapi.dll" ), "iphlpapi.dll shouldn't be already loaded\n" );` work? One less ! but yes I think it'd be better.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61860
On Mon Feb 19 07:23:16 2024 +0000, Rémi Bernon wrote:
Removing this doesn't seem to make any difference to the test on Windows? Is this even necessary? Removing this makes the build fail. Care to elaborate?
<https://testbot.winehq.org/JobDetails.pl?Key=143222>: ``` /home/winetest/tools/testbot/var/wine-exe32/../wine/dlls/kernel32/tests/sforward.c:33: undefined reference to `IcmpCreateFile(a)0' /usr/bin/i686-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-exe32/../wine/dlls/kernel32/tests/sforward.c:34: undefined reference to `IcmpCloseHandle(a)4' collect2: error: ld returned 1 exit status Task: The exe32 Wine build failed ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61861
On Mon Feb 19 11:02:30 2024 +0000, Jinoh Kang wrote:
Removing this makes the build fail. Care to elaborate? <https://testbot.winehq.org/JobDetails.pl?Key=143222>: ``` /home/winetest/tools/testbot/var/wine-exe32/../wine/dlls/kernel32/tests/sforward.c:33: undefined reference to `IcmpCreateFile(a)0' /usr/bin/i686-w64-mingw32-ld: /home/winetest/tools/testbot/var/wine-exe32/../wine/dlls/kernel32/tests/sforward.c:34: undefined reference to `IcmpCloseHandle(a)4' collect2: error: ld returned 1 exit status Task: The exe32 Wine build failed ``` Oh hmm, probably something with test DLL dependencies. It didn't consider they needed to be rebuilt so I missed the error. Anyway, if that's only for sforward.dll, I think you can use `sforward_IMPORTS = icmp` instead.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/364#note_61862
participants (4)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin -
Rémi Bernon