This serie is a first attempt to provide a solution to bug 57308 [1].
Basically, Arch distro is looking for Wine support in distributing debug information for Wine PE modules separated from image.
A quick overview of distro (Debian, Fedora, Arch) situation i (I may have missed a couple of items), but: - no distro ship separate debug information for PE modules (they do it for ELF ones though) - at best, they ship unstripped PE modules.
IMO, we need to help them pave the way to what should be the best expected situation (in Wine and wrt distros) to support separate debug information for PE images.
First, I believe we need to use a (solid) key to bind the stripped image and the file holding the debug information. In ELF format, this is typically done by inserting a .note.gnu.build-id section (in both stripped image and debug information file) which contains the key. This key is used to either search in fixed location (eg /usr/lib/.debug), or as key to download the debug information from a remote server (debuginfod). The key is generally made of a hash of (some sections of) the image file. This allows (esp. in the case of downloading the debug information from a debugger) to discriminate between several versions of the same module.
I think we should target this kind of solution instead of storing the debug information alongside the module (usually done as ntdll.dll.debug). This wouldn't let debuggers download the expected debug info upon request.
Fortunately, gcc and clang provide a build-id option (when generating PE images) which stores this key in the PE image.
What this serie does: - fixes & improves dbghelp in the support of the build-id information from gcc/mingw & clang, - improves Wine's configure script so that clang can be used to generate dwarf debug information and build-id information in PE builds, - adds a couple of additional search locations for debug information files,
It takes the following assumptions for the storage: - debug information files for PE images are stored in same hierarchy as debug information files for ELF images (eg /usr/lib/.debug) - naming of such files is done as ELF, ie by using the hex string generated from the binary blob of the key. This is *NOT* the usual way how the GUID will be printed (because the first 3 integers of the GUID are stored in little-endian format). (Note: I had to make a choice here. I don't have strong arguments one way or the other, but we need to agree on the convention here).
What distro should do to support separate debug info: - configure Wine with --enable-build-id - adapt at least the packaging tool to extract the key out of unstripped image [2], - if binutils isn't compiled with PE support (this is apparently the Arch case), adapt the scripts to use x86_64-objcopy instead of objcopy (and friends).
I marked this MR as draft for now, as: - I still need to polish some items (like the default location for looking up debug information files), - wait for feedback on this proposal (it does make a couple of assumptions that need sharing IMO),
Further steps: - the steps above are mainly targetted to have a solution for packaged download of debug information files, - this should be integrated in debuginfod (server side: ingestion of PE files and client side: ensure gdb, lldb are compatible; implement debuginfod in winedbg).
Feed back welcomed!
[1]: https://bugs.winehq.org/show_bug.cgi?id=57308
[2]:
The least invasive readelf replacement (assuming binutils is compiled with PE support). (otherwise, use x86_64-objdump)
BUILDID=`objdump -p "$INPUT" | sed -n "/RSDS signature / {s/.*signature //; s/ age.*//p; q;}"` BUILDID=${BUILDID:6:2}${BUILDID:4:2}${BUILDID:2:2}${BUILDID:0:2}${BUILDID:10:2}${BUILDID:8:2}${BUILDID:14:2}${BUILDID:12:2}${BUILDID:16}
Additional note: gcc/mingw puts the desired debug entry inside a dedicated section (.buildid), while clangs keeps it in .rdata (as msvc does). So, this invalidates any attempt to get information from the .buildid section (as stated in bug report).
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/dbghelp/msc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/dlls/dbghelp/msc.c b/dlls/dbghelp/msc.c index d9b37509a9b..e2e91e1abd2 100644 --- a/dlls/dbghelp/msc.c +++ b/dlls/dbghelp/msc.c @@ -4379,7 +4379,13 @@ static BOOL codeview_process_info(const struct process *pcs,
TRACE("Got RSDS type of PDB file: guid=%s age=%08x name=%s\n", wine_dbgstr_guid(&rsds->guid), rsds->age, debugstr_a(rsds->name)); - ret = pdb_process_file(pcs, msc_dbg, rsds->name, &rsds->guid, 0, rsds->age); + /* gcc/mingw and clang can emit build-id information, but with an empty PDB filename. + * Don't search for the .pdb file in that case. + */ + if (rsds->name[0]) + ret = pdb_process_file(pcs, msc_dbg, rsds->name, &rsds->guid, 0, rsds->age); + else + ret = TRUE; break; } default: @@ -4487,7 +4493,7 @@ typedef struct _FPO_DATA __ENDTRY
/* we haven't found yet any debug information, fallback to unmatched pdb */ - if (module->module.SymType == SymDeferred) + if (!ret && module->module.SymType == SymDeferred) { SYMSRV_INDEX_INFOW info = {.sizeofstruct = sizeof(info)}; char buffer[MAX_PATH];
From: Eric Pouech epouech@codeweavers.com
Fedora (at least) now put the debug info here (instead of /usr/lib/debug/.build).
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/dbghelp/module.c | 93 ++++++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 41 deletions(-)
diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index 0ade470961b..f5130010538 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -662,69 +662,80 @@ static WCHAR* append_hex(WCHAR* dst, const BYTE* id, const BYTE* end) return dst; }
-/****************************************************************** - * image_locate_build_id_target - * - * Try to find the .so file containing the debug info out of the build-id note information - */ -static struct image_file_map* image_locate_build_id_target(const BYTE* id, unsigned idlen) +static BOOL image_locate_build_id_target_in_dir(struct image_file_map *fmap_link, const BYTE* id, unsigned idlen, const WCHAR *from) { - struct image_file_map* fmap_link = NULL; - DWORD sz; - WCHAR* p; - WCHAR* z; + size_t from_len = wcslen(from); + BOOL found = FALSE; + WCHAR *p, *z;
- fmap_link = HeapAlloc(GetProcessHeap(), 0, sizeof(*fmap_link)); - if (!fmap_link) return NULL; - - p = malloc(sizeof(L"/usr/lib/debug/.build-id/") + - (idlen * 2 + 1) * sizeof(WCHAR) + sizeof(L".debug")); - if (!p) goto fail; - wcscpy(p, L"/usr/lib/debug/.build-id/"); - z = p + wcslen(p); - if (idlen) + if ((p = malloc((from_len + idlen * 2 + 1) * sizeof(WCHAR) + sizeof(L".debug")))) { + memcpy(p, from, from_len * sizeof(WCHAR)); + z = p + from_len; z = append_hex(z, id, id + 1); if (idlen > 1) { *z++ = L'/'; z = append_hex(z, id + 1, id + idlen); } + *z = L'\0'; + TRACE("checking %s\n", debugstr_w(p)); + found = image_check_debug_link_gnu_id(p, fmap_link, id, idlen); + if (!found) + { + wcscpy(z, L".debug"); + TRACE("checking %s\n", debugstr_w(p)); + found = image_check_debug_link_gnu_id(p, fmap_link, id, idlen); + } + free(p); } - wcscpy(z, L".debug"); - TRACE("checking %s\n", debugstr_w(p)); + return found; +}
- if (image_check_debug_link_gnu_id(p, fmap_link, id, idlen)) - { - free(p); +/****************************************************************** + * image_locate_build_id_target + * + * Try to find an image file containing the debug info out of the build-id + * note information. + */ +static struct image_file_map* image_locate_build_id_target(const BYTE* id, unsigned idlen) +{ + struct image_file_map* fmap_link; + DWORD sz; + + if (!idlen) return NULL; + + if (!(fmap_link = HeapAlloc(GetProcessHeap(), 0, sizeof(*fmap_link)))) + return NULL; + if (image_locate_build_id_target_in_dir(fmap_link, id, idlen, L"/usr/lib/debug/.build-id/")) + return fmap_link; + if (image_locate_build_id_target_in_dir(fmap_link, id, idlen, L"/usr/lib/.build-id/")) return fmap_link; - }
sz = GetEnvironmentVariableW(L"WINEHOMEDIR", NULL, 0); if (sz) { - z = realloc(p, sz * sizeof(WCHAR) + - sizeof(L"\.cache\debuginfod_client\") + - idlen * 2 * sizeof(WCHAR) + sizeof(L"\debuginfo") + 500); - if (!z) goto fail; - p = z; - GetEnvironmentVariableW(L"WINEHOMEDIR", p, sz); - z = p + sz - 1; - wcscpy(z, L"\.cache\debuginfod_client\"); - z += wcslen(z); - z = append_hex(z, id, id + idlen); - wcscpy(z, L"\debuginfo"); - TRACE("checking %ls\n", p); - if (image_check_debug_link_gnu_id(p, fmap_link, id, idlen)) + WCHAR *p, *z; + p = malloc(sz * sizeof(WCHAR) + + sizeof(L"\.cache\debuginfod_client\") + + idlen * 2 * sizeof(WCHAR) + sizeof(L"\debuginfo") + 500); + if (p && GetEnvironmentVariableW(L"WINEHOMEDIR", p, sz) == sz - 1) { + BOOL found; + + wcscpy(p + sz - 1, L"\.cache\debuginfod_client\"); + z = p + wcslen(p); + z = append_hex(z, id, id + idlen); + wcscpy(z, L"\debuginfo"); + TRACE("checking %ls\n", p); + found = image_check_debug_link_gnu_id(p, fmap_link, id, idlen); free(p); - return fmap_link; + if (found) return fmap_link; } }
TRACE("not found\n"); -fail: - free(p); + HeapFree(GetProcessHeap(), 0, fmap_link); return NULL; }
From: Eric Pouech epouech@codeweavers.com
When stripping, binutils' objcopy also sets the flag, yet without stripping into a .DBG file.
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/dbghelp/pe_module.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 8071b8987e3..74f5b08e613 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -656,8 +656,8 @@ static BOOL pe_load_msc_debug_info(const struct process* pcs, struct module* mod if (nDbg != 1 || dbg->Type != IMAGE_DEBUG_TYPE_MISC || misc->DataType != IMAGE_DEBUG_MISC_EXENAME) { - ERR("-Debug info stripped, but no .DBG file in module %s\n", - debugstr_w(module->modulename)); + WARN("-Debug info stripped, but no .DBG file in module %s\n", + debugstr_w(module->modulename)); } else {
From: Eric Pouech epouech@codeweavers.com
Esp. the failures with ERROR_BAD_EXE_FORMAT still fill all the fields in the returned structure.
Add more flexibility to .dbg file creation (optional DEBUG_DIRECTORY).
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/dbghelp/tests/path.c | 121 ++++++++++++++++++++++++++------------ 1 file changed, 85 insertions(+), 36 deletions(-)
diff --git a/dlls/dbghelp/tests/path.c b/dlls/dbghelp/tests/path.c index 804d6885ec3..83d43008983 100644 --- a/dlls/dbghelp/tests/path.c +++ b/dlls/dbghelp/tests/path.c @@ -671,20 +671,22 @@ static BOOL create_test_pdb_ds(const WCHAR* pdb_name, const GUID* guid, DWORD ag return TRUE; }
-static BOOL create_test_dbg(const WCHAR* dbg_name, WORD machine, DWORD timestamp, DWORD size) +static BOOL create_test_dbg(const WCHAR* dbg_name, WORD machine, DWORD charac, DWORD timestamp, DWORD size, struct debug_directory_blob *blob) { HANDLE hfile;
/* minimalistic .dbg made of a header and a DEBUG_DIRECTORY without any data */ - const IMAGE_SEPARATE_DEBUG_HEADER header = {.Signature = 0x4944 /* DI */, - .Flags = 0, .Machine = machine, .Characteristics = 0x010E, .TimeDateStamp = timestamp, + IMAGE_SEPARATE_DEBUG_HEADER header = + { + .Signature = 0x4944 /* DI */, + .Flags = 0, .Machine = machine, .Characteristics = charac, .TimeDateStamp = timestamp, .CheckSum = 0, .ImageBase = 0x00040000, .SizeOfImage = size, .NumberOfSections = 0, - .ExportedNamesSize = 0, .DebugDirectorySize = sizeof(IMAGE_DEBUG_DIRECTORY)}; - const IMAGE_DEBUG_DIRECTORY debug_dir = {.Characteristics = 0, .TimeDateStamp = timestamp + 1, - .MajorVersion = 0, .MinorVersion = 0, .Type = IMAGE_DEBUG_TYPE_CODEVIEW, - .SizeOfData = 0, .AddressOfRawData = 0, - .PointerToRawData = sizeof(header) + header.NumberOfSections * sizeof(IMAGE_SECTION_HEADER) + - header.DebugDirectorySize}; + .ExportedNamesSize = 0, .DebugDirectorySize = 0 + }; + DWORD where, expected_size;; + + if (blob) + header.DebugDirectorySize = sizeof(IMAGE_DEBUG_DIRECTORY);
hfile = CreateFileW(dbg_name, GENERIC_WRITE, FILE_SHARE_READ, NULL, CREATE_ALWAYS, 0, 0); ok(hfile != INVALID_HANDLE_VALUE, "failed to create %ls err %lu\n", dbg_name, GetLastError()); @@ -692,8 +694,18 @@ static BOOL create_test_dbg(const WCHAR* dbg_name, WORD machine, DWORD timestamp
check_write_file(hfile, &header, sizeof(header)); /* FIXME: 0 sections... as header.NumberOfSections */ - check_write_file(hfile, &debug_dir, sizeof(debug_dir)); - ok(SetFilePointer(hfile, 0, NULL, FILE_CURRENT) == debug_dir.PointerToRawData, "mismatch\n"); + if (blob) + { + where = SetFilePointer(hfile, 0, NULL, FILE_CURRENT); + blob->debug_directory.PointerToRawData = (blob->debug_directory.SizeOfData) ? + where + sizeof(IMAGE_DEBUG_DIRECTORY) : 0; + check_write_file(hfile, &blob->debug_directory, sizeof(IMAGE_DEBUG_DIRECTORY)); + check_write_file(hfile, blob->content, blob->debug_directory.SizeOfData); + } + expected_size = sizeof(header) + header.NumberOfSections * sizeof(IMAGE_SECTION_HEADER); + if (blob) + expected_size += sizeof(IMAGE_DEBUG_DIRECTORY) + blob->debug_directory.SizeOfData; + ok(SetFilePointer(hfile, 0, NULL, FILE_CURRENT) == expected_size, "Incorrect file length\n");
CloseHandle(hfile); return TRUE; @@ -725,16 +737,16 @@ static void test_srvgetindexes_pe(void) DWORD sig; WCHAR pdb_name[16]; WCHAR dbg_name[16]; - BOOL in_error; + DWORD last_error; } indexes[] = { /* error cases */ -/* 0 */{0, {-1, -1, -1}, .in_error = TRUE}, - {IMAGE_FILE_DEBUG_STRIPPED, { 0, -1, -1}, .in_error = TRUE}, - {IMAGE_FILE_DEBUG_STRIPPED, { 1, -1, -1}, .in_error = TRUE}, - {IMAGE_FILE_DEBUG_STRIPPED, { 2, -1, -1}, .in_error = TRUE}, - {IMAGE_FILE_DEBUG_STRIPPED, {-1, -1, -1}, .in_error = TRUE}, /* not 100% logical ! */ +/* 0 */{0, {-1, -1, -1}, 0, &null_guid, 0, .last_error = ERROR_BAD_EXE_FORMAT}, + {IMAGE_FILE_DEBUG_STRIPPED, { 0, -1, -1}, 0, &null_guid, 0, .last_error = ERROR_BAD_EXE_FORMAT}, + {IMAGE_FILE_DEBUG_STRIPPED, { 1, -1, -1}, 123, &null_guid, 0xaaaabbbb, .pdb_name = L"pdbjg.pdb", .last_error = ERROR_BAD_EXE_FORMAT}, + {IMAGE_FILE_DEBUG_STRIPPED, { 2, -1, -1}, 124, &guid1, 0, .pdb_name = L"pdbds.pdb", .last_error = ERROR_BAD_EXE_FORMAT}, + {IMAGE_FILE_DEBUG_STRIPPED, {-1, -1, -1}, 0, &null_guid, 0, .last_error = ERROR_BAD_EXE_FORMAT}, /* not 100% logical ! */ /* success */ /* 5 */{0, { 0, -1, -1}, 0, &null_guid, 0 }, {0, { 1, -1, -1}, 123, &null_guid, 0xaaaabbbb, .pdb_name = L"pdbjg.pdb"}, @@ -796,16 +808,18 @@ static void test_srvgetindexes_pe(void) memset(&ssii, 0xa5, sizeof(ssii)); ssii.sizeofstruct = sizeof(ssii); ret = SymSrvGetFileIndexInfoW(filename, &ssii, 0); - if (indexes[i].in_error) + if (indexes[i].last_error) { ok(!ret, "SymSrvGetFileIndexInfo should have failed\n"); - ok(GetLastError() == ERROR_BAD_EXE_FORMAT, "Mismatch in GetLastError: %lu\n", GetLastError()); + ok(GetLastError() == indexes[i].last_error, "Mismatch in GetLastError: %lu\n", GetLastError()); } else - { ok(ret, "SymSrvGetFileIndexInfo failed: %lu\n", GetLastError()); - - ok(ssii.age == indexes[i].age, "Mismatch in age: %lx\n", ssii.age); + if (ret || indexes[i].last_error == ERROR_BAD_EXE_FORMAT) + { + todo_wine_if(i==0||i==4) + { + ok(ssii.age == indexes[i].age, "Mismatch in age: %lu\n", ssii.age); ok(IsEqualGUID(&ssii.guid, indexes[i].guid), "Mismatch in guid: guid=%s\n", wine_dbgstr_guid(&ssii.guid));
@@ -815,9 +829,13 @@ static void test_srvgetindexes_pe(void) (!ssii.stripped) == ((indexes[i].charac & IMAGE_FILE_DEBUG_STRIPPED) == 0), "Mismatch in stripped: %x\n", ssii.stripped); ok(ssii.timestamp == 0x67890000 + i, "Mismatch in timestamp: %lx\n", ssii.timestamp); + } + if (i > 4) + { ok(!wcscmp(ssii.file, filename + 2), "Mismatch in file: '%ls'\n", ssii.file); ok(!wcscmp(ssii.dbgfile, indexes[i].dbg_name), "Mismatch in dbgfile: '%ls'\n", ssii.dbgfile); ok(!wcscmp(ssii.pdbfile, indexes[i].pdb_name), "Mismatch in pdbfile: '%ls'\n", ssii.pdbfile); + } } ret = DeleteFileW(filename); ok(ret, "Couldn't delete test DLL file\n"); @@ -882,48 +900,78 @@ static void test_srvgetindexes_dbg(void) WCHAR filename[128]; SYMSRV_INDEX_INFOW ssii; BOOL ret; + struct debug_directory_blob *blob_refs[1];
static struct { /* input parameters */ - WORD machine; - DWORD timestamp; - DWORD imagesize; + WORD machine; + DWORD characteristics; + DWORD timestamp; + DWORD imagesize; + int blob; + /* output parameters */ + DWORD age; + const GUID *guid; + WCHAR pdbname[16]; + WCHAR dbgname[16]; + DWORD last_error; } indexes[] = { - {IMAGE_FILE_MACHINE_I386, 0x1234, 0x00560000}, - {IMAGE_FILE_MACHINE_AMD64, 0x1235, 0x00570000}, + {IMAGE_FILE_MACHINE_I386, 0, 0x1234, 0x00560000, -1, 0, &null_guid, .last_error = ERROR_BAD_EXE_FORMAT}, + {IMAGE_FILE_MACHINE_AMD64, 0, 0x1235, 0x00570000, -1, 0, &null_guid, .last_error = ERROR_BAD_EXE_FORMAT}, + {IMAGE_FILE_MACHINE_I386, 0, 0x1234, 0x00560000, 0, 123, &guid1, .pdbname=L"foo.pdb"}, + {IMAGE_FILE_MACHINE_AMD64, 0, 0x1235, 0x00570000, 0, 123, &guid1, .pdbname=L"foo.pdb"}, }; + + blob_refs[0] = make_pdb_ds_blob(0x1226, &guid1, 123, "foo.pdb"); for (i = 0; i < ARRAY_SIZE(indexes); i++) { winetest_push_context("dbg#%02u", i);
/* create dll */ swprintf(filename, ARRAY_SIZE(filename), L"winetest%02u.dbg", i); - ret = create_test_dbg(filename, indexes[i].machine, indexes[i].timestamp, indexes[i].imagesize); + ret = create_test_dbg(filename, indexes[i].machine, indexes[i].characteristics, + indexes[i].timestamp, indexes[i].imagesize, + indexes[i].blob == -1 ? NULL : blob_refs[indexes[i].blob]); ok(ret, "Couldn't create dbg file %ls\n", filename);
memset(&ssii, 0x45, sizeof(ssii)); ssii.sizeofstruct = sizeof(ssii); ret = SymSrvGetFileIndexInfoW(filename, &ssii, 0); - ok(ret, "SymSrvGetFileIndexInfo failed: %lu\n", GetLastError()); + if (indexes[i].last_error) + { + ok(!ret, "SymSrvGetFileIndexInfo should have\n"); + ok(GetLastError() == ERROR_BAD_EXE_FORMAT, "Unexpected last error: %lu\n", GetLastError()); + } + else + ok(ret, "SymSrvGetFileIndexInfo failed: %lu\n", GetLastError());
- ok(ssii.age == 0, "Mismatch in age: %lx\n", ssii.age); - ok(!memcmp(&ssii.guid, &null_guid, sizeof(GUID)), + todo_wine + { + ok(ssii.age == indexes[i].age, "Mismatch in age: %lx\n", ssii.age); + ok(IsEqualGUID(&ssii.guid, indexes[i].guid), "Mismatch in guid: guid=%s\n", wine_dbgstr_guid(&ssii.guid)); - + } + todo_wine_if(i < 2) + { ok(ssii.sig == 0, "Mismatch in sig: %lx\n", ssii.sig); ok(ssii.size == indexes[i].imagesize, "Mismatch in size: %lx\n", ssii.size); ok(!ssii.stripped, "Mismatch in stripped: %x\n", ssii.stripped); ok(ssii.timestamp == indexes[i].timestamp, "Mismatch in timestamp: %lx\n", ssii.timestamp); + } + if (i >= 2) ok(!wcscmp(ssii.file, filename), "Mismatch in file: %ls\n", ssii.file); - ok(!ssii.pdbfile[0], "Mismatch in pdbfile: %ls\n", ssii.pdbfile); - ok(!ssii.dbgfile[0], "Mismatch in dbgfile: %ls\n", ssii.dbgfile); + todo_wine + ok(!wcscmp(ssii.pdbfile, indexes[i].pdbname), "Mismatch in pdbfile: %ls\n", ssii.pdbfile); + if (i >= 2) + ok(!wcscmp(ssii.dbgfile, indexes[i].dbgname), "Mismatch in dbgfile: %ls\n", ssii.dbgfile);
DeleteFileW(filename); winetest_pop_context(); } + for (i = 0; i < ARRAY_SIZE(blob_refs); i++) free(blob_refs[i]); }
static void make_path(WCHAR file[MAX_PATH], const WCHAR* topdir, const WCHAR* subdir, const WCHAR* base) @@ -1533,7 +1581,8 @@ static void test_load_modules_path(void) if (test_files[val].guid) create_test_pdb_ds(filename, test_files[val].guid, test_files[val].age_or_timestamp); else - create_test_dbg(filename, IMAGE_FILE_MACHINE_AMD64 /* FIXME */, test_files[val].age_or_timestamp, 0x40000 * val * 0x20000); + /*create_test_dbg(filename, IMAGE_FILE_MACHINE_AMD64, 0x10E, test_files[val].age_or_timestamp, 0x40000 * val * 0x20000, blob); */ + ok(0, "not supported yet\n"); } else ok(0, "Unrecognized file reference %c\n", *ptr); } @@ -1754,7 +1803,7 @@ static void test_load_modules_details(void) if (test_files[val].guid) create_test_pdb_ds(filename, test_files[val].guid, test_files[val].age_or_timestamp); else - create_test_dbg(filename, IMAGE_FILE_MACHINE_AMD64 /* FIXME */, test_files[val].age_or_timestamp, 0x40000 * val * 0x20000); + create_test_dbg(filename, IMAGE_FILE_MACHINE_AMD64 /* FIXME */, 0x10E, test_files[val].age_or_timestamp, 0x40000 * val * 0x20000, NULL); } else ok(0, "Unrecognized file reference %c\n", *ptr); }
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/dbghelp/msc.c | 29 +++++++++++++---------------- dlls/dbghelp/path.c | 2 +- dlls/dbghelp/pe_module.c | 2 +- dlls/dbghelp/tests/path.c | 15 --------------- 4 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/dlls/dbghelp/msc.c b/dlls/dbghelp/msc.c index e2e91e1abd2..d2bbf7f784c 100644 --- a/dlls/dbghelp/msc.c +++ b/dlls/dbghelp/msc.c @@ -4555,38 +4555,35 @@ DWORD msc_get_file_indexinfo(void* image, const IMAGE_DEBUG_DIRECTORY* debug_dir num_misc_records++; } } - return info->stripped && !num_misc_records ? ERROR_BAD_EXE_FORMAT : ERROR_SUCCESS; + return (!num_dir || (info->stripped && !num_misc_records)) ? ERROR_BAD_EXE_FORMAT : ERROR_SUCCESS; }
DWORD dbg_get_file_indexinfo(void* image, DWORD size, SYMSRV_INDEX_INFOW* info) { const IMAGE_SEPARATE_DEBUG_HEADER *header; + IMAGE_DEBUG_DIRECTORY *dbg; DWORD num_directories;
- if (size < sizeof(*header)) return ERROR_BAD_EXE_FORMAT; + if (size < sizeof(*header)) return ERROR_BAD_FORMAT; header = image; if (header->Signature != 0x4944 /* DI */ || size < sizeof(*header) + header->NumberOfSections * sizeof(IMAGE_SECTION_HEADER) + header->ExportedNamesSize + header->DebugDirectorySize) - return ERROR_BAD_EXE_FORMAT; + return ERROR_BAD_FORMAT; + + info->size = header->SizeOfImage; + /* seems to use header's timestamp, not debug_directory one */ + info->timestamp = header->TimeDateStamp; + info->stripped = FALSE; /* FIXME */
/* header is followed by: * - header->NumberOfSections of IMAGE_SECTION_HEADER * - header->ExportedNameSize * - then num_directories of IMAGE_DEBUG_DIRECTORY */ + dbg = (IMAGE_DEBUG_DIRECTORY*)((char*)(header + 1) + + header->NumberOfSections * sizeof(IMAGE_SECTION_HEADER) + + header->ExportedNamesSize); num_directories = header->DebugDirectorySize / sizeof(IMAGE_DEBUG_DIRECTORY);
- if (!num_directories) return ERROR_BAD_EXE_FORMAT; - - info->age = 0; - memset(&info->guid, 0, sizeof(info->guid)); - info->sig = 0; - info->dbgfile[0] = L'\0'; - info->pdbfile[0] = L'\0'; - info->size = header->SizeOfImage; - /* seems to use header's timestamp, not debug_directory one */ - info->timestamp = header->TimeDateStamp; - info->stripped = FALSE; /* FIXME */ - - return ERROR_SUCCESS; + return msc_get_file_indexinfo(image, dbg, num_directories, info); } diff --git a/dlls/dbghelp/path.c b/dlls/dbghelp/path.c index 0f52bca3684..c1fc6addb0c 100644 --- a/dlls/dbghelp/path.c +++ b/dlls/dbghelp/path.c @@ -832,7 +832,7 @@ BOOL WINAPI SymSrvGetFileIndexInfoW(const WCHAR *file, SYMSRV_INDEX_INFOW* info, if (hMap) CloseHandle(hMap); if (hFile != INVALID_HANDLE_VALUE) CloseHandle(hFile);
- if (ret == ERROR_SUCCESS) wcscpy(info->file, file_name(file)); /* overflow? */ + if (ret == ERROR_SUCCESS || ret == ERROR_BAD_EXE_FORMAT) wcscpy(info->file, file_name(file)); /* overflow? */ SetLastError(ret); return ret == ERROR_SUCCESS; } diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 74f5b08e613..75060e2dfa6 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -1019,7 +1019,7 @@ DWORD pe_get_file_indexinfo(void* image, DWORD size, SYMSRV_INDEX_INFOW* info) if (!(nthdr = RtlImageNtHeader(image))) return ERROR_BAD_FORMAT;
dbg = RtlImageDirectoryEntryToData(image, FALSE, IMAGE_DIRECTORY_ENTRY_DEBUG, &dirsize); - if (!dbg || dirsize < sizeof(dbg)) return ERROR_BAD_EXE_FORMAT; + if (!dbg) dirsize = 0;
/* fill in information from NT header */ info->timestamp = nthdr->FileHeader.TimeDateStamp; diff --git a/dlls/dbghelp/tests/path.c b/dlls/dbghelp/tests/path.c index 83d43008983..8a27effcdfc 100644 --- a/dlls/dbghelp/tests/path.c +++ b/dlls/dbghelp/tests/path.c @@ -817,8 +817,6 @@ static void test_srvgetindexes_pe(void) ok(ret, "SymSrvGetFileIndexInfo failed: %lu\n", GetLastError()); if (ret || indexes[i].last_error == ERROR_BAD_EXE_FORMAT) { - todo_wine_if(i==0||i==4) - { ok(ssii.age == indexes[i].age, "Mismatch in age: %lu\n", ssii.age); ok(IsEqualGUID(&ssii.guid, indexes[i].guid), "Mismatch in guid: guid=%s\n", wine_dbgstr_guid(&ssii.guid)); @@ -829,13 +827,9 @@ static void test_srvgetindexes_pe(void) (!ssii.stripped) == ((indexes[i].charac & IMAGE_FILE_DEBUG_STRIPPED) == 0), "Mismatch in stripped: %x\n", ssii.stripped); ok(ssii.timestamp == 0x67890000 + i, "Mismatch in timestamp: %lx\n", ssii.timestamp); - } - if (i > 4) - { ok(!wcscmp(ssii.file, filename + 2), "Mismatch in file: '%ls'\n", ssii.file); ok(!wcscmp(ssii.dbgfile, indexes[i].dbg_name), "Mismatch in dbgfile: '%ls'\n", ssii.dbgfile); ok(!wcscmp(ssii.pdbfile, indexes[i].pdb_name), "Mismatch in pdbfile: '%ls'\n", ssii.pdbfile); - } } ret = DeleteFileW(filename); ok(ret, "Couldn't delete test DLL file\n"); @@ -948,24 +942,15 @@ static void test_srvgetindexes_dbg(void) else ok(ret, "SymSrvGetFileIndexInfo failed: %lu\n", GetLastError());
- todo_wine - { ok(ssii.age == indexes[i].age, "Mismatch in age: %lx\n", ssii.age); ok(IsEqualGUID(&ssii.guid, indexes[i].guid), "Mismatch in guid: guid=%s\n", wine_dbgstr_guid(&ssii.guid)); - } - todo_wine_if(i < 2) - { ok(ssii.sig == 0, "Mismatch in sig: %lx\n", ssii.sig); ok(ssii.size == indexes[i].imagesize, "Mismatch in size: %lx\n", ssii.size); ok(!ssii.stripped, "Mismatch in stripped: %x\n", ssii.stripped); ok(ssii.timestamp == indexes[i].timestamp, "Mismatch in timestamp: %lx\n", ssii.timestamp); - } - if (i >= 2) ok(!wcscmp(ssii.file, filename), "Mismatch in file: %ls\n", ssii.file); - todo_wine ok(!wcscmp(ssii.pdbfile, indexes[i].pdbname), "Mismatch in pdbfile: %ls\n", ssii.pdbfile); - if (i >= 2) ok(!wcscmp(ssii.dbgfile, indexes[i].dbgname), "Mismatch in dbgfile: %ls\n", ssii.dbgfile);
DeleteFileW(filename);
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- dlls/dbghelp/dbghelp_private.h | 2 ++ dlls/dbghelp/module.c | 20 ++++++++++++++++++-- dlls/dbghelp/pe_module.c | 24 ++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/dlls/dbghelp/dbghelp_private.h b/dlls/dbghelp/dbghelp_private.h index 33714e73bc1..f113e88003e 100644 --- a/dlls/dbghelp/dbghelp_private.h +++ b/dlls/dbghelp/dbghelp_private.h @@ -787,6 +787,8 @@ extern BOOL pe_unmap_directory(struct module* module, int dirno, const c extern DWORD pe_get_file_indexinfo(void* image, DWORD size, SYMSRV_INDEX_INFOW* info); extern const BYTE* pe_lock_region_from_rva(struct module *module, DWORD rva, DWORD size, DWORD *length); extern BOOL pe_unlock_region(struct module *module, const BYTE* region); +struct image_file_map; +extern BOOL pe_has_buildid_debug(struct image_file_map *fmap, GUID *guid);
/* source.c */ extern unsigned source_new(struct module* module, const char* basedir, const char* source); diff --git a/dlls/dbghelp/module.c b/dlls/dbghelp/module.c index f5130010538..84b3173a27f 100644 --- a/dlls/dbghelp/module.c +++ b/dlls/dbghelp/module.c @@ -525,6 +525,7 @@ static BOOL image_check_debug_link_gnu_id(const WCHAR* file, struct image_file_m { struct image_section_map buildid_sect; DWORD read_bytes; + GUID guid; HANDLE handle; WCHAR *path; WORD magic; @@ -543,6 +544,9 @@ static BOOL image_check_debug_link_gnu_id(const WCHAR* file, struct image_file_m ret = elf_map_handle(handle, fmap); CloseHandle(handle);
+ if (ret && pe_has_buildid_debug(fmap, &guid)) + return TRUE; + if (ret && image_find_section(fmap, ".note.gnu.build-id", &buildid_sect)) { const UINT32* note; @@ -555,12 +559,13 @@ static BOOL image_check_debug_link_gnu_id(const WCHAR* file, struct image_file_m { if (note[1] == idlen && !memcmp(note + 3 + ((note[0] + 3) >> 2), id, idlen)) return TRUE; - WARN("mismatch in buildid information for %s\n", debugstr_w(file)); } } image_unmap_section(&buildid_sect); image_unmap_file(fmap); } + if (ret) + WARN("mismatch in buildid information for %s\n", debugstr_w(file)); return FALSE; }
@@ -847,7 +852,18 @@ BOOL image_check_alternate(struct image_file_map* fmap, const struct module* mod struct image_file_map* fmap_link = NULL;
/* if present, add the .gnu_debuglink file as an alternate to current one */ - if (image_find_section(fmap, ".note.gnu.build-id", &buildid_sect)) + if (fmap->modtype == DMT_PE) + { + GUID guid; + + if (pe_has_buildid_debug(fmap, &guid)) + { + /* reorder bytes to match little endian order */ + fmap_link = image_locate_build_id_target((const BYTE*)&guid, sizeof(guid)); + } + } + /* if present, add the .note.gnu.build-id as an alternate to current one */ + if (!fmap_link && image_find_section(fmap, ".note.gnu.build-id", &buildid_sect)) { const UINT32* note;
diff --git a/dlls/dbghelp/pe_module.c b/dlls/dbghelp/pe_module.c index 75060e2dfa6..bd3d6000535 100644 --- a/dlls/dbghelp/pe_module.c +++ b/dlls/dbghelp/pe_module.c @@ -1036,3 +1036,27 @@ DWORD pe_get_file_indexinfo(void* image, DWORD size, SYMSRV_INDEX_INFOW* info) } return msc_get_file_indexinfo(image, dbg, dirsize / sizeof(*dbg), info); } + +/* check if image contains a debug entry that contains a gcc/mingw - clang build-id information */ +BOOL pe_has_buildid_debug(struct image_file_map *fmap, GUID *guid) +{ + BOOL ret = FALSE; + + if (fmap->modtype == DMT_PE) + { + SYMSRV_INDEX_INFOW info = {.sizeofstruct = sizeof(info)}; + const void *image = pe_map_full(fmap, NULL); + + if (image) + { + DWORD retval = pe_get_file_indexinfo((void*)image, GetFileSize(fmap->u.pe.hMap, NULL), &info); + if ((retval == ERROR_SUCCESS || retval == ERROR_BAD_EXE_FORMAT) && info.age && !info.pdbfile[0]) + { + *guid = info.guid; + ret = TRUE; + } + pe_unmap_full(fmap); + } + } + return ret; +}
From: Eric Pouech epouech@codeweavers.com
Signed-off-by: Eric Pouech epouech@codeweavers.com --- configure.ac | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/configure.ac b/configure.ac index 0f5b89d1b47..27813382846 100644 --- a/configure.ac +++ b/configure.ac @@ -998,16 +998,23 @@ This is an error since --enable-archs=$wine_arch was requested.])]) if test "x$ac_debug_format_seen" = x then case $wine_crossdebug in - *dwarf) WINE_TRY_PE_CFLAGS([-gdwarf-4]) ;; + *dwarf) WINE_TRY_PE_CFLAGS([-gdwarf-4],[:], + [WINE_TRY_PE_CFLAGS([-gdwarf-4 -Wl,-debug:dwarf], + [AS_VAR_APPEND([${wine_arch}_CFLAGS],[" -gdwarf-4"]) + AS_VAR_APPEND([${wine_arch}_LDFLAGS],[" -Wl,-debug:dwarf"])])]) ;; pdb) WINE_TRY_PE_CFLAGS([-gcodeview]) ;; esac fi AS_VAR_SET([${wine_arch}_DEBUG],[$wine_crossdebug])
test "x$enable_werror" != xyes || WINE_TRY_PE_CFLAGS([-Werror]) - test "x$enable_build_id" != xyes || WINE_TRY_PE_CFLAGS([-Wl,--build-id], - [AS_VAR_APPEND([${wine_arch}_CFLAGS],[" -Wl,--build-id"]) - AS_VAR_APPEND([${wine_arch}_LDFLAGS],[" -Wl,--build-id"])]) + if test "x$enable_build_id" = xyes + then + WINE_TRY_PE_CFLAGS([-Wl,--build-id], + [AS_VAR_APPEND([${wine_arch}_LDFLAGS],[" -Wl,--build-id"])], + [WINE_TRY_PE_CFLAGS([-Wl,-build-id], + [AS_VAR_APPEND([${wine_arch}_LDFLAGS],[" -Wl,-build-id"])])]) + fi
done
Thank you! The command for distributions looks broken. Could you check?
How it looks for me: (Firefox on Arch Linux) {width=792 height=76}
On Fri Oct 25 13:49:48 2024 +0000, Chih-Hsuan Yen wrote:
Thank you! The command for distributions looks broken. Could you check? How it looks for me: (Firefox on Arch Linux) {width=792 height=76}
Yeah, does that for me too.
Or rather, did. Fixed.
Thank you very much for this patch! I managed to get separate debug files working. Besides this merge request, [pacman](https://gitlab.archlinux.org/yan12125/pacman/-/commits/pe-symbols) and [wine package](https://gitlab.archlinux.org/archlinux/packaging/packages/wine/-/tree/strip-...) changes are also needed.
At the pacman side, I need to [skip creation of some symlinks](https://gitlab.archlinux.org/yan12125/pacman/-/commit/0a0c5dc57d685c58715519...). I'm not sure what's the purpose of `/usr/lib/debug/.build-id/xxx/yyy` links (those that do not end with `.debug`) for ELF files, and apparently they confuse dbghelp.
At the wine package side, I need to replace `-g` in `CROSSCFLAGS` and `CROSSCXXFLAGS` with `-gdwarf-4`. I understand that wine supports up to DWARF 4, while I remember wine automatically used `-gdwarf-4` when given `-g` some time ago. If this is an intended change, maybe documenting it somewhere is good.
if binutils isn't compiled with PE support (this is apparently the Arch case)
To clarify, I was not sure if it's okay to just use binutils instead of mingw-w64-binutils. It seems fine after more testing.
On Fri Nov 15 08:08:11 2024 +0000, Chih-Hsuan Yen wrote:
Thank you very much for this patch! I managed to get separate debug files working. Besides this merge request, [pacman](https://gitlab.archlinux.org/yan12125/pacman/-/commits/pe-symbols) and [wine package](https://gitlab.archlinux.org/archlinux/packaging/packages/wine/-/tree/strip-...) changes are also needed. At the pacman side, I need to [skip creation of some symlinks](https://gitlab.archlinux.org/yan12125/pacman/-/commit/0a0c5dc57d685c58715519...). I'm not sure what's the purpose of `/usr/lib/debug/.build-id/xxx/yyy` links (those that do not end with `.debug`) for ELF files, and apparently they confuse dbghelp. At the wine package side, I need to replace `-g` in `CROSSCFLAGS` and `CROSSCXXFLAGS` with `-gdwarf-4`. I understand that wine supports up to DWARF 4, while I remember wine automatically used `-gdwarf-4` when given `-g` some time ago. If this is an intended change, maybe documenting it somewhere is good.
if binutils isn't compiled with PE support (this is apparently the
Arch case) To clarify, I was not sure if it's okay to just use binutils instead of mingw-w64-binutils. It seems fine after more testing.
At the pacman side, I need to [skip creation of some symlinks](https://gitlab.archlinux.org/yan12125/pacman/-/commit/0a0c5dc57d685c58715519...). I'm not sure what's the purpose of `/usr/lib/debug/.build-id/xxx/yyy` links (those that do not end with `.debug`) for ELF files, and apparently they confuse dbghelp.
That's not the intent. Something must be wrong somewhere. In the link to patched strip.sh.in file, the call to package_source_file is meant to be done in both elf and pe cases. It's not clear to me if you did your tests with that part active or not.
The generic idea I'm pushing for is to maximize the sharing of code betwen elf and pe cases. So what I would do for the changes in strip.sh.in: - don't pass an additional binary_type variable - keep the fallback from readelf -h in tidy_script() to add PE files for calling collect_debug_symbols - don't change collect_debug_symbols() - in build_id(); if readelf fails, fallback to reading the buildid with objcopy
At the end, running wine with WINEDEBUG=+dbghelp should give you the actual path the split debuginfo is read from (and it should be from /usr/lib/debug/.build_id)
At the wine package side, I need to replace `-g` in `CROSSCFLAGS` and `CROSSCXXFLAGS` with `-gdwarf-4`. I understand that wine supports up to DWARF 4, while I remember wine automatically used `-gdwarf-4` when given `-g` some time ago. If this is an intended change, maybe documenting it somewhere is good.
No, that's not intended, and the expected behavior is what you describe (CROSSCFLAGS=-g should end up with -gdwarf-4). But I can't repro here. Could you share your configure options? Maybe I didn't retest with your combination.
In the link to patched strip.sh.in file, the call to package_source_file is meant to be done in both elf and pe cases. It's not clear to me if you did your tests with that part active or not.
I disabled `package_source_file` as it does not work for PE as-is. That function eventually calls `debugedit`, which seems to support only ELF for now. I didn't survey equivalent commands for PE due to lack of time.
The generic idea I'm pushing for is to maximize the sharing of code betwen elf and pe cases. So what I would do for the changes in strip.sh.in:
Thanks for the suggestion. I prefer to avoid fallbacks as that pacman script already runs for ages, particularly for packages with hundreds of PE files like wine.
At the end, running wine with WINEDEBUG=+dbghelp should give you the actual path the split debuginfo is read from (and it should be from /usr/lib/debug/.build_id)
I remember dbghelp fails to find symbols when `/usr/lib/debug/.build-id/xxx/yyy` exists. Specifically, dbghelp only looks into such a file when searching for debugging symbols, and `/usr/lib/debug/.build-id/xxx/yyy.debug` is not checked at all. I don't have time to re-test and gather logs, though.
On Sat Nov 16 02:56:50 2024 +0000, Chih-Hsuan Yen wrote:
In the link to patched strip.sh.in file, the call to
package_source_file is meant to be done in both elf and pe cases. It's not clear to me if you did your tests with that part active or not. I disabled `package_source_file` as it does not work for PE as-is. That function eventually calls `debugedit`, which seems to support only ELF for now. I didn't survey equivalent commands for PE due to lack of time.
The generic idea I'm pushing for is to maximize the sharing of code
betwen elf and pe cases. So what I would do for the changes in strip.sh.in: Thanks for the suggestion. I prefer to avoid fallbacks as that pacman script already runs for ages, particularly for packages with hundreds of PE files like wine.
At the end, running wine with WINEDEBUG=+dbghelp should give you the
actual path the split debuginfo is read from (and it should be from /usr/lib/debug/.build_id) I remember dbghelp fails to find symbols when `/usr/lib/debug/.build-id/xxx/yyy` exists. Specifically, dbghelp only looks into such a file when searching for debugging symbols, and `/usr/lib/debug/.build-id/xxx/yyy.debug` is not checked at all. I don't have time to re-test and gather logs, though.
dbghelp (with this MR) searches both /usr/lib/debug/.build-id/xxx/yyy and /usr/lib/debug/.build-id/xxx/yyy.debug so something else must be going wrong.
(I'll rebase this MR after MR!6825 has been merged)
On Mon Nov 18 14:49:36 2024 +0000, eric pouech wrote:
dbghelp (with this MR) searches both /usr/lib/debug/.build-id/xxx/yyy and /usr/lib/debug/.build-id/xxx/yyy.debug so something else must be going wrong. (I'll rebase this MR after MR!6825 has been merged)
Sounds good, thank you for the efforts. I will re-test after this is rebased.
-
Alfred Agrell (＠Alcaro) -
Chih-Hsuan Yen (＠yan12125)
-
Eric Pouech
-
eric pouech (＠epo)