May 2020 - Wine-Devel - list.winehq.org

Re: [PATCH 6/6] ntoskrnl.exe/tests: Add a test for accessing process memory.

by Marvin

Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=72509 Your paranoid android. === w8 (testbot log) === An error occurred while waiting for the test to complete: the 3128 process does not exist or is not a child process

May 31, 2020

Re: [PATCH 1/6] ntoskrnl.exe: Implement PsGetProcessSectionBaseAddress() function.

by Jacek Caban

Hi Paul, On 31.05.2020 16:41, Paul Gofman wrote: > + if (!(h = OpenProcess( PROCESS_ALL_ACCESS, FALSE, process->info.UniqueProcessId))) > + { > + WARN("Could not open process %#04lx.\n", process->info.UniqueProcessId); > + return NULL; > + } This should work, but ObOpenObjectByPointer() is generally used when we need a handle from a kernel object pointer. Thanks, Jacek

May 31, 2020

[PATCH 6/6] ntoskrnl.exe/tests: Add a test for accessing process memory.

by Paul Gofman

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver.c | 70 ++++++++++++++++++++++++++++++ dlls/ntoskrnl.exe/tests/driver.h | 3 ++ dlls/ntoskrnl.exe/tests/ntoskrnl.c | 10 ++++- 3 files changed, 82 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index ba2b4d6ac8e..6e62990deec 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -2067,6 +2067,75 @@ static void test_dpc(void) KeRevertToUserAffinityThread(); } +static void test_process_memory(const struct test_input *test_input) +{ + NTSTATUS (WINAPI *pMmCopyVirtualMemory)(PEPROCESS fromprocess, void *fromaddress, PEPROCESS toprocess, + void *toaddress, SIZE_T bufsize, KPROCESSOR_MODE mode, SIZE_T *copied); + char buffer[sizeof(teststr)]; + ULONG64 modified_value; + PEPROCESS process; + KAPC_STATE state; + NTSTATUS status; + SIZE_T size; + BYTE *base; + + pMmCopyVirtualMemory = get_proc_address("MmCopyVirtualMemory"); + + status = PsLookupProcessByProcessId((HANDLE)(ULONG_PTR)test_input->process_id, &process); + ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status); + + if (status) + return; + +#if 0 + /* Crashes on Windows. */ + PsGetProcessSectionBaseAddress(NULL); +#endif + + base = PsGetProcessSectionBaseAddress(process); + ok(!!base, "Got NULL base address.\n"); + + ok(process == PsGetCurrentProcess(), "Got unexpected process %p, PsGetCurrentProcess() %p.\n", + process, PsGetCurrentProcess()); + + modified_value = 0xdeadbeeffeedcafe; + if (pMmCopyVirtualMemory) + { + size = 0xdeadbeef; + status = pMmCopyVirtualMemory(process, base + test_input->teststr_offset, PsGetCurrentProcess(), + buffer, sizeof(buffer), UserMode, &size); + todo_wine ok(status == STATUS_ACCESS_VIOLATION, "Got unexpected status %#x.\n", status); + ok(!size, "Got unexpected size %#lx.\n", size); + + memset(buffer, 0, sizeof(buffer)); + size = 0xdeadbeef; +#if 0 + /* Passing NULL for the copied size address hangs Windows. */ + pMmCopyVirtualMemory(process, base + test_input->teststr_offset, PsGetCurrentProcess(), + buffer, sizeof(buffer), KernelMode, NULL); +#endif + status = pMmCopyVirtualMemory(process, base + test_input->teststr_offset, PsGetCurrentProcess(), + buffer, sizeof(buffer), KernelMode, &size); + todo_wine ok(status == STATUS_SUCCESS, "Got unexpected status %#x.\n", status); + todo_wine ok(size == sizeof(buffer), "Got unexpected size %lu.\n", size); + todo_wine ok(!strcmp(buffer, teststr), "Got unexpected test string.\n"); + } + else + { + win_skip("MmCopyVirtualMemory is not available.\n"); + } + + if (!test_input->running_under_wine) + { + KeStackAttachProcess((PKPROCESS)process, &state); + todo_wine ok(!strcmp(teststr, (char *)(base + test_input->teststr_offset)), + "Strings do not match.\n"); + *test_input->modified_value = modified_value; + KeUnstackDetachProcess(&state); + } + ObDereferenceObject(process); +} + static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *stack) { ULONG length = stack->Parameters.DeviceIoControl.OutputBufferLength; @@ -2122,6 +2191,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st #endif test_affinity(); test_dpc(); + test_process_memory(test_input); if (main_test_work_item) return STATUS_UNEXPECTED_IO_ERROR; diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index aa9de674e2f..6012efc41c8 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -42,6 +42,9 @@ struct test_input int running_under_wine; int winetest_report_success; int winetest_debug; + DWORD process_id; + SIZE_T teststr_offset; + ULONG64 *modified_value; WCHAR path[1]; }; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 2535ed903e7..8d2da92455a 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -148,8 +148,9 @@ static void main_test(void) static const WCHAR dokW[] = {'d','o','k',0}; WCHAR temppathW[MAX_PATH], pathW[MAX_PATH]; struct test_input *test_input; - UNICODE_STRING pathU; DWORD len, written, read; + ULONG64 modified_value; + UNICODE_STRING pathU; LONG new_failures; char buffer[512]; HANDLE okfile; @@ -165,6 +166,11 @@ static void main_test(void) test_input->running_under_wine = !strcmp(winetest_platform, "wine"); test_input->winetest_report_success = winetest_report_success; test_input->winetest_debug = winetest_debug; + test_input->process_id = GetCurrentProcessId(); + test_input->teststr_offset = (SIZE_T)((BYTE *)&teststr - (BYTE *)NtCurrentTeb()->Peb->ImageBaseAddress); + test_input->modified_value = &modified_value; + modified_value = 0; + memcpy(test_input->path, pathU.Buffer, len); res = DeviceIoControl(device, IOCTL_WINETEST_MAIN_TEST, test_input, offsetof( struct test_input, path[len / sizeof(WCHAR)]), @@ -172,6 +178,8 @@ static void main_test(void) ok(res, "DeviceIoControl failed: %u\n", GetLastError()); ok(written == sizeof(new_failures), "got size %x\n", written); + todo_wine ok(modified_value == 0xdeadbeeffeedcafe, "Got unexpected value %#I64x.\n", modified_value); + okfile = CreateFileW(pathW, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL); ok(okfile != INVALID_HANDLE_VALUE, "failed to create %s: %u\n", wine_dbgstr_w(pathW), GetLastError()); -- 2.26.2

May 31, 2020

[PATCH 5/6] ntoskrnl.exe: Add stub for KeUnstackDetachProcess().

by Paul Gofman

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 5 +++++ dlls/ntoskrnl.exe/ntoskrnl.exe.spec | 2 +- include/ddk/ntifs.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index d957774ffa4..01ace06edaf 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -4286,3 +4286,8 @@ void WINAPI KeStackAttachProcess(KPROCESS *process, KAPC_STATE *apc_state) { FIXME("process %p, apc_state %p stub.\n", process, apc_state); } + +void WINAPI KeUnstackDetachProcess(KAPC_STATE *apc_state) +{ + FIXME("apc_state %p stub.\n", apc_state); +} diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec index eacb15adaab..de08306040d 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec @@ -648,7 +648,7 @@ @ stub KeSynchronizeExecution @ stub KeTerminateThread @ extern KeTickCount -@ stub KeUnstackDetachProcess +@ stdcall KeUnstackDetachProcess(ptr) @ stub KeUpdateRunTime @ stub KeUpdateSystemTime @ stub KeUserModeCallback diff --git a/include/ddk/ntifs.h b/include/ddk/ntifs.h index aa430805334..73d602595c4 100644 --- a/include/ddk/ntifs.h +++ b/include/ddk/ntifs.h @@ -133,6 +133,7 @@ BOOLEAN WINAPI FsRtlIsNameInExpression(PUNICODE_STRING, PUNICODE_STRING, BOOLEAN DEVICE_OBJECT * WINAPI IoGetAttachedDevice(DEVICE_OBJECT*); PEPROCESS WINAPI IoGetRequestorProcess(IRP*); void WINAPI KeStackAttachProcess(KPROCESS*,KAPC_STATE*); +void WINAPI KeUnstackDetachProcess(KAPC_STATE*); NTSTATUS WINAPI ObOpenObjectByPointer(void*,ULONG,PACCESS_STATE,ACCESS_MASK,POBJECT_TYPE,KPROCESSOR_MODE,HANDLE*); NTSTATUS WINAPI ObQueryNameString(PVOID,POBJECT_NAME_INFORMATION,ULONG,PULONG); BOOLEAN WINAPI PsIsSystemThread(PETHREAD); -- 2.26.2

May 31, 2020

[PATCH 4/6] ntoskrnl.exe: Add stub for KeStackAttachProcess().

by Paul Gofman

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 5 +++++ dlls/ntoskrnl.exe/ntoskrnl.exe.spec | 2 +- include/ddk/ntifs.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index 8532d18085e..d957774ffa4 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -4281,3 +4281,8 @@ void * WINAPI PsGetProcessSectionBaseAddress(PEPROCESS process) TRACE("returning %p.\n", image_base); return image_base; } + +void WINAPI KeStackAttachProcess(KPROCESS *process, KAPC_STATE *apc_state) +{ + FIXME("process %p, apc_state %p stub.\n", process, apc_state); +} diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec index 21bb4cc2584..eacb15adaab 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec @@ -644,7 +644,7 @@ @ stub KeSetTimeIncrement @ stdcall KeSetTimer(ptr int64 ptr) @ stdcall KeSetTimerEx(ptr int64 long ptr) -@ stub KeStackAttachProcess +@ stdcall KeStackAttachProcess(ptr ptr) @ stub KeSynchronizeExecution @ stub KeTerminateThread @ extern KeTickCount diff --git a/include/ddk/ntifs.h b/include/ddk/ntifs.h index 39636e80730..aa430805334 100644 --- a/include/ddk/ntifs.h +++ b/include/ddk/ntifs.h @@ -132,6 +132,7 @@ typedef struct _FS_FILTER_CALLBACKS BOOLEAN WINAPI FsRtlIsNameInExpression(PUNICODE_STRING, PUNICODE_STRING, BOOLEAN, PWCH); DEVICE_OBJECT * WINAPI IoGetAttachedDevice(DEVICE_OBJECT*); PEPROCESS WINAPI IoGetRequestorProcess(IRP*); +void WINAPI KeStackAttachProcess(KPROCESS*,KAPC_STATE*); NTSTATUS WINAPI ObOpenObjectByPointer(void*,ULONG,PACCESS_STATE,ACCESS_MASK,POBJECT_TYPE,KPROCESSOR_MODE,HANDLE*); NTSTATUS WINAPI ObQueryNameString(PVOID,POBJECT_NAME_INFORMATION,ULONG,PULONG); BOOLEAN WINAPI PsIsSystemThread(PETHREAD); -- 2.26.2

May 31, 2020

[PATCH 3/6] include/ddk: Add _KAPC_STATE structure definition.

by Paul Gofman

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- include/ddk/wdm.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h index df30051b405..85d637639f5 100644 --- a/include/ddk/wdm.h +++ b/include/ddk/wdm.h @@ -52,6 +52,7 @@ struct _KAPC; struct _IRP; struct _DEVICE_OBJECT; struct _DRIVER_OBJECT; +struct _KPROCESS; typedef VOID (WINAPI *PKDEFERRED_ROUTINE)(struct _KDPC *, PVOID, PVOID, PVOID); typedef VOID (WINAPI *PKSTART_ROUTINE)(PVOID); @@ -224,12 +225,22 @@ typedef struct _IO_TIMER_ROUTINE *PIO_TIMER_ROUTINE; typedef struct _ETHREAD *PETHREAD; typedef struct _KTHREAD *PKTHREAD, *PRKTHREAD; typedef struct _EPROCESS *PEPROCESS; +typedef struct _KPROCESS KPROCESS, *PKPROCESS, *PRKPROCESS; typedef struct _IO_WORKITEM *PIO_WORKITEM; typedef struct _OBJECT_TYPE *POBJECT_TYPE; typedef struct _OBJECT_HANDLE_INFORMATION *POBJECT_HANDLE_INFORMATION; typedef struct _ZONE_HEADER *PZONE_HEADER; typedef struct _LOOKASIDE_LIST_EX *PLOOKASIDE_LIST_EX; +typedef struct _KAPC_STATE +{ + LIST_ENTRY ApcListHead[2]; + PKPROCESS Process; + UCHAR KernelApcInProgress; + UCHAR KernelApcPending; + UCHAR UserApcPending; +} KAPC_STATE, *PKAPC_STATE; + #define FM_LOCK_BIT 0x1 typedef struct _FAST_MUTEX -- 2.26.2

May 31, 2020

[PATCH 2/6] ntoskrnl.exe: Zero copied size in MmCopyVirtualMemory() stub.

by Paul Gofman

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 11 +++++++---- include/ddk/wdm.h | 1 + 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index 8d9a3e308ce..8532d18085e 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -3985,11 +3985,14 @@ PVOID WINAPI PsGetProcessWow64Process(PEPROCESS process) /********************************************************************* * MmCopyVirtualMemory (NTOSKRNL.@) */ -NTSTATUS WINAPI MmCopyVirtualMemory(PEPROCESS fromprocess, PVOID fromaddress, PEPROCESS toprocess, - PVOID toaddress, SIZE_T bufsize, KPROCESSOR_MODE mode, - PSIZE_T copied) +NTSTATUS WINAPI MmCopyVirtualMemory(PEPROCESS fromprocess, void *fromaddress, PEPROCESS toprocess, + void *toaddress, SIZE_T bufsize, KPROCESSOR_MODE mode, + SIZE_T *copied) { - FIXME("stub: %p %p %p %p %lu %d %p\n", fromprocess, fromaddress, toprocess, toaddress, bufsize, mode, copied); + FIXME("fromprocess %p, fromaddress %p, toprocess %p, toaddress %p, bufsize %lu, mode %d, copied %p stub.\n", + fromprocess, fromaddress, toprocess, toaddress, bufsize, mode, copied); + + *copied = 0; return STATUS_NOT_IMPLEMENTED; } diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h index 57e4cf4fe53..df30051b405 100644 --- a/include/ddk/wdm.h +++ b/include/ddk/wdm.h @@ -1735,6 +1735,7 @@ PVOID WINAPI MmAllocateContiguousMemory(SIZE_T,PHYSICAL_ADDRESS); PVOID WINAPI MmAllocateNonCachedMemory(SIZE_T); PMDL WINAPI MmAllocatePagesForMdl(PHYSICAL_ADDRESS,PHYSICAL_ADDRESS,PHYSICAL_ADDRESS,SIZE_T); void WINAPI MmBuildMdlForNonPagedPool(MDL*); +NTSTATUS WINAPI MmCopyVirtualMemory(PEPROCESS,void*,PEPROCESS,void*,SIZE_T,KPROCESSOR_MODE,SIZE_T*); void WINAPI MmFreeNonCachedMemory(PVOID,SIZE_T); void * WINAPI MmGetSystemRoutineAddress(UNICODE_STRING*); PVOID WINAPI MmMapLockedPagesSpecifyCache(PMDLX,KPROCESSOR_MODE,MEMORY_CACHING_TYPE,PVOID,ULONG,MM_PAGE_PRIORITY); -- 2.26.2

May 31, 2020

[PATCH 1/6] ntoskrnl.exe: Implement PsGetProcessSectionBaseAddress() function.

by Paul Gofman

Signed-off-by: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntoskrnl.exe/ntoskrnl.c | 30 +++++++++++++++++++++++++++++ dlls/ntoskrnl.exe/ntoskrnl.exe.spec | 2 +- include/ddk/ntddk.h | 1 + 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index fbf6262b3eb..8d9a3e308ce 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -4248,3 +4248,33 @@ void WINAPI KeSignalCallDpcDone(void *barrier) { InterlockedDecrement((LONG *)barrier); } + +void * WINAPI PsGetProcessSectionBaseAddress(PEPROCESS process) +{ + void *image_base; + NTSTATUS status; + SIZE_T size; + HANDLE h; + + TRACE("process %p.\n", process); + + if (!(h = OpenProcess( PROCESS_ALL_ACCESS, FALSE, process->info.UniqueProcessId))) + { + WARN("Could not open process %#04lx.\n", process->info.UniqueProcessId); + return NULL; + } + + status = NtReadVirtualMemory(h, &process->info.PebBaseAddress->ImageBaseAddress, + &image_base, sizeof(image_base), &size); + + NtClose(h); + + if (status || size != sizeof(image_base)) + { + WARN("Error reading process memory, status %#x, size %lu.\n", status, size); + return NULL; + } + + TRACE("returning %p.\n", image_base); + return image_base; +} diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec index 2b7f57e895f..21bb4cc2584 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec @@ -904,7 +904,7 @@ @ stub PsGetProcessJob @ stub PsGetProcessPeb @ stub PsGetProcessPriorityClass -@ stub PsGetProcessSectionBaseAddress +@ stdcall PsGetProcessSectionBaseAddress(ptr) @ stub PsGetProcessSecurityPort @ stub PsGetProcessSessionId @ stub PsGetProcessWin32Process diff --git a/include/ddk/ntddk.h b/include/ddk/ntddk.h index 2b05fda7118..b9f8295db88 100644 --- a/include/ddk/ntddk.h +++ b/include/ddk/ntddk.h @@ -229,6 +229,7 @@ NTSTATUS WINAPI KeExpandKernelStackAndCallout(PEXPAND_STACK_CALLOUT,void*,SIZE_ void WINAPI KeSetTargetProcessorDpc(PRKDPC,CCHAR); BOOLEAN WINAPI MmIsAddressValid(void *); HANDLE WINAPI PsGetProcessId(PEPROCESS); +void * WINAPI PsGetProcessSectionBaseAddress(PEPROCESS); HANDLE WINAPI PsGetThreadId(PETHREAD); HANDLE WINAPI PsGetThreadProcessId(PETHREAD); NTSTATUS WINAPI PsRemoveLoadImageNotifyRoutine(PLOAD_IMAGE_NOTIFY_ROUTINE); -- 2.26.2

May 31, 2020

Re: [PATCH 1/3] makedep: Align PE sections so they can be mmapped.

by Alexandre Julliard

Jacek Caban <jacek(a)codeweavers.com> writes: > Oh, right. Being more friendly for the loader is a valid argument to > use it in Wine. > > I measured build size change by checking 32-bit prefix size (so not > exactly measuring build size, but with shared Gecko and Mono installs, > it's mostly PEs that are in prefix and I already had it scripted). GCC > build size increased from 133MB to 152MB, clang build (where I used > CROSSLDFLAGS=-Wl,-Xlink=-filealign:409) from 121MB to 139MB. This is > not great, but probably not a deal breaker. This would still need a > configure check. It should probably be handled in winegcc instead of configure. Being able to mmap files is also a good thing WRT memory usage, so I feel it would be worth doing even if the size increases a bit. -- Alexandre Julliard julliard(a)winehq.org

May 31, 2020

Re: [PATCH 1/3] makedep: Align PE sections so they can be mmapped.

by Jacek Caban

On 30.05.2020 21:45, Rémi Bernon wrote: > On 5/29/20 2:57 PM, Jacek Caban wrote: >> On 27.05.2020 13:49, Rémi Bernon wrote: >>> >>> Alright. So, "split*" means that the debug sections are extracted to >>> a separate file, and, currently, this file is also a PE file, as the >>> default objcopy operation is to use the same output format as the >>> source file. >>> >>> Having the debug section in an ELF file instead in this case >>> shouldn't be an issue right? Gdb and any other tool that were >>> already able to parse DWARF from PE files should be able to parse >>> DWARF from ELF without trouble. Using an ELF container allows perf >>> to read it. I don't think there's anything that requires PE >>> container with DWARF debug info, but maybe there is? >>> >>> Regarding the default name and location of this ELF module, I see >>> that we already implement in dbghelp the same logic as gdb does, >>> which includes looking for .debug/debugfile, where debugfile is the >>> contents of GNU debuglink, so this should work fine as well, while >>> at the same time making perf work OOTB. >>> >>> From a quick test, as long as the .debug folder is also copied >>> alongside the dll in the prefix, both winedbg and winedbg --gdb are >>> happy with the ELF container. >>> >>> So, in the end, although I understand the root issue is some >>> limitation of perf itself, this feels much simpler to fix in Wine >>> with these small tweaks that makes every tool happy, than to try >>> implementing PE support in perf. I think it would require much more >>> work there, and would be way harder to justify if it were to be >>> upstreamed (this is clearly a Wine specific use-case and I'm not >>> sure how eager perf maintainers would be to have huge code changes >>> just for the sake of PE module parsing). >> >> >> Yes, I agree that it could all work, I even made sure that >> dbghelp.dll can handle it when I was adding support for debug links >> in PE. I'm not against (at least part) of the patchset. But the thing >> is that it's quite a lot of perf-specific tweaking. Even if we take >> those patches, it would still be interesting to be better supported >> by perf, so why not at least evaluate it first. > > Sure it's perf specific, but to be honest I'm not seeing any actual > use case for this separate debug file except for perf compatibility. > I'm not talking about the PDB here, just the separate PE module with > DWARF debug infos. For me, it was indeed a side quest when working on PDB files motivated by your use case. I was hoping that we will figure out questionable parts later, which brings us to this thread. But it provides some improvements regardless of perf. For example, it significantly reduces prefix size created by an unstripped build. I was also thinking about using split debug for tests instead of linking them twice. > I understand why debug symbols are usually split, for instance by OS > distributions, to reduce the size of the packages, but it is usually > done already using build wrappers. Yeah, and if we could get it supported by perf then whatever distributions do (like using build id), they could do the same to PE files and perf would work. > It's indeed possible to do the same thing for PE files, as implemented > in the attached patches, using libbfd to parse them, but it still > feels very hacky. Then I may be pessimistic but I also expect it to be > a bit hard to justify such a change, as it is -in the other way this > time- a wine-specific change in perf. > > Also, although the basic features seems to work fine (perf annotate > relies on external tools for disassembly for instance), it's limited > to providing the symbol list and it's not going to be well integrated > with the ELF/DWARF part of the code, so it may lack features. Great, thanks for looking at it! It already supports more than we can achieve by adjusting Wine, right? I don't know if upstream would be interested in something like this, but I think that it would be worth asking them. > >> >> The above is about two other patches. I didn't look at PE alignment >> fix, but I just expect that it should be fixable upstream. With that >> in mind, my comments to patches: >> > > I'm not sure to understand, you mean fixable in perf as well? I don't > think it is easily fixable there either, perf uses mmap hooks to > detect which files are mapped to which memory range, and if the > section aren't aligned we copy them instead, which makes it loose track. Oh, right. Being more friendly for the loader is a valid argument to use it in Wine. I measured build size change by checking 32-bit prefix size (so not exactly measuring build size, but with shared Gecko and Mono installs, it's mostly PEs that are in prefix and I already had it scripted). GCC build size increased from 133MB to 152MB, clang build (where I used CROSSLDFLAGS=-Wl,-Xlink=-filealign:409) from 121MB to 139MB. This is not great, but probably not a deal breaker. This would still need a configure check. Thanks, Jacek

May 31, 2020