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: Pedro Alves <palves at redhat dot com>
- To: Kevin Buettner <kevinb at redhat dot com>, gdb-patches at sourceware dot org
- Date: Tue, 9 Apr 2019 18:46:45 +0100
- Subject: Re: [PATCH] Fix amd64->i386 linux syscall restart problem
- References: <20190316221341.021f7c62@f29-4.lan>
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) ®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. */
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