From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8949 invoked by alias); 9 Apr 2019 17:46:52 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 8935 invoked by uid 89); 9 Apr 2019 17:46:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=gdb's, gdbs X-HELO: mail-wr1-f68.google.com Received: from mail-wr1-f68.google.com (HELO mail-wr1-f68.google.com) (209.85.221.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 09 Apr 2019 17:46:49 +0000 Received: by mail-wr1-f68.google.com with SMTP id r4so22011120wrq.8 for ; Tue, 09 Apr 2019 10:46:48 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id b11sm35641663wru.61.2019.04.09.10.46.45 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 09 Apr 2019 10:46:46 -0700 (PDT) Subject: Re: [PATCH] Fix amd64->i386 linux syscall restart problem To: Kevin Buettner , gdb-patches@sourceware.org References: <20190316221341.021f7c62@f29-4.lan> From: Pedro Alves Message-ID: <975e7120-1b61-2807-5aab-b961e07a80d0@redhat.com> Date: Tue, 09 Apr 2019 17:46:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190316221341.021f7c62@f29-4.lan> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-04/txt/msg00150.txt.bz2 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