[PATCH v6 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. -- v6: ntdll: Maintain a PE module link map and expose it to GDB. loader: Expose the standard debugging symbols for 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 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..f27d6df8c17 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,39 @@ static void set_process_name( int argc, char *argv[] ) for (i = 1; i < argc; i++) argv[i] -= off; } +struct wld_r_debug_extended +{ + struct r_debug base; + struct wld_r_debug_extended *r_next; +}; + +/* GDB integration, _r_debug_state is *required* for GDB to hook the preloader */ +__attribute((visibility("default"))) void _r_debug_state(void) {} +__attribute((visibility("default"))) struct wld_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))_r_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 +1440,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 +1470,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 +1510,17 @@ 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 wld_r_debug_extended *)ld_so_r_debug; + else wld_printf( "_r_debug not found in ld.so\n" ); + + _r_debug_state(); /* notify GDB that _r_debug is ready */ + + 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
Updated with an additional call of the rendez-vous function when everything is setup. Renamed from `_dl_debug_state` to `_r_debug_state` which is equivalent for GDB but looks more consistent with `_r_debug`. Also added a custom `wld_r_debug_extended` definition as the `r_debug_extended` structure seems to be missing in some older distributions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62960
On Wed Feb 28 19:15:39 2024 +0000, Rémi Bernon wrote:
changed this line in [version 6 of the diff](/wine/wine/-/merge_requests/4518/diffs?diff_id=102108&start_sha=3c4cb10af12f107e94a2808829d6a2878dafe716#8de1f39ed579b059e12d845afcdf25dd7603ab8a_1395_1394) Fwiw I have implemented 3) in https://gitlab.winehq.org/rbernon/binutils-gdb/-/commit/f9f0ebde1e53d4553e50..., though I don't intend to send it upstream until I hear back from my previous submission.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_62961
On Wed Feb 28 19:18:36 2024 +0000, Rémi Bernon wrote:
Fwiw I have implemented 3) in https://gitlab.winehq.org/rbernon/binutils-gdb/-/commit/f9f0ebde1e53d4553e50..., though I don't intend to send it upstream until I hear back from my previous submission. Thanks, that looks promising. I think that changing gdb like that is the cleanest solution.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_63117
On Mon Feb 26 06:48:52 2024 +0000, Rémi Bernon wrote:
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. I don't see a reason why debugger wouldn't be able to load symbols, symbol links or build ids from the file when passed a PE file. My understanding is that the separated symbol blobs are meant to simplify things for JIT engines, so they can just generate a blobs without full ELF, but debugger could just fallback to its normal treatment when its got a proper image.
For file module file names, the debugger could do the same thing as perf does and try to get it from the mapping. Moving the overhead of getting file names from Wine (where it's paid even when no debugger is involved) to the debugger does not seem like a bad thing. And even without that, it shouldn't be needed to get symbols, unless some relative links are involved. That said, I generally like the `_r_debug` idea. If we can make it work, that's fine with me. It's just preloader, loader, ntdll interaction that is not great. I don't have a good idea how to improve that part, the best I could think of is to try harder to not do it at all and handle PEs separately in ntdll. With JIT interface, the preloader part and debugger patch would still be needed to make gdb intercept ld.so (although perhaps we could then just set `DT_DEBUG` to ld.so `_r_debug` and use `_r_debug_state` to trigger gdb with your patch to hook it). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_63118
On Thu Feb 29 21:58:43 2024 +0000, Jacek Caban wrote:
I don't see a reason why debugger wouldn't be able to load symbols, symbol links or build ids from the file when passed a PE file. My understanding is that the separated symbol blobs are meant to simplify things for JIT engines, so they can just generate a blobs without full ELF, but debugger could just fallback to its normal treatment when its got a proper image. For file module file names, the debugger could do the same thing as perf does and try to get it from the mapping. Moving the overhead of getting file names from Wine (where it's paid even when no debugger is involved) to the debugger does not seem like a bad thing. And even without that, it shouldn't be needed to get symbols, unless some relative links are involved. That said, I generally like the `_r_debug` idea. If we can make it work, that's fine with me. It's just preloader, loader, ntdll interaction that is not great. I don't have a good idea how to improve that part, the best I could think of is to try harder to not do it at all and handle PEs separately in ntdll. With JIT interface, the preloader part and debugger patch would still be needed to make gdb intercept ld.so (although perhaps we could then just set `DT_DEBUG` to ld.so `_r_debug` and use `_r_debug_state` to trigger gdb with your patch to hook it). Quoting the GDB manual https://sourceware.org/gdb/current/onlinedocs/gdb.html/Registering-Code.html... [emphasis mine]
To register code with GDB, the JIT should follow this protocol:
Generate an object file **in memory** with symbols and other desired debug information. The file must include the **virtual addresses** of the sections.
This sounds twice as complicated as r\_debug approach. Also, holding mostly-unused debug info will just waste memory. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_66072
On Tue May 21 00:59:37 2024 +0000, Fabian Maurer wrote:
I found another issue: When changing a DLL and adding a few newlines, then GDB still uses the old line numbers for the stacktrace / breakpoints. `wineboot -u` after changing the DLL helps. Just noting: This was solved long ago, was the same issue with the wrong filepaths being used.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_70996
There's been loader changes resulting in merge conflicts -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_74853
On Mon Sep 16 17:22:29 2024 +0000, Fabian Maurer wrote:
There's been loader changes resulting in merge conflicts There's also another merge conflict in `virtual_init()`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_82315
Any updates on this? Would be really nice to have gdb integration upstreamed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_89346
On Tue Mar 26 02:18:26 2024 +0000, Jinoh Kang wrote:
Quoting the GDB manual https://sourceware.org/gdb/current/onlinedocs/gdb.html/Registering-Code.html... [emphasis mine]
To register code with GDB, the JIT should follow this protocol:
Generate an object file **in memory** with symbols and other desired debug information. The file must include the **virtual addresses** of the sections. This sounds twice as complicated as r\_debug approach. Also, holding mostly-unused debug info will just waste memory. I gave it a quick try, and I don't see what benefit it has over the r_debug approach.
The debugger still looks up a couple of predefined symbols which it hooks, these *have* to live in wine preloader, otherwise the debugger cannot find them, and it's pretty much the same kind of thing as `_dl_debug_state` for notifications and `r_debug` for the module map, but with different names. Then, it doesn't save us the interactions between loader/wine and ntdll as the JIT map also needs to be populated, and that's pretty much something only ntdll can do, and ntdll needs to call the hooked function when it's done updating the map. Also, last thing, I've tried with GDB and it looks like it expects the in-memory blobs to be object files, opens them with libbfd and also checks that it's compatible architecture with the debugged target. Both of these fail when passing the in-memory modules as-is, and we would need to generate ELF object blobs with the debug infos dynamically for every module. That seem more complicated than the `r_debug` approach which works OOTB. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4518#note_94789
participants (5)
-
Aida Jonikienė -
Fabian Maurer (＠DarkShadow44)
-
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)
-
Rémi Bernon