From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45683 invoked by alias); 4 Apr 2019 16:51:35 -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 45674 invoked by uid 89); 4 Apr 2019 16:51:35 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=nine X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 04 Apr 2019 16:51:33 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D02E309179A for ; Thu, 4 Apr 2019 16:51:32 +0000 (UTC) Received: from f29-4.lan (ovpn-116-219.phx2.redhat.com [10.3.116.219]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E9C151001E74 for ; Thu, 4 Apr 2019 16:51:31 +0000 (UTC) Date: Thu, 04 Apr 2019 16:51:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix amd64->i386 linux syscall restart problem Message-ID: <20190404095130.345bc9c1@f29-4.lan> In-Reply-To: <20190316221341.021f7c62@f29-4.lan> References: <20190316221341.021f7c62@f29-4.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-04/txt/msg00071.txt.bz2 Ping. On Sat, 16 Mar 2019 22:13:41 -0700 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. 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 >