On 5/7/21 9:27 PM, Simon Marchi wrote: > On 2021-05-07 4:44 a.m., Tom de Vries wrote: >> Hi, >> >> In a linux kernel mailing list discussion, it was mentioned that "gdb has >> this odd thing where it takes the 64-bit vs 32-bit data for the whole process >> from one thread, and picks the worst possible thread to do it (ie explicitly >> not even the main thread, ...)" [1]. >> >> The picking of the thread is done here in >> x86_linux_nat_target::read_description: >> ... >> /* GNU/Linux LWP ID's are process ID's. */ >> tid = inferior_ptid.lwp (); >> if (tid == 0) >> tid = inferior_ptid.pid (); /* Not a threaded program. */ >> ... >> >> To understand what this code does, let's investigate a scenario in which >> inferior_ptid.lwp () != inferior_ptid.pid (). >> >> Say we start exec jit-attach-pie, identified with pid x. The main thread >> starts another thread that sleeps, and then the main thread waits for the >> sleeping thread. So we have two threads, identified with LWP IDs x and x+1: >> ... >> PID LWP CMD >> x x ./jit-attach-pie >> x x+1 ./jit-attach-pie >> ... >> [ The thread with LWP x is known as the thread group leader. ] >> >> When attaching to this exec using the pid, gdb does a stop_all_threads which >> iterates over all the threads, first LWP x, and then LWP x+1. >> >> So the state we arrive with at x86_linux_nat_target::read_description is: >> ... >> (gdb) p inferior_ptid >> $1 = {m_pid = x, m_lwp = x+1, m_tid = 0} >> ... >> and consequently we probe 64/32-bitness from thread LWP x+1. >> >> [ Note that this is different from when gdb doesn't attach but instead >> launches the exec itself, in which case there's no stop_all_threads needed, >> and the probed thread is LWP x. ] > > Well, it's mostly because when running, there's only one thread to begin > with, Correct, I've updated the comment. > and the io_uring threads are likely not there yet. > I'm trying to discuss things here first relative to the current situation, so without the kernel patch that makes io_uring threads user-visible. >> According to aforementioned remark, a better choice would have been the main >> thread, that is, LWP x. > > That doesn't tell the whole story though. Ack, updated log message. > Up to now, probing the > non-main thread was maybe a slightly strange thing to do, but it worked > because all threads were set up the same way for that matter. It worked > under the assumption that all threads had the same CS/DS contents (the > registers we use to detect architecture). The failure happens when > there are io_uring threads present. These threads are of a strange kind > because they are like any userspace threads, except that they never > return to userspace. Because of that, they don't have a meaningful > userspace register state, reading their registers returns garbage. So > if we choose one of these threads for probing the architecture of the > process (which, as you mentioned below, is bogus, but that's how it > works now), then we get bad results. > > From what I read (from the lkml thread you linked), the kernel's plan is > to mitigate it by filling the register state of these threads to > something GDB is happy with, even though that's unnecessary otherwise. > I don't know what's the state of that though. But I agree it would be > good to fix this on our side as well. > >> >> This patch implement that choice, by simply doing: >> ... >> tid = inferior_ptid.pid (); > > I think that's ok. > >> ... >> >> The fact that gdb makes a per-process choice for 64/32-bitness is a problem in >> itself: each thread can be in either 64 or 32 bit mode. That is a problem >> that this patch doesn't fix. > > Not only that, but from what I understood, it's not right to decide once > and for all whether a thread is 32 or 64. It could switch between the > two modes. So all we can really know is what mode a thread is > currently, while it is stopped. But who knows what mode it's going to > be in 5 instructions from now. > Ack, I've updated the message to make that more clear. > This actually is a problem to people who debug early boot code with > QEMU, because there the "thread" goes from 32 to 64 bit mode at some > point. To handle this, it sounds like GDB should re-evaluate the > architecture of each thread every time it stops. That sounds expensive, > but maybe this mode could exist behind a knob that is disabled by > default, for these special cases. > > There might also be features that can only work if we assume the > architecture of a thread never changes, I'm not sure. > It would already be good if users could tell gdb that things have switched, currently not even that is possible, see PR27838. > Also, we already have kind of partial machinery for having threads with > different architectures, that's why we have > target_ops::thread_architecture. But read_description is used to set > inferior::gdbarch, which is kind of the "default" arch of the process, > used for many things. And the default implementation of > thread_architecture, process_stratum_target::thread_architecture, just > returns inf->gdbarch. So in practice, today, for Linux it's true that > we have one single architecture for the whole inferior. The exception > is aarch64-linux, which can return different architectures for different > SVE configurations. > >> Tested on x86_64-linux. >> >> [1] https://lore.kernel.org/io-uring/\ >> CAHk-=wh0KoEZXPYMGkfkeVEerSCEF1AiCZSvz9TRrx=Kj74D+Q@mail.gmail.com/ > > Hmm, the link is not clickable / does not lead to the right place, > because it is split in two. > Yeah, I don't like lines overrunning, but OK, fixed. >> >> Any comments? > > 1. I think it would be good to have a test for this. I wanted to actually > try making (or finding) a program that uses io_uring and see the problem > with my own eyes, but I didn't have time yet. But there could be such a > test in our testsuite (we would check that the kernel actually supports > io_uring, of course). We could validate that when we select one of the > io threads and do things like backtrace, the failure is handled > gracefully. > Yeah, that sounds like something I could try once the kernel exposing io_uring threads to user-space lands in tumbleweed. > 2. There are other architectures than x86-64 where it looks like we would > also probe a non-main thread, like ARM: > > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/arm-linux-nat.c;h=662dade0a122f8adf25da91577b1de375df7785b;hb=HEAD > > It might not fail like it does on x86-64, it depends on the kernel > implementation. But we should probably change them the same way. > Ack, I've scanned the sources of other ports, and added a few more (cc-ing port maintainers). > 3. Have you tested attaching to a program where the main thread has > exited? It might still work (and there might be a test for it in our > testsuite) because the thread group leader stays around as a zombie as > long as other threads exist. But it would be good to test it. Good question. I've tried and got both with system gdb and patched upstream gdb the same result: ... $ gdb -q -p $(pidof a.out) Attaching to process 28988 warning: process 28988 is a zombie - the process has already terminated ptrace: Operation not permitted. (gdb) $ ~/gdb_versions/devel/gdb -q -p $(pidof a.out) Attaching to process 28988 warning: process 28988 is a zombie - the process has already terminated ptrace: Operation not permitted. (gdb) q ... Updated patch attached. Any further comments? Thanks, - Tom