From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16287 invoked by alias); 15 Jan 2014 20:37: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 16146 invoked by uid 89); 15 Jan 2014 20:37:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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; Wed, 15 Jan 2014 20:37:50 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s0FKbmjO022196 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 15 Jan 2014 15:37:48 -0500 Received: from psique (ovpn-113-91.phx2.redhat.com [10.3.113.91]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s0FKbiSx010075 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 15 Jan 2014 15:37:46 -0500 From: Sergio Durigan Junior To: Pedro Alves Cc: Yao Qi , GDB Patches Subject: Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support References: <52CF4B40.3030500@codesourcery.com> <52CF57CF.9030503@codesourcery.com> <52CFD221.3050100@redhat.com> <52D04291.2000901@redhat.com> <52D41081.7050907@redhat.com> X-URL: http://www.redhat.com Date: Wed, 15 Jan 2014 20:37:00 -0000 In-Reply-To: <52D41081.7050907@redhat.com> (Pedro Alves's message of "Mon, 13 Jan 2014 16:12:49 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-01/txt/msg00562.txt.bz2 On Monday, January 13 2014, Pedro Alves wrote: > On 01/10/2014 08:41 PM, Sergio Durigan Junior wrote: >> By debugging it a little more, I see that we don't write the pseudo PC >> to the regcache. You are probably talking about this loop: >> >> /* We get here if no register data has been found. Mark registers >> as unavailable. */ >> for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) >> regcache_raw_supply (regcache, regn, NULL); > > No. Below that, when regno==-1. Note my patch adds an early > return _after_ that loop. > > /* We get here if no register data has been found. Mark registers > as unavailable. */ > for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++) > regcache_raw_supply (regcache, regn, NULL); > > /* We can often usefully guess that the PC is going to be the same > as the address of the tracepoint. */ > pc_regno = gdbarch_pc_regnum (gdbarch); > if (pc_regno >= 0 && (regno == -1 || regno == pc_regno)) > ^^^^^^^^^^^ > { > struct tracepoint *tp = get_tracepoint (tracepoint_number); > > if (tp && tp->base.loc) > { > /* But don't try to guess if tracepoint is multi-location... */ > if (tp->base.loc->next) > { > warning (_("Tracepoint %d has multiple " > "locations, cannot infer $pc"), > tp->base.number); > return; > } > /* ... or does while-stepping. */ > if (tp->step_count > 0) > { > warning (_("Tracepoint %d does while-stepping, " > "cannot infer $pc"), > tp->base.number); > return; > } > > store_unsigned_integer (regs, register_size (gdbarch, pc_regno), > gdbarch_byte_order (gdbarch), > tp->base.loc->address); > regcache_raw_supply (regcache, pc_regno, regs); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > } > } > } > > > regno==-1 means "retrieve all raw registers". It just > seems like no such call happens to trigger in this case. OK. >>> It's fine with me to skip the test on targets with pseudo >>> register PCs for now, though probably we should record this >>> info somewhere, probably in the code, like in the patch below. >> >> Nice, I like the comments, though your patch doesn't really change >> anything for s390x because of what I explained above (regno == 1 and >> pc_regno == 90), but I like to make things more explicit and your patch >> does that. >> >> I can push both our patches if you wish, btw. > > I've pushed mine in now. I'd really prefer to avoid > hardcoding knowledge of which targets support tracing > in the testsuite. Exposing the host-side bits to > all targets helps identify design bugs, just like > this very case of pseudo-register PCs. OK, makes sense. > But even if GDB doesn't know how to infer the value > of PC, saying the current frame is level -1 is a bug: > > ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="",func="??",args=[]}^M > ^^^^^^^^^ > > that's of course, the sentinel frame peeking through. > This is caused by the s390's heuristic unwinder accepting the > frame (the fallback heuristic unwinders _always_ accept the > frame), but then unwind->this_id method throws that > "PC not available\n" error. > IOW, the issue is in the target's unwinding mechanism not > having been adjusted to handle unavailable register > values gracefully (which can happen with e.g., a trimmed > core file too). Could you please try the patch below? > You should see something like: > > (gdb) tfind > Found trace frame 0, tracepoint 1 > #0 in ?? () > > That is, frame #0 instead of -1. > > This is just the minimal necessary for > frames. We could get better info out > of "info frame" (this will show "outermost"), but > this would still be necessary. I believe mi-traceframe-changed.exp > should pass with this patch. Yes, confirming that it passes with the patch. Sorry for taking long to test, I had issues trying to get another s390x machine to test it. Thanks for the patch and the further investigation. > --- > gdb/s390-linux-tdep.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c > index b75c86a..8b71e78 100644 > --- a/gdb/s390-linux-tdep.c > +++ b/gdb/s390-linux-tdep.c > @@ -2039,7 +2039,9 @@ static struct s390_unwind_cache * > s390_frame_unwind_cache (struct frame_info *this_frame, > void **this_prologue_cache) > { > + volatile struct gdb_exception ex; > struct s390_unwind_cache *info; > + > if (*this_prologue_cache) > return *this_prologue_cache; > > @@ -2050,10 +2052,15 @@ s390_frame_unwind_cache (struct frame_info *this_frame, > info->frame_base = -1; > info->local_base = -1; > > - /* Try to use prologue analysis to fill the unwind cache. > - If this fails, fall back to reading the stack backchain. */ > - if (!s390_prologue_frame_unwind_cache (this_frame, info)) > - s390_backchain_frame_unwind_cache (this_frame, info); > + TRY_CATCH (ex, RETURN_MASK_ERROR) > + { > + /* Try to use prologue analysis to fill the unwind cache. > + If this fails, fall back to reading the stack backchain. */ > + if (!s390_prologue_frame_unwind_cache (this_frame, info)) > + s390_backchain_frame_unwind_cache (this_frame, info); > + } > + if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR) > + throw_exception (ex); > > return info; > } > -- > 1.7.11.7 -- Sergio