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

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

[Kevin Buettner]

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.  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/

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.

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];
+
+	  *(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.  */
+  if (register_size (regcache->tdesc, 0) == 4)
+    {
+      void *ptr = (gdb_byte *) buf
+                + i386_regmap[find_regno (regcache->tdesc, "eax")];
+
+      *(int64_t *) ptr = *(int32_t *) ptr;
+    }
 }
 
 static void

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

[Kevin Buettner]

Ping.

On Sat, 16 Mar 2019 22:13:41 -0700
Kevin Buettner <kevinb@redhat.com> 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.  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/
> 
> 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.
> 
> 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];
> +
> +	  *(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.  */
> +  if (register_size (regcache->tdesc, 0) == 4)
> +    {
> +      void *ptr = (gdb_byte *) buf
> +                + i386_regmap[find_regno (regcache->tdesc, "eax")];
> +
> +      *(int64_t *) ptr = *(int32_t *) ptr;
> +    }
>  }
>  
>  static void
> 

Re: [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

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

[Kevin Buettner]

On Tue, 9 Apr 2019 18:46:45 +0100
Pedro Alves <palves@redhat.com> wrote:

> > +	  void *ptr = (gdb_byte *) gregs 
> > +		    + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM];  
> 
> Need to wrap the expression that spawns two lines in ()s.

I've made this change.  And the other one too.

> > +
> > +  /* Sign extend EAX value to avoid potential syscall restart problems.  */  
> 
> I'd rather see both implementations use the same comment.  Could you
> merge them?

I started to merge them and then decided to write a more detailed
comment based on the text that I wrote for the commit comment.  I
have, for the moment anyway, copied the comment to both locations with
only slight changes which reflect where the comment is located.  The
problem with having copies of the same long comment in two or more
places is making sure that if one gets updated, then the rest do too. 
It might be better to have one refer to the other.

I'm thinking that it might be preferable to have something like this
in gdb/gdbserver/linux-x86-low.c:

  /* Sign extend EAX value to avoid potential syscall restart problems.
  
     See amd64_linux_collect_native_gregset() in gdb/amd64-linux-nat.c
     for a detailed explanation.  */

Below is a diff showing the new comments.  It also includes the
changes which wrap the multi-line expressions in parens.

diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 06018c8f1c..8d0e8eb35c 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -106,14 +106,51 @@ amd64_linux_collect_native_gregset (const struct regcache *regcache,
   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.	 */
+      /* Sign extend EAX value to avoid potential syscall restart
+	 problems.  
+
+	 On Linux, when a syscall is interrupted by a signal, the
+	 (kernel function implementing 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 it can cause the syscall to be restarted.  We are
+	 concerned with the latter case here.
+	 
+	 On (32-bit) i386, the status (-ERESTARTSYS) is placed in the
+	 EAX register.  When debugging a 32-bit process from a 64-bit
+	 (amd64) GDB, the debugger fetches 64-bit registers even
+	 though the process being debugged is only 32-bit.  The
+	 register cache is only 32 bits wide though; GDB discards the
+	 high 32 bits when placing 64-bit values in the 32-bit
+	 regcache.  Normally, this is not a problem since the 32-bit
+	 process should only care about the lower 32-bit portions of
+	 these registers.  That said, it can happen that the 64-bit
+	 value being restored will be different from the 64-bit value
+	 that was originally retrieved from the kernel.  The one place
+	 (that we know of) where it does matter is in the kernel's
+	 syscall restart code.  The kernel's code for restarting a
+	 syscall after a signal expects to see a negative value
+	 (specifically -ERESTARTSYS) in the 64-bit RAX register in
+	 order to correctly cause a syscall to be restarted.
+	 
+	 The call to amd64_collect_native_gregset, above, is setting
+	 the high 32 bits of RAX (and other registers too) to 0.  For
+	 syscall restart, 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.
+
+	 The test case gdb.base/interrupt.exp tests for this problem.
+	 Without this sign extension code in place, it'll show
+	 a number of failures when testing against unix/-m32.  */
 
       if (regnum == -1 || regnum == I386_EAX_REGNUM)
 	{
-	  void *ptr = (gdb_byte *) gregs 
-		    + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM];
+	  void *ptr = ((gdb_byte *) gregs 
+		       + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM]);
 
 	  *(int64_t *) ptr = *(int32_t *) ptr;
 	}
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 20f369c496..9a97cdf5c0 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -339,11 +339,48 @@ 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.  */
+  /* Sign extend EAX value to avoid potential syscall restart
+     problems. 
+
+     On Linux, when a syscall is interrupted by a signal, the (kernel
+     function implementing 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 it can cause the syscall
+     to be restarted.  We are concerned with the latter case here.
+     
+     On (32-bit) i386, the status (-ERESTARTSYS) is placed in the EAX
+     register.  When debugging a 32-bit process from a 64-bit (amd64)
+     GDB, the debugger fetches 64-bit registers even though the
+     process being debugged is only 32-bit.  The register cache is
+     only 32 bits wide though; GDB discards the high 32 bits when
+     placing 64-bit values in the 32-bit regcache.  Normally, this is
+     not a problem since the 32-bit process should only care about the
+     lower 32-bit portions of these registers.  That said, it can
+     happen that the 64-bit value being restored will be different
+     from the 64-bit value that was originally retrieved from the
+     kernel.  The one place (that we know of) where it does matter is
+     in the kernel's syscall restart code.  The kernel's code for
+     restarting a syscall after a signal expects to see a negative
+     value (specifically -ERESTARTSYS) in the 64-bit RAX register in
+     order to correctly cause a syscall to be restarted.
+     
+     The call to collect_register, above, is setting the high 32 bits
+     of RAX (and other registers too) to 0.  For syscall restart, 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.
+
+     The test case gdb.base/interrupt.exp tests for this problem. 
+     Without this sign extension code in place, it'll show a number of
+     failures when testing against native-gdbserver/-m32.  */
+
   if (register_size (regcache->tdesc, 0) == 4)
     {
-      void *ptr = (gdb_byte *) buf
-                + i386_regmap[find_regno (regcache->tdesc, "eax")];
+      void *ptr = ((gdb_byte *) buf
+                   + i386_regmap[find_regno (regcache->tdesc, "eax")]);
 
       *(int64_t *) ptr = *(int32_t *) ptr;
     }

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

[Pedro Alves]

On 4/10/19 3:29 AM, Kevin Buettner wrote:
> On Tue, 9 Apr 2019 18:46:45 +0100
> Pedro Alves <palves@redhat.com> wrote:
> 
>>> +	  void *ptr = (gdb_byte *) gregs 
>>> +		    + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM];  
>>
>> Need to wrap the expression that spawns two lines in ()s.
> 
> I've made this change.  And the other one too.
> 
>>> +
>>> +  /* Sign extend EAX value to avoid potential syscall restart problems.  */  
>>
>> I'd rather see both implementations use the same comment.  Could you
>> merge them?
> 
> I started to merge them and then decided to write a more detailed
> comment based on the text that I wrote for the commit comment.  I
> have, for the moment anyway, copied the comment to both locations with
> only slight changes which reflect where the comment is located.  The
> problem with having copies of the same long comment in two or more
> places is making sure that if one gets updated, then the rest do too. 
> It might be better to have one refer to the other.
> 
> I'm thinking that it might be preferable to have something like this
> in gdb/gdbserver/linux-x86-low.c:
> 
>   /* Sign extend EAX value to avoid potential syscall restart problems.
>   
>      See amd64_linux_collect_native_gregset() in gdb/amd64-linux-nat.c
>      for a detailed explanation.  */

I'm OK with that.  My concern with different comments in two places
is that at some point, like we managed to merge the x86
watchpoints code from gdb and gdbserver under gdb/nat/, we might be
able to merge this code as well.  And different comments make the job
a bit harder for the person doing that, since the person _then_ has to
decide what the resulting comment will be like.  If we can do it now,
it's preferable.  A pointer from one end to the other, like you're
proposing, works for me too.

> Below is a diff showing the new comments.  It also includes the
> changes which wrap the multi-line expressions in parens.

Thanks, that new version of the comment looks great.

Pedro Alves

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

[Kevin Buettner]

On Wed, 10 Apr 2019 12:42:30 +0100
Pedro Alves <palves@redhat.com> wrote:

> > Below is a diff showing the new comments.  It also includes the
> > changes which wrap the multi-line expressions in parens.  
> 
> Thanks, that new version of the comment looks great.

It's in now.  Thanks for the review.

Kevin

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

[Tom de Vries]

On 11-04-19 02:16, Kevin Buettner wrote:
> On Wed, 10 Apr 2019 12:42:30 +0100
> Pedro Alves <palves@redhat.com> wrote:
> 
>>> Below is a diff showing the new comments.  It also includes the
>>> changes which wrap the multi-line expressions in parens.  
>>
>> Thanks, that new version of the comment looks great.
> 
> It's in now.  Thanks for the review.

Hi,

the tests fixed by this commit fail on the 8.3 branch (filed as PR24592).

The commit applies cleanly on the 8.3 branch, and make the tests pass.

OK to backport?

Thanks,
- Tom

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

[Tom de Vries]

On 21-05-19 14:59, Tom de Vries wrote:
> On 11-04-19 02:16, Kevin Buettner wrote:
>> On Wed, 10 Apr 2019 12:42:30 +0100
>> Pedro Alves <palves@redhat.com> wrote:
>>
>>>> Below is a diff showing the new comments.  It also includes the
>>>> changes which wrap the multi-line expressions in parens.  
>>>
>>> Thanks, that new version of the comment looks great.
>>
>> It's in now.  Thanks for the review.
> 
> Hi,
> 
> the tests fixed by this commit fail on the 8.3 branch (filed as PR24592).
> 
> The commit applies cleanly on the 8.3 branch, and make the tests pass.
> 
> OK to backport?

Hi,

[ Ping. ]

I'm probably missing some context here. It seems sofar there are no
commits to the 8.3 branch. AFAIU, a respin release 8.3.1 is targeted for
3 months after 8.3, so around 11th of August.

What kind of fixes are acceptable for the respin? Do they have to be
regression fixes? Or are functionality fixes like this one also allowed?
Who's responsibility is it to backport fixes?

Thanks,
- Tom

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

[Kevin Buettner]

On Fri, 21 Jun 2019 08:34:46 +0200
Tom de Vries <tdevries@suse.de> wrote:

> On 21-05-19 14:59, Tom de Vries wrote:
> > On 11-04-19 02:16, Kevin Buettner wrote:  
> >> On Wed, 10 Apr 2019 12:42:30 +0100
> >> Pedro Alves <palves@redhat.com> wrote:
> >>  
> >>>> Below is a diff showing the new comments.  It also includes the
> >>>> changes which wrap the multi-line expressions in parens.    
> >>>
> >>> Thanks, that new version of the comment looks great.  
> >>
> >> It's in now.  Thanks for the review.  
> > 
> > Hi,
> > 
> > the tests fixed by this commit fail on the 8.3 branch (filed as PR24592).
> > 
> > The commit applies cleanly on the 8.3 branch, and make the tests pass.
> > 
> > OK to backport?  
> 
> Hi,
> 
> [ Ping. ]
> 
> I'm probably missing some context here. It seems sofar there are no
> commits to the 8.3 branch. AFAIU, a respin release 8.3.1 is targeted for
> 3 months after 8.3, so around 11th of August.
> 
> What kind of fixes are acceptable for the respin? Do they have to be
> regression fixes? Or are functionality fixes like this one also allowed?
> Who's responsibility is it to backport fixes?
> 
> Thanks,
> - Tom

I'm okay with it going into the next point release for 8.3.

Kevin

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On 08-07-19 19:00, Kevin Buettner wrote:
> 
> On Fri, 21 Jun 2019 08:34:46 +0200
> Tom de Vries <tdevries@suse.de> wrote:
> 
>> On 21-05-19 14:59, Tom de Vries wrote:
>>> On 11-04-19 02:16, Kevin Buettner wrote:  
>>>> On Wed, 10 Apr 2019 12:42:30 +0100
>>>> Pedro Alves <palves@redhat.com> wrote:
>>>>  
>>>>>> Below is a diff showing the new comments.  It also includes the
>>>>>> changes which wrap the multi-line expressions in parens.    
>>>>>
>>>>> Thanks, that new version of the comment looks great.  
>>>>
>>>> It's in now.  Thanks for the review.  
>>>
>>> Hi,
>>>
>>> the tests fixed by this commit fail on the 8.3 branch (filed as PR24592).
>>>
>>> The commit applies cleanly on the 8.3 branch, and make the tests pass.
>>>
>>> OK to backport?  
>>
>> Hi,
>>
>> [ Ping. ]
>>
>> I'm probably missing some context here. It seems sofar there are no
>> commits to the 8.3 branch. AFAIU, a respin release 8.3.1 is targeted for
>> 3 months after 8.3, so around 11th of August.
>>
>> What kind of fixes are acceptable for the respin? Do they have to be
>> regression fixes? Or are functionality fixes like this one also allowed?
>> Who's responsibility is it to backport fixes?
>>
>> Thanks,
>> - Tom
> 
> I'm okay with it going into the next point release for 8.3.

Hi Kevin,

I noticed the follow-up commit e90a813d96 "Fix regression caused by
recently added syscall restart code". I reproduced the regression with
the 8.3 branch + commit 3f52fdbcb5, and confirmed that the follow-up
commit fixes it, so I suppose this one is necessary as well.

Since this is the first time I'm pushing something to a gdb release
branch, I'm posting here the two pre-commit-formatted patches for review.

Thanks,
- Tom



[-- Attachment #2: 0001-Fix-amd64-i386-linux-syscall-restart-problem.patch --]
[-- Type: text/x-patch, Size: 9845 bytes --]

Fix amd64->i386 linux syscall restart problem

[ Backport of master commit 3f52fdbcb5. ]

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.  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/

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.

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:

	PR gdb/24592
	* 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:

	PR gdb/24592
	* linux-x86-low.c (x86_fill_gregset): Sign extend EAX value
	when using a 64-bit gdbserver.

---
 gdb/ChangeLog                 |  9 ++++++
 gdb/amd64-linux-nat.c         | 69 +++++++++++++++++++++++++++++++++++++++++--
 gdb/gdbserver/ChangeLog       |  6 ++++
 gdb/gdbserver/linux-x86-low.c | 13 ++++++++
 4 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bc8d65abd3..b4781b6114 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2019-04-10  Kevin Buettner  <kevinb@redhat.com>
+
+	PR gdb/24592
+	* 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.
+
 2019-05-11  Joel Brobecker  <brobecker@adacore.com>
 
 	* version.in: Set GDB version number to 8.3.0.DATE-git.
diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index a0bb105f5a..8d0e8eb35c 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -92,6 +92,71 @@ 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 value to avoid potential syscall restart
+	 problems.  
+
+	 On Linux, when a syscall is interrupted by a signal, the
+	 (kernel function implementing 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 it can cause the syscall to be restarted.  We are
+	 concerned with the latter case here.
+	 
+	 On (32-bit) i386, the status (-ERESTARTSYS) is placed in the
+	 EAX register.  When debugging a 32-bit process from a 64-bit
+	 (amd64) GDB, the debugger fetches 64-bit registers even
+	 though the process being debugged is only 32-bit.  The
+	 register cache is only 32 bits wide though; GDB discards the
+	 high 32 bits when placing 64-bit values in the 32-bit
+	 regcache.  Normally, this is not a problem since the 32-bit
+	 process should only care about the lower 32-bit portions of
+	 these registers.  That said, it can happen that the 64-bit
+	 value being restored will be different from the 64-bit value
+	 that was originally retrieved from the kernel.  The one place
+	 (that we know of) where it does matter is in the kernel's
+	 syscall restart code.  The kernel's code for restarting a
+	 syscall after a signal expects to see a negative value
+	 (specifically -ERESTARTSYS) in the 64-bit RAX register in
+	 order to correctly cause a syscall to be restarted.
+	 
+	 The call to amd64_collect_native_gregset, above, is setting
+	 the high 32 bits of RAX (and other registers too) to 0.  For
+	 syscall restart, 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.
+
+	 The test case gdb.base/interrupt.exp tests for this problem.
+	 Without this sign extension code in place, it'll show
+	 a number of failures when testing against unix/-m32.  */
+
+      if (regnum == -1 || regnum == I386_EAX_REGNUM)
+	{
+	  void *ptr = ((gdb_byte *) gregs 
+		       + amd64_linux_gregset32_reg_offset[I386_EAX_REGNUM]);
+
+	  *(int64_t *) ptr = *(int32_t *) ptr;
+	}
+    }
+}
+
 /* Fill GDB's register cache with the general-purpose register values
    in *GREGSETP.  */
 
@@ -109,7 +174,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 +302,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/ChangeLog b/gdb/gdbserver/ChangeLog
index 6648a1ed53..f9f6abc22e 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2019-04-10  Kevin Buettner  <kevinb@redhat.com>
+
+	PR gdb/24592
+	* linux-x86-low.c (x86_fill_gregset): Sign extend EAX value
+	when using a 64-bit gdbserver.
+
 2019-03-04  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	* configure.srv: Use '$enable_unittest' instead of '$development'
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 029796e361..dd7673126e 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -338,6 +338,19 @@ 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. 
+
+     See amd64_linux_collect_native_gregset() in gdb/amd64-linux-nat.c
+     for a detailed explanation.  */
+  if (register_size (regcache->tdesc, 0) == 4)
+    {
+      void *ptr = ((gdb_byte *) buf
+                   + i386_regmap[find_regno (regcache->tdesc, "eax")]);
+
+      *(int64_t *) ptr = *(int32_t *) ptr;
+    }
 }
 
 static void

[-- Attachment #3: 0002-Fix-regression-caused-by-recently-added-syscall-restart-code.patch --]
[-- Type: text/x-patch, Size: 2822 bytes --]

Fix regression caused by recently added syscall restart code

[ Backport of master commit e90a813d96. ]

This line of code...

       *(int64_t *) ptr = *(int32_t *) ptr;

...in linux-x86-low.c is not needed (and does not work correctly)
within a 32-bit executable.  I added an __x86_64__ ifdef (which is
used extensively elsewhere in the file for like purposes) to prevent
this code from being included in 32-bit builds.

It fixes the following regressions when running on native
i686-pc-linux-gnu:

FAIL: gdb.server/abspath.exp: continue to main
FAIL: gdb.server/connect-without-multi-process.exp: multiprocess=auto: continue to main
FAIL: gdb.server/connect-without-multi-process.exp: multiprocess=off: continue to main
FAIL: gdb.server/ext-restart.exp: restart: run to main
FAIL: gdb.server/ext-restart.exp: run to main
FAIL: gdb.server/ext-run.exp: continue to main
FAIL: gdb.server/ext-wrapper.exp: print d
FAIL: gdb.server/ext-wrapper.exp: restart: print d
FAIL: gdb.server/ext-wrapper.exp: restart: run to marker
FAIL: gdb.server/ext-wrapper.exp: run to marker
FAIL: gdb.server/no-thread-db.exp: continue to breakpoint: after tls assignment
FAIL: gdb.server/reconnect-ctrl-c.exp: first: stop with control-c
FAIL: gdb.server/reconnect-ctrl-c.exp: second: stop with control-c
FAIL: gdb.server/run-without-local-binary.exp: run test program until the end
FAIL: gdb.server/server-kill.exp: continue to breakpoint: after server_pid assignment
FAIL: gdb.server/server-kill.exp: tstatus
FAIL: gdb.server/server-run.exp: continue to main

gdb/gdbserver/ChangeLog:

	PR gdb/24592
	* linux-x86-low.c (x86_fill_gregset): Don't compile 64-bit
	sign extension code on 32-bit builds.

---
 gdb/gdbserver/ChangeLog       | 6 ++++++
 gdb/gdbserver/linux-x86-low.c | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index f9f6abc22e..9968804308 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,9 @@
+2019-05-06  Kevin Buettner  <kevinb@redhat.com>
+
+	PR gdb/24592
+	* linux-x86-low.c (x86_fill_gregset): Don't compile 64-bit
+	sign extension code on 32-bit builds.
+
 2019-04-10  Kevin Buettner  <kevinb@redhat.com>
 
 	PR gdb/24592
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index dd7673126e..adddfe7797 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -339,6 +339,7 @@ x86_fill_gregset (struct regcache *regcache, void *buf)
   collect_register_by_name (regcache, "orig_eax",
 			    ((char *) buf) + ORIG_EAX * REGSIZE);
 
+#ifdef __x86_64__
   /* Sign extend EAX value to avoid potential syscall restart
      problems. 
 
@@ -351,6 +352,7 @@ x86_fill_gregset (struct regcache *regcache, void *buf)
 
       *(int64_t *) ptr = *(int32_t *) ptr;
     }
+#endif
 }
 
 static void

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

[Kevin Buettner]

Hi Tom,

Yes, that's right, those two commits are needed.  I've looked it over.  It
looks good to me.

Kevin

On Fri, 12 Jul 2019 13:42:51 +0200
Tom de Vries <tdevries@suse.de> wrote:

> On 08-07-19 19:00, Kevin Buettner wrote:
> > 
> > On Fri, 21 Jun 2019 08:34:46 +0200
> > Tom de Vries <tdevries@suse.de> wrote:
> >   
> >> On 21-05-19 14:59, Tom de Vries wrote:  
> >>> On 11-04-19 02:16, Kevin Buettner wrote:    
> >>>> On Wed, 10 Apr 2019 12:42:30 +0100
> >>>> Pedro Alves <palves@redhat.com> wrote:
> >>>>    
> >>>>>> Below is a diff showing the new comments.  It also includes the
> >>>>>> changes which wrap the multi-line expressions in parens.      
> >>>>>
> >>>>> Thanks, that new version of the comment looks great.    
> >>>>
> >>>> It's in now.  Thanks for the review.    
> >>>
> >>> Hi,
> >>>
> >>> the tests fixed by this commit fail on the 8.3 branch (filed as PR24592).
> >>>
> >>> The commit applies cleanly on the 8.3 branch, and make the tests pass.
> >>>
> >>> OK to backport?    
> >>
> >> Hi,
> >>
> >> [ Ping. ]
> >>
> >> I'm probably missing some context here. It seems sofar there are no
> >> commits to the 8.3 branch. AFAIU, a respin release 8.3.1 is targeted for
> >> 3 months after 8.3, so around 11th of August.
> >>
> >> What kind of fixes are acceptable for the respin? Do they have to be
> >> regression fixes? Or are functionality fixes like this one also allowed?
> >> Who's responsibility is it to backport fixes?
> >>
> >> Thanks,
> >> - Tom  
> > 
> > I'm okay with it going into the next point release for 8.3.  
> 
> Hi Kevin,
> 
> I noticed the follow-up commit e90a813d96 "Fix regression caused by
> recently added syscall restart code". I reproduced the regression with
> the 8.3 branch + commit 3f52fdbcb5, and confirmed that the follow-up
> commit fixes it, so I suppose this one is necessary as well.
> 
> Since this is the first time I'm pushing something to a gdb release
> branch, I'm posting here the two pre-commit-formatted patches for review.
> 
> Thanks,
> - Tom
> 
>