[PATCH] Fix amd64->i386 linux syscall restart problem

[Pedro Alves]

On 3/17/19 5:13 AM, Kevin Buettner wrote:
> This commit fixes some failures in gdb.base/interrupt.exp
> when debugging a 32-bit i386 linux inferior from an amd64 host.
> 
> When running the following test...
> 
>   make check RUNTESTFLAGS="--target_board unix/-m32 interrupt.exp"
> 
> ... without this commit, I see the following output:
> 
> FAIL: gdb.base/interrupt.exp: continue (the program exited)
> FAIL: gdb.base/interrupt.exp: echo data
> FAIL: gdb.base/interrupt.exp: Send Control-C, second time
> FAIL: gdb.base/interrupt.exp: signal SIGINT (the program is no longer running)
> ERROR: Undefined command "".
> ERROR: GDB process no longer exists
> 
> 		=== gdb Summary ===
> 
> When the test is run with this commit in place, we see 12 passes
> instead.  This is the desired behavior.
> 
> Analysis:
> 
> On Linux, when a syscall is interrupted by a signal, the syscall
> may return -ERESTARTSYS when a signal occurs.  Doing so indicates that
> the syscall is restartable.  Then, depending on settings associated
> with the signal handler, and after the signal handler is called, the
> kernel can then either return -EINTR or can cause the syscall to be
> restarted.  In this discussion, we are concerned with the latter
> case.
> 
> On i386, the kernel returns this status via the EAX register.
> 
> When debugging a 32-bit (i386) process from a 64-bit (amd64)
> GDB, the debugger fetches 64-bit registers even though the
> process being debugged is 32-bit.  Since we're debugging a 32-bit
> target, only 32 bits are being saved in the register cache.
> Now, ideally, GDB would save all 64-bits in the regcache and
> then would be able to restore those same values when it comes
> time to continue the target.  I've looked into doing this, but
> it's not easy and I don't see many benefits to doing so.  

Yeah, it'd not be easy.  We'd have to make the target backend
report a 64-bit target description, and then make gdb expose a
32-bit view over the registers, using pseudo registers.  MIPS
port has something like that (mips64_transfers_32bit_regs_p),
though, so there's precedent.  An advantage in such a design is that 
would fix the troubles with debugging low level x86 code that changes
machine mode (16-bit/32-bit/64-bit), without losing any of the
high bits in the registers, which are also preserved by the cpu.
We could then even have a gdb knob to manually switch "view"
mode (16-bit/32-bit/64-bit), and that would not change how the
registers are transferred in the backend -- it would always work
at the max width the machine supports.  That is, an advantage, compared
to a solution that makes gdb fetch a new target description when
the mode changes.

> One
> benefit, however, would be that EAX would appear as a negative
> value for doing syscall restarts.
> 
> At the moment, GDB is setting the high 32 bits of RAX (and other
> registers too) to 0.  So, when GDB restores EAX just prior to
> a syscall restart, the high 32 bits of RAX are zeroed, thus making
> it look like a positive value.  For this particular purpose, we
> need to sign extend EAX so that RAX will appear as a negative
> value when EAX is set to -ERESTARTSYS.  This in turn will cause
> the signal handling code in the kernel to recognize -ERESTARTSYS
> which will in turn cause the syscall to be restarted.
> 
> This commit is based on work by Jan Kratochvil from 2009:
> 
> https://sourceware.org/ml/gdb-patches/2009-11/msg00592.html
> 
> Jan's patch had the sign extension code in amd64-nat.c.  Several
> other native targets make use of this code, so it seemed better
> to move the sign extension code to a linux specific file.  I
> also added similar code to gdbserver.
> 
> Another approach is to fix the problem in the kernel.  Hui Zhu
> tried to get a fix into the kernel back in 2014, but it was not
> accepted.  Discussion regarding this approach may be found here:
> 
> https://lore.kernel.org/patchwork/patch/457841/
> 

In that discussion, I proposed a different kernel fix, and 
H. Peter Anvin kind of agreed with it (said "This seems a lot saner"),
but no one ever submitted such a patch, I believe.

> Even if a fix were to be put into the kernel, we'd still need
> some kind of fix in GDB in order to support older kernels.

That's reasonable.

> 
> Finally, I'll note that Fedora has been carrying a similar patch for
> at least nine years.  Other distributions, including RHEL and CentOS
> have picked up this change and have been using it too.
> 
> gdb/ChangeLog:
> 
> 	* amd64-linux-nat.c (amd64_linux_collect_native_gregset): New
> 	function.
> 	(fill_gregset): Call amd64_linux_collect_native_gregset instead
> 	of amd64_collect_native_gregset.
> 	(amd64_linux_nat_target::store_registers): Likewise.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* linux-x86-low.c (x86_fill_gregset): Sign extend EAX value
> 	when using a 64-bit gdbserver.
> ---
>  gdb/amd64-linux-nat.c         | 32 ++++++++++++++++++++++++++++++--
>  gdb/gdbserver/linux-x86-low.c |  9 +++++++++
>  2 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
> index a0bb105f5a..06018c8f1c 100644
> --- a/gdb/amd64-linux-nat.c
> +++ b/gdb/amd64-linux-nat.c
> @@ -92,6 +92,34 @@ static int amd64_linux_gregset32_reg_offset[] =
>  /* Transfering the general-purpose registers between GDB, inferiors
>     and core files.  */
>  
> +/* See amd64_collect_native_gregset.  This linux specific version handles
> +   issues with negative EAX values not being restored correctly upon syscall
> +   return when debugging 32-bit targets.  It has no effect on 64-bit
> +   targets.  */
> +
> +static void
> +amd64_linux_collect_native_gregset (const struct regcache *regcache,
> +			            void *gregs, int regnum)
> +{
> +  amd64_collect_native_gregset (regcache, gregs, regnum);
> +
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32)
> +    {
> +      /* Sign-extend %eax as during return from a syscall it is being checked
> +	 for -ERESTART* values -512 being above 0xfffffffffffffe00; tested by
> +	 interrupt.exp.	 */> +
> +      if (regnum == -1 || regnum == I386_EAX_REGNUM)
> +	{
> +	  void *ptr = (gdb_byte *) gregs 
> +		    + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM];

Need to wrap the expression that spawns two lines in ()s.

> +
> +	  *(int64_t *) ptr = *(int32_t *) ptr;
> +	}
> +    }
> +}
> +
>  /* Fill GDB's register cache with the general-purpose register values
>     in *GREGSETP.  */
>  
> @@ -109,7 +137,7 @@ void
>  fill_gregset (const struct regcache *regcache,
>  	      elf_gregset_t *gregsetp, int regnum)
>  {
> -  amd64_collect_native_gregset (regcache, gregsetp, regnum);
> +  amd64_linux_collect_native_gregset (regcache, gregsetp, regnum);
>  }
>  
>  /* Transfering floating-point registers between GDB, inferiors and cores.  */
> @@ -237,7 +265,7 @@ amd64_linux_nat_target::store_registers (struct regcache *regcache, int regnum)
>        if (ptrace (PTRACE_GETREGS, tid, 0, (long) &regs) < 0)
>  	perror_with_name (_("Couldn't get registers"));
>  
> -      amd64_collect_native_gregset (regcache, &regs, regnum);
> +      amd64_linux_collect_native_gregset (regcache, &regs, regnum);
>  
>        if (ptrace (PTRACE_SETREGS, tid, 0, (long) &regs) < 0)
>  	perror_with_name (_("Couldn't write registers"));
> diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
> index 029796e361..20f369c496 100644
> --- a/gdb/gdbserver/linux-x86-low.c
> +++ b/gdb/gdbserver/linux-x86-low.c
> @@ -338,6 +338,15 @@ x86_fill_gregset (struct regcache *regcache, void *buf)
>  
>    collect_register_by_name (regcache, "orig_eax",
>  			    ((char *) buf) + ORIG_EAX * REGSIZE);
> +
> +  /* Sign extend EAX value to avoid potential syscall restart problems.  */

I'd rather see both implementations use the same comment.  Could you
merge them?

> +  if (register_size (regcache->tdesc, 0) == 4)
> +    {
> +      void *ptr = (gdb_byte *) buf
> +                + i386_regmap[find_regno (regcache->tdesc, "eax")];

Ditto wrt parens.

> +
> +      *(int64_t *) ptr = *(int32_t *) ptr;
> +    }
>  }
>  
>  static void
> 

Otherwise OK.

Thanks,
Pedro Alves