Re: [PATCH][gdb/tdep] Use pid to choose x86_64 process 64/32-bitness

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

On 5/7/21 4:27 PM, Simon Marchi via Gdb-patches 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, and the io_uring threads are likely not there yet.
> 
>> 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.  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.
> 
> 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.
> 
> 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.

A while ago I suggested having a better mechanism to update the 
architecture of each thread mid-execution (in order to support SVE 
dynamic vector lenghts for gdbserver), including remote packet changes. 
But the feedback was that it would be too much overhead.

In summary, assuming a per-process architecture is not right for SVE and 
it sounds like we should be moving towards per-thread architectures in 
the future.

We'll need to dump per-thread GDB_TDESC notes for a core file as well.