[PATCH v5 0/2] MR4518: ntdll: Integrate with GDB using r_debug.
Effectively implementing the debugger interface as described in https://sourceware.org/git/?p=glibc.git;a=blob;f=elf/rtld-debugger-interface... This is an actual attempt at having this upstream, split from https://gitlab.winehq.org/wine/wine/-/merge_requests/1074 which people have been finding useful for debugging Wine directly in GDB. This supports everything you would expect, from automatic symbol loading as well as cross-syscall backtraces with the new Python unwinder. -- v5: ntdll: Maintain a PE module link map and expose it to GDB. https://gitlab.winehq.org/wine/wine/-/merge_requests/4518
From: Rémi Bernon <rbernon(a)codeweavers.com> --- loader/main.c | 3 ++- loader/preloader.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/loader/main.c b/loader/main.c index c258e025183..5a208cc048c 100644 --- a/loader/main.c +++ b/loader/main.c @@ -38,7 +38,8 @@ extern char **environ; -/* the preloader will set this variable */ +/* the preloader will set these variables */ +__attribute((visibility("default"))) struct r_debug *wine_r_debug = NULL; const __attribute((visibility("default"))) struct wine_preload_info *wine_main_preload_info = NULL; /* canonicalize path and return its directory name */ diff --git a/loader/preloader.c b/loader/preloader.c index d0551bae63a..776f593608d 100644 --- a/loader/preloader.c +++ b/loader/preloader.c @@ -79,12 +79,16 @@ #ifdef HAVE_ELF_H # include <elf.h> #endif + +/* define _r_debug so we can re-define it as r_debug_extended */ +#define _r_debug no_r_debug; #ifdef HAVE_LINK_H # include <link.h> #endif #ifdef HAVE_SYS_LINK_H # include <sys/link.h> #endif +#undef _r_debug #include "wine/asm.h" #include "main.h" @@ -1387,6 +1391,33 @@ static void set_process_name( int argc, char *argv[] ) for (i = 1; i < argc; i++) argv[i] -= off; } +/* GDB integration, _dl_debug_state is *required* for GDB to hook the preloader */ +__attribute((visibility("default"))) void _dl_debug_state(void) {} +__attribute((visibility("default"))) struct r_debug_extended _r_debug = {{0}}; + +/* sets the preloader r_debug address into DT_DEBUG */ +static void init_r_debug( struct wld_auxv *av ) +{ + ElfW(Phdr) *phdr, *ph; + ElfW(Dyn) *dyn = NULL; + char *l_addr; + int phnum; + + _r_debug.base.r_version = 2; + _r_debug.base.r_brk = (ElfW(Addr))_dl_debug_state; + + if (!(phnum = get_auxiliary( av, AT_PHNUM, 0 ))) return; + if (!(phdr = (void *)get_auxiliary( av, AT_PHDR, 0 ))) return; + l_addr = (char *)phdr - sizeof(ElfW(Ehdr)); + _r_debug.base.r_ldbase = (ElfW(Addr))l_addr; + + for (ph = phdr; ph < &phdr[phnum]; ++ph) if (ph->p_type == PT_DYNAMIC) break; + if (ph >= &phdr[phnum]) return; + + dyn = (void *)(ph->p_vaddr + l_addr); + while (dyn->d_tag != DT_DEBUG && dyn->d_tag != DT_NULL) dyn++; + if (dyn->d_tag == DT_DEBUG) dyn->d_un.d_ptr = (uintptr_t)&_r_debug; +} /* * wld_start @@ -1403,6 +1434,7 @@ void* wld_start( void **stack ) struct wld_auxv new_av[8], delete_av[3], *av; struct wld_link_map main_binary_map, ld_so_map; struct wine_preload_info **wine_main_preload_info; + struct r_debug *ld_so_r_debug, **wine_r_debug; pargc = *stack; argv = (char **)pargc + 1; @@ -1432,6 +1464,8 @@ void* wld_start( void **stack ) dump_auxiliary( av ); #endif + init_r_debug( av ); + /* reserve memory that Wine needs */ if (reserve) preload_reserve( reserve ); for (i = 0; preload_info[i].size; i++) @@ -1470,6 +1504,15 @@ void* wld_start( void **stack ) interp = (char *)main_binary_map.l_addr + main_binary_map.l_interp; map_so_lib( interp, &ld_so_map ); + /* expose ld.so _r_debug as a separate namespace in r_next */ + ld_so_r_debug = find_symbol( &ld_so_map, "_r_debug", STT_OBJECT ); + if (ld_so_r_debug) _r_debug.r_next = (struct r_debug_extended *)ld_so_r_debug; + else wld_printf( "_r_debug not found in ld.so\n" ); + + wine_r_debug = find_symbol( &main_binary_map, "wine_r_debug", STT_OBJECT ); + if (wine_r_debug) *wine_r_debug = &_r_debug.base; + else wld_printf( "wine_r_debug not found\n" ); + /* store pointer to the preload info into the appropriate main binary variable */ wine_main_preload_info = find_symbol( &main_binary_map, "wine_main_preload_info", STT_OBJECT ); if (wine_main_preload_info) *wine_main_preload_info = preload_info; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4518
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/unix/virtual.c | 130 +++++++++++++++++++++++++++++++++++++- 1 file changed, 129 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/virtual.c b/dlls/ntdll/unix/virtual.c index 02c0bebe879..13bc0b4aa03 100644 --- a/dlls/ntdll/unix/virtual.c +++ b/dlls/ntdll/unix/virtual.c @@ -51,6 +51,12 @@ #ifdef HAVE_SYS_USER_H # include <sys/user.h> #endif +#ifdef HAVE_LINK_H +# include <link.h> +#endif +#ifdef HAVE_SYS_LINK_H +# include <sys/link.h> +#endif #ifdef HAVE_LIBPROCSTAT_H # include <libprocstat.h> #endif @@ -79,6 +85,120 @@ WINE_DEFAULT_DEBUG_CHANNEL(virtual); WINE_DECLARE_DEBUG_CHANNEL(module); WINE_DECLARE_DEBUG_CHANNEL(virtual_ranges); +/* Gdb integration, in loader/main.c */ +static struct r_debug *wine_r_debug; + +#if defined(HAVE_LINK_H) || defined(HAVE_SYS_LINK_H) + +struct link_map_entry +{ + struct link_map map; + const void *module; +}; +static struct link_map link_map = {.l_name = (char *)""}; + +static void r_debug_set_state( int state ) +{ + wine_r_debug->r_map = &link_map; + wine_r_debug->r_state = state; + ((void (*)(void))wine_r_debug->r_brk)(); +} + +static char *get_path_from_fd( int fd, int sz ) +{ +#ifdef linux + char *ret = malloc( 32 + sz ); + + if (ret) sprintf( ret, "/proc/self/fd/%u", fd ); + return ret; +#elif defined(F_GETPATH) + char *ret = malloc( PATH_MAX + sz ); + + if (!ret) return NULL; + if (!fcntl( fd, F_GETPATH, ret )) return ret; + free( ret ); + return NULL; +#else + return NULL; +#endif +} + +static char *r_debug_path_from_fd( int fd ) +{ + char *real, *path; + if (!(path = get_path_from_fd( fd, 0 ))) return NULL; + if (!(real = realpath( path, NULL ))) return path; + free( path ); + return real; +} + +static void r_debug_add_module( void *module, int fd, INT_PTR offset ) +{ + struct link_map *ptr = link_map.l_next, *next; + struct link_map_entry *entry; + + if (!wine_r_debug) return; + + while (ptr) + { + entry = LIST_ENTRY(ptr, struct link_map_entry, map); + if (entry->module == module) break; + ptr = ptr->l_next; + } + + r_debug_set_state( RT_ADD ); + + if (ptr) entry->map.l_addr = offset; + else if ((entry = calloc( 1, sizeof(*entry) ))) + { + entry->module = module; + entry->map.l_addr = offset; + entry->map.l_name = r_debug_path_from_fd( fd ); + + entry->map.l_next = link_map.l_next; + if ((next = entry->map.l_next)) next->l_prev = &entry->map; + entry->map.l_prev = &link_map; + link_map.l_next = &entry->map; + } + + r_debug_set_state( RT_CONSISTENT ); +} + +static void r_debug_remove_module( void *module ) +{ + struct link_map *ptr = link_map.l_next, *next; + struct link_map_entry *entry; + + if (!wine_r_debug) return; + + while (ptr) + { + entry = LIST_ENTRY(ptr, struct link_map_entry, map); + if (entry->module == module) break; + ptr = ptr->l_next; + } + if (!ptr) return; + + r_debug_set_state( RT_DELETE ); + + entry->map.l_prev->l_next = entry->map.l_next; + if ((next = entry->map.l_next)) next->l_prev = entry->map.l_prev; + + r_debug_set_state( RT_CONSISTENT ); + + free( entry->map.l_name ); + free( entry ); +} + +#else /* defined(HAVE_LINK_H) || defined(HAVE_SYS_LINK_H) */ + +#define RT_CONSISTENT 0 +static void r_debug_set_state( int state ) {} +static void r_debug_add_module( void *module, int fd, INT_PTR offset ) {} +static void r_debug_remove_module( void *module ) {} + +#endif /* defined(HAVE_LINK_H) || defined(HAVE_SYS_LINK_H) */ + struct preload_info { void *addr; @@ -2924,6 +3044,7 @@ static NTSTATUS map_image_into_view( struct file_view *view, const WCHAR *filena #ifdef VALGRIND_LOAD_PDB_DEBUGINFO VALGRIND_LOAD_PDB_DEBUGINFO(fd, ptr, total_size, ptr - (char *)wine_server_get_ptr( image_info->base )); #endif + r_debug_add_module( ptr, fd, ptr - (char *)wine_server_get_ptr( image_info->base ) ); return STATUS_SUCCESS; } @@ -3279,11 +3400,14 @@ static void *alloc_virtual_heap( SIZE_T size ) void virtual_init(void) { const struct preload_info **preload_info = dlsym( RTLD_DEFAULT, "wine_main_preload_info" ); + struct r_debug **r_debug = dlsym( RTLD_DEFAULT, "wine_r_debug" ); const char *preload = getenv( "WINEPRELOADRESERVE" ); size_t size; int i; pthread_mutexattr_t attr; + if (r_debug && (wine_r_debug = *r_debug)) r_debug_set_state( RT_CONSISTENT ); + pthread_mutexattr_init( &attr ); pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ); pthread_mutex_init( &virtual_mutex, &attr ); @@ -5641,7 +5765,11 @@ static NTSTATUS unmap_view_of_section( HANDLE process, PVOID addr, ULONG flags ) SERVER_END_REQ; if (!status) { - if (view->protect & SEC_IMAGE) release_builtin_module( view->base ); + if (view->protect & SEC_IMAGE) + { + r_debug_remove_module( view->base ); + release_builtin_module( view->base ); + } if (flags & MEM_PRESERVE_PLACEHOLDER) free_pages_preserve_placeholder( view, view->base, view->size ); else delete_view( view ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4518
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=143098 Your paranoid android. === debian11 (build log) === ../wine/loader/preloader.c:1396:45: error: variable ���_r_debug��� has initializer but incomplete type ../wine/loader/preloader.c:1396:74: error: extra brace group at end of initializer ../wine/loader/preloader.c:1406:13: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1407:13: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1412:13: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1509:32: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1513:48: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1396:62: error: storage size of ���_r_debug��� isn���t known Task: The win32 Wine build failed === debian11b (build log) === ../wine/loader/preloader.c:1396:45: error: variable ���_r_debug��� has initializer but incomplete type ../wine/loader/preloader.c:1396:74: error: extra brace group at end of initializer ../wine/loader/preloader.c:1406:13: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1407:13: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1412:13: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1509:32: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1513:48: error: invalid use of undefined type ���struct r_debug_extended��� ../wine/loader/preloader.c:1396:62: error: storage size of ���_r_debug��� isn���t known Task: The wow64 Wine build failed
On Wed Feb 14 20:59:34 2024 +0000, Fabian Maurer wrote:
Well, so far the filename was never used for anything except for debug logs, so it didn't matter yet. But when loading `c:\users\fabian\Temp\IXP010.TMP\ADVPACK.DLL`, wine will load the builtin DLL instead, which has a different path. But you'd still pass GDB the filename `c:\users\fabian\Temp\IXP010.TMP\ADVPACK.DLL`, which is wrong. I'm pretty sure this is the issue with the breakpoints as well (my other comment), since GDB loads data from the dlls inside `c:\windows\system32` instead of my build folder. GDB should be passed the path of the actual loaded DLL, not the one that the the apps see. Since Alexandre said we can't expose that real path to apps though, you have to get it a different way. For example see https://gitlab.winehq.org/DarkShadow44/wine/-/commit/ef8888b33622adcc3823665..., this fixes the issues I encountered. Not guaranteeing correctness or anything, it's more a POC. I see, I pushed an update which should address this nicely. Since we have the actual file descriptor being mapped we can resolve the path from it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_61203
On Wed Feb 14 21:36:40 2024 +0000, Rémi Bernon wrote:
I see, I pushed an update which should address this nicely. Since we have the actual file descriptor being mapped we can resolve the path from it. Awesome thanks!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_61204
Fwiw, using the custom python unwinder in https://gitlab.winehq.org/rbernon/wine/-/commit/d14edd5abc0791692660acb8ac8c... and this Gdb change https://gitlab.winehq.org/rbernon/binutils-gdb/-/commit/7725b24533c218787e36... which I sent to the gdb-patches mailing-list, it is possible to unwind through `__wine_(syscall|unix_call)_dispatcher` as well as back from user callbacks through `KiUserCallbackDispatcher`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_61333
Jacek Caban (@jacek) commented about loader/preloader.c:
for (i = 1; i < argc; i++) argv[i] -= off; }
+/* GDB integration, _dl_debug_state is *required* for GDB to hook the preloader */ +__attribute((visibility("default"))) void _dl_debug_state(void) {}
You never call `_dl_debug_state` in this commit. Does it work without it or are you relying on debugger figuring things out when it's called from ntdll? An explicit `_dl_debug_state` call in `wld_start` would seem right to me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62582
Jacek Caban (@jacek) commented about loader/preloader.c:
interp = (char *)main_binary_map.l_addr + main_binary_map.l_interp; map_so_lib( interp, &ld_so_map );
+ /* expose ld.so _r_debug as a separate namespace in r_next */ + ld_so_r_debug = find_symbol( &ld_so_map, "_r_debug", STT_OBJECT ); + if (ld_so_r_debug) _r_debug.r_next = (struct r_debug_extended *)ld_so_r_debug; + else wld_printf( "_r_debug not found in ld.so\n" ); + + wine_r_debug = find_symbol( &main_binary_map, "wine_r_debug", STT_OBJECT ); + if (wine_r_debug) *wine_r_debug = &_r_debug.base; + else wld_printf( "wine_r_debug not found\n" );
While `_r_debug` itself seems good to allow the debugger to hook Unix libs, I'm not sure if it's the best choice for PE files. The whole `wine_r_debug` thing and ntdll looking into the preloader does not seem appealing to me. Do we need this for PE files anyway? There is also an interface for JIT code [1] that looks similar, but simpler, maybe we could use that in the second commit instead. [1] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Declarations.html#Dec... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62583
On Sun Feb 25 19:44:17 2024 +0000, Jacek Caban wrote:
While `_r_debug` itself seems good to allow the debugger to hook Unix libs, I'm not sure if it's the best choice for PE files. The whole `wine_r_debug` thing and ntdll looking into the preloader does not seem appealing to me. Do we need this for PE files anyway? There is also an interface for JIT code [1] that looks similar, but simpler, maybe we could use that in the second commit instead. [1] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Declarations.html#Dec... The interface for JIT code is meant for block of code without any corresponding file on disk. It requires to load the debug info in memory, and attach it to the debugger notifications. It doesn't even support passing the module file name.
This doesn't seem like a good fit to me, as we would need to find and load the debug info ourselves. On the other hand, using `_r_debug` would let us benefit from all the standard mechanisms. For instance, using `_r_debug` Gdb can find the debug link and load split debug info directly. Or even read the build-id, and lookup the PE file debug info in the systems debug info database. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62598
On Sun Feb 25 19:41:34 2024 +0000, Jacek Caban wrote:
You never call `_dl_debug_state` in this commit. Does it work without it or are you relying on debugger figuring things out when it's called from ntdll? An explicit `_dl_debug_state` call in `wld_start` would seem right to me. Gdb first looks for `DT_DEBUG` in the dynamic table, expecting to find the address of `_r_debug` there, but it's not set there until we write it ourselves. I couldn't find how to make the linker put `_r_debug` there, so then it falls back to looking for known rendez-vous symbols such as `_dl_debug_state`, and sets a breakpoint into it.
If neither of this succeeds, Gdb bails out early and will never try again. If it does, it will later retry `DT_DEBUG` again, for instance when stopping the execution, and will find our `_r_debug` after we've set it. Then sure, I can probably call `_dl_debug_state` so it gets used. Note that I have found an issue still: Gdb doesn't seem to put breakpoints in every `_r_debug.r_brk`, only in `_dl_debug_state`. This means that although it is refreshed and you can see it in `info shared` list, debug symbols for entries in ld.so link_map don't get automatically loaded. They are only loaded when `_dl_debug_state` is called, and although we call it for the PE module updates, we then miss the unix .so updates. I don't see any other way to do that reliably for every .so than to hook `dlopen` and call `_dl_debug_state` there, like I was doing before (but we still don't actually need to mirror the link map). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62599
On Mon Feb 26 07:00:09 2024 +0000, Rémi Bernon wrote:
Gdb first looks for `DT_DEBUG` in the dynamic table, expecting to find the address of `_r_debug` there, but it's not set there until we write it ourselves. I couldn't find how to make the linker put `_r_debug` there, so then it falls back to looking for known rendez-vous symbols such as `_dl_debug_state`, and sets a breakpoint into it. If neither of this succeeds, Gdb bails out early and will never try again. If it does, it will later retry `DT_DEBUG` again, for instance when stopping the execution, and will find our `_r_debug` after we've set it. Then sure, I can probably call `_dl_debug_state` so it gets used. Note that I have found an issue still: Gdb doesn't seem to put breakpoints in every `_r_debug.r_brk`, only in `_dl_debug_state`. This means that although it is refreshed and you can see it in `info shared` list, debug symbols for entries in ld.so link_map don't get automatically loaded. They are only loaded when `_dl_debug_state` is called, and although we call it for the PE module updates, we then miss the unix .so updates. I don't see any other way to do that reliably for every .so than to hook `dlopen` and call `_dl_debug_state` there, like I was doing before (but we still don't actually need to mirror the link map). Do you know if GDB not putting a breakpoints on extended `_r_debug` is intended or a bug? It's not clear to me; I guess that if it's meant for a single loader with multiple namespaces then a single `r_brk` is enough, but the way those structures are arranged suggests that multiple break points make sense.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62718
On Mon Feb 26 20:38:39 2024 +0000, Jacek Caban wrote:
Do you know if GDB not putting a breakpoints on extended `_r_debug` is intended or a bug? It's not clear to me; I guess that if it's meant for a single loader with multiple namespaces then a single `r_brk` is enough, but the way those structures are arranged suggests that multiple break points make sense. As far as I can see, it's not obviously a bug. It simply doesn't bother and only supports setting a breakpoint in the first `r_brk`. It even only does that when it can locate the main `_r_debug` from the `DT_DEBUG` entry statically, like I described above and which doesn't even work here. Otherwise a single breakpoint is set in one of the rendez-vous symbols instead, which might not even be the same as the main `r_brk`.
So, I think the options to make sure ld.so link map updates are seen are: 1) Redefine dlopen to hook it, and call the rendez-vous breakpoint before / after it has completed. We lose a bit of granularity as Gdb will only load the symbols after the constructors have been executed. 2) Hotpatch ld.so rendez-vous symbols to jump into ours. Hum? 3) Patch Gdb to hook every `r_brk` once it has located the first `_r_debug` (triggered the first time we call the rendez-vous breakpoint). The last one is probably the cleanest but I'm still waiting for a feedback on my much simpler Python unwinder change, so... Well, I can always keep a fork like I do for Valgrind in the meantime. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62844
On Tue Feb 27 18:49:27 2024 +0000, Rémi Bernon wrote:
As far as I can see, it's not obviously a bug. It simply doesn't bother and only supports setting a breakpoint in the first `r_brk`. It even only does that when it can locate the main `_r_debug` from the `DT_DEBUG` entry statically, like I described above and which doesn't even work here. Otherwise a single breakpoint is set in one of the rendez-vous symbols instead, which might not even be the same as the main `r_brk`. So, I think the options to make sure ld.so link map updates are seen are: 1) Redefine dlopen to hook it, and call the rendez-vous breakpoint before / after it has completed. We lose a bit of granularity as Gdb will only load the symbols after the constructors have been executed. 2) Hotpatch ld.so rendez-vous symbols to jump into ours (it doesn't call it through its `r_brk` but statically). Hum? 3) Patch Gdb to hook every `r_brk` once it has located the first `_r_debug` (triggered the first time we call the rendez-vous breakpoint). The last one is probably the cleanest but I'm still waiting for a feedback on my much simpler Python unwinder change, so... Well, I can always keep a fork like I do for Valgrind in the meantime. A 1b) option is to call the rendez-vous from ntdll, when it dlopen a library, this is also good enough for some use cases, though it'll miss dlopen called from elsewhere (but still could be enough if called soon after).
Also, this is only for automatic symbol loading, it's not 100% mandatory and it's always possible to manually load the symbols later from the shared library list entry. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62845
participants (4)
-
Fabian Maurer (＠DarkShadow44) -
Jacek Caban (＠jacek)
-
Marvin -
Rémi Bernon