RE: [PATCHv5 03/11] gdb/x86: move reading of cs and ds state into gdb/nat directory

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Andrew Burgess <aburgess@redhat.com>
> Sent: Freitag, 26. April 2024 17:02
> To: gdb-patches@sourceware.org
> Cc: Andrew Burgess <aburgess@redhat.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; John Baldwin <jhb@FreeBSD.org>
> Subject: [PATCHv5 03/11] gdb/x86: move reading of cs and ds state into gdb/nat
> directory
> 
> This patch is part of a series that has the aim sharing the x86 Linux
> target description creation code between GDB and gdbserver.
> 
> Within GDB part of this process involves reading the cs and ds state
> from the 'struct user_regs_struct' using a ptrace call.
> 
> This isn't done by gdbserver, which is part of the motivation for this
> whole series; the approach gdbserver takes is inferior to the approach
> GDB takes (gdbserver relies on reading the file being debugged, and
> extracting similar information from the file headers).
> 
> This commit moves the reading of cs and ds, which is used to figure
> out if a thread is 32-bit or 64-bit (or in x32 mode), into the gdb/nat
> directory so that the code can be shared with gdbserver, but at this
> point I'm not actually using the code in gdbserver, that will come
> later.
> 
> As such there should be no user visible changes after this commit, GDB
> continues to do things as it did before (reading cs/ds), while
> gdbserver continues to use its own approach (which doesn't require
> reading cs/ds).
> 
> Approved-By: John Baldwin <jhb@FreeBSD.org>
> ---
>  gdb/nat/x86-linux.c | 47
> +++++++++++++++++++++++++++++++++++++++++++++
>  gdb/nat/x86-linux.h | 28 +++++++++++++++++++++++++++
>  gdb/x86-linux-nat.c | 42 +++++-----------------------------------
>  3 files changed, 80 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c
> index 7a21c8f2c26..ad3ed3c2289 100644
> --- a/gdb/nat/x86-linux.c
> +++ b/gdb/nat/x86-linux.c
> @@ -19,6 +19,8 @@
> 
>  #include "x86-linux.h"
>  #include "x86-linux-dregs.h"
> +#include "nat/gdb_ptrace.h"
> +#include <sys/user.h>
> 
>  /* Per-thread arch-specific data we want to keep.  */
> 
> @@ -79,3 +81,48 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
>  {
>    x86_linux_update_debug_registers (lwp);
>  }
> +
> +#ifdef __x86_64__
> +/* Value of CS segment register:
> +     64bit process: 0x33
> +     32bit process: 0x23  */
> +#define AMD64_LINUX_USER64_CS 0x33
> +
> +/* Value of DS segment register:
> +     LP64 process: 0x0
> +     X32 process: 0x2b  */
> +#define AMD64_LINUX_X32_DS 0x2b
> +#endif
> +
> +/* See nat/x86-linux.h.  */
> +
> +x86_linux_arch_size
> +x86_linux_ptrace_get_arch_size (int tid)
> +{
> +#ifdef __x86_64__
> +  unsigned long cs;
> +  unsigned long ds;
> +
> +  /* Get CS register.  */
> +  errno = 0;
> +  cs = ptrace (PTRACE_PEEKUSER, tid,
> +	       offsetof (struct user_regs_struct, cs), 0);
> +  if (errno != 0)
> +    perror_with_name (_("Couldn't get CS register"));
> +
> +  bool is_64bit = cs == AMD64_LINUX_USER64_CS;
> +
> +  /* Get DS register.  */
> +  errno = 0;
> +  ds = ptrace (PTRACE_PEEKUSER, tid,
> +	       offsetof (struct user_regs_struct, ds), 0);
> +  if (errno != 0)
> +    perror_with_name (_("Couldn't get DS register"));
> +
> +  bool is_x32 = ds == AMD64_LINUX_X32_DS;
> +
> +  return x86_linux_arch_size (is_64bit, is_x32);
> +#else
> +  return x86_linux_arch_size (false, false);
> +#endif

The old function had:

    if (sizeof (void *) == 4 && is_64bit && !is_x32)
      error (_("Can't debug 64-bit process with 32-bit GDB"));

That makes me wonder: Was this just dead code
or do we actually need this still?
I saw that you add a similar message back in later commits.
But here there might be user visible change, contrary to your
commit message.

The rest looked fine to me.

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928