This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix amd64->i386 linux syscall restart problem
- From: Kevin Buettner <kevinb at redhat dot com>
- To: gdb-patches at sourceware dot org
- Date: Thu, 4 Apr 2019 09:51:30 -0700
- Subject: Re: [PATCH] Fix amd64->i386 linux syscall restart problem
- References: <20190316221341.021f7c62@f29-4.lan>
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) ®s) < 0)
> perror_with_name (_("Couldn't get registers"));
>
> - amd64_collect_native_gregset (regcache, ®s, regnum);
> + amd64_linux_collect_native_gregset (regcache, ®s, regnum);
>
> if (ptrace (PTRACE_SETREGS, tid, 0, (long) ®s) < 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
>