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

[Tom de Vries <tdevries@suse.de>]

[ correcting mistyped email address ]

On 5/22/21 10:32 AM, Tom de Vries wrote:
> On 5/21/21 12:50 PM, Alan Hayward wrote:
>>> I think it makes sense in any case to probe the main thread, we know
>>> it's always ok.
>>>
>>  ...But agreed this is the sensible option. Even if io_uring is ok, something else
>> could get added down the line which isn’t ok. Using pid should keep it safe.
>>
> Changes since last post:
> - added riscv64 (adding maintainers in cc)
> - added suggested change for arm
> - added comment at read_description in target.h
> 
> Any further comments?
> 
> Thanks,
> - Tom
> 
> 
> 0001-gdb-tdep-Use-pid-to-choose-process-64-32-bitness.patch
> 
> [gdb/tdep] Use pid to choose process 64/32-bitness
> 
> 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 just one thread to begin with,
> and consequently the probed thread is LWP x. ]
> 
> According to aforementioned remark, a better choice would have been the main
> thread, that is, LWP x.
> 
> This patch implement that choice, by simply doing:
> ...
>   tid = inferior_ptid.pid ();
> ...
> 
> The fact that gdb makes a per-process permanent choice for 64/32-bitness is a
> problem in itself: each thread can be in either 64 or 32 bit mode, and change
> forth and back.  That is a problem that this patch doesn't fix.
> 
> Now finally: why does this matter in the context of the linux kernel
> discussion?  The discussion was related to a patch that exposed io_uring
> threads to user-space.  This made it possible that one of those threads would
> be picked out to select 64/32-bitness.  Given that such threads are atypical
> user-space threads in the sense that they don't return to user-space and don't
> have a userspace register state, reading their registers returns garbage, 
> and
> so it could f.i. occur that in a 64-bit process with all normal user-space
> threads in 64-bit mode, the probing would return 32-bit.
> 
> It may be that this is worked-around on the kernel side by providing userspace
> register state in those threads such that current gdb is happy.  Nevertheless,
> it seems prudent to fix this on the gdb size as well.
> 
> Tested on x86_64-linux.
> 
> [1] https://lore.kernel.org/io-uring/CAHk-=wh0KoEZXPYMGkfkeVEerSCEF1AiCZSvz9TRrx=Kj74D+Q@mail.gmail.com/
> 
> gdb/ChangeLog:
> 
> 2021-05-07  Tom de Vries  <tdevries@suse.de>
> 
> 	PR tdep/27822
> 	* target.h (struct target_ops): Mention target_thread_architecture in
> 	read_description comment.
> 	* x86-linux-nat.c (x86_linux_nat_target::read_description): Use
> 	pid to determine if process is 64-bit or 32-bit.
> 	* aarch64-linux-nat.c (aarch64_linux_nat_target::read_description):
> 	Same.
> 	* ppc-linux-nat.c (ppc_linux_nat_target::read_description): Same.
>         * riscv-linux-nat.c (riscv_linux_nat_target::read_description): Same.
> 	* s390-linux-nat.c (s390_linux_nat_target::read_description): Same.
> 	* arm-linux-nat.c (arm_linux_nat_target::read_description): Same.
> 	Likewise, use pid to determine if kernel supports reading VFP
> 	registers.
> 
> ---
>  gdb/aarch64-linux-nat.c | 2 +-
>  gdb/arm-linux-nat.c     | 4 ++--
>  gdb/ppc-linux-nat.c     | 4 +---
>  gdb/riscv-linux-nat.c   | 2 +-
>  gdb/s390-linux-nat.c    | 2 +-
>  gdb/target.h            | 5 ++++-
>  gdb/x86-linux-nat.c     | 5 +----
>  7 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index ae8db2988c2..61224022f6a 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -723,7 +723,7 @@ aarch64_linux_nat_target::read_description ()
>    gdb_byte regbuf[ARM_VFP3_REGS_SIZE];
>    struct iovec iovec;
>  
> -  tid = inferior_ptid.lwp ();
> +  tid = inferior_ptid.pid ();
>  
>    iovec.iov_base = regbuf;
>    iovec.iov_len = ARM_VFP3_REGS_SIZE;
> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
> index 662dade0a12..880ac0da044 100644
> --- a/gdb/arm-linux-nat.c
> +++ b/gdb/arm-linux-nat.c
> @@ -537,7 +537,7 @@ arm_linux_nat_target::read_description ()
>      {
>        elf_gregset_t gpregs;
>        struct iovec iov;
> -      int tid = inferior_ptid.lwp ();
> +      int tid = inferior_ptid.pid ();
>  
>        iov.iov_base = &gpregs;
>        iov.iov_len = sizeof (gpregs);
> @@ -556,7 +556,7 @@ arm_linux_nat_target::read_description ()
>      {
>        /* Make sure that the kernel supports reading VFP registers.  Support was
>  	 added in 2.6.30.  */
> -      int pid = inferior_ptid.lwp ();
> +      int pid = inferior_ptid.pid ();
>        errno = 0;
>        char *buf = (char *) alloca (ARM_VFP3_REGS_SIZE);
>        if (ptrace (PTRACE_GETVFPREGS, pid, 0, buf) < 0 && errno == EIO)
> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index 171f5b386fa..06a30efeaef 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -1946,9 +1946,7 @@ ppc_linux_nat_target::auxv_parse (gdb_byte **readptr,
>  const struct target_desc *
>  ppc_linux_nat_target::read_description ()
>  {
> -  int tid = inferior_ptid.lwp ();
> -  if (tid == 0)
> -    tid = inferior_ptid.pid ();
> +  int tid = inferior_ptid.pid ();
>  
>    if (have_ptrace_getsetevrregs)
>      {
> diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
> index 04bf46b3bb1..c0f5a27a37e 100644
> --- a/gdb/riscv-linux-nat.c
> +++ b/gdb/riscv-linux-nat.c
> @@ -202,7 +202,7 @@ const struct target_desc *
>  riscv_linux_nat_target::read_description ()
>  {
>    const struct riscv_gdbarch_features features
> -    = riscv_linux_read_features (inferior_ptid.lwp ());
> +    = riscv_linux_read_features (inferior_ptid.pid ());
>    return riscv_lookup_target_description (features);
>  }
>  
> diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
> index 41b50ce4800..8f6eb61505b 100644
> --- a/gdb/s390-linux-nat.c
> +++ b/gdb/s390-linux-nat.c
> @@ -988,7 +988,7 @@ s390_linux_nat_target::auxv_parse (gdb_byte **readptr,
>  const struct target_desc *
>  s390_linux_nat_target::read_description ()
>  {
> -  int tid = s390_inferior_tid ();
> +  int tid = inferior_ptid.pid ();
>  
>    have_regset_last_break
>      = check_regset (tid, NT_S390_LAST_BREAK, 8);
> diff --git a/gdb/target.h b/gdb/target.h
> index d867a58e2a8..51139042691 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -841,7 +841,10 @@ struct target_ops
>      /* Describe the architecture-specific features of this target.  If
>         OPS doesn't have a description, this should delegate to the
>         "beneath" target.  Returns the description found, or NULL if no
> -       description was available.  */
> +       description was available.  This should return something that
> +       describes a whole process, and if there are things that are
> +       thread-specific, target_thread_architecture should be used for
> +       that.  */
>      virtual const struct target_desc *read_description ()
>  	 TARGET_DEFAULT_RETURN (NULL);
>  
> diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
> index 85c7f0ddc94..adea1ad0092 100644
> --- a/gdb/x86-linux-nat.c
> +++ b/gdb/x86-linux-nat.c
> @@ -113,10 +113,7 @@ x86_linux_nat_target::read_description ()
>    static uint64_t xcr0;
>    uint64_t xcr0_features_bits;
>  
> -  /* GNU/Linux LWP ID's are process ID's.  */
> -  tid = inferior_ptid.lwp ();
> -  if (tid == 0)
> -    tid = inferior_ptid.pid (); /* Not a threaded program.  */
> +  tid = inferior_ptid.pid ();
>  
>  #ifdef __x86_64__
>    {
>