This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support
- From: Pedro Alves <palves at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: Yao Qi <yao at codesourcery dot com>, GDB Patches <gdb-patches at sourceware dot org>
- Date: Mon, 13 Jan 2014 16:12:49 +0000
- Subject: Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support
- Authentication-results: sourceware.org; auth=none
- References: <m3mwj4onqn dot fsf at redhat dot com> <52CF4B40 dot 3030500 at codesourcery dot com> <52CF57CF dot 9030503 at codesourcery dot com> <m38uuoo5do dot fsf at redhat dot com> <52CFD221 dot 3050100 at redhat dot com> <m38uunn3qn dot fsf at redhat dot com> <52D04291 dot 2000901 at redhat dot com> <m31u0fmuxv dot fsf at redhat dot com>
On 01/10/2014 08:41 PM, Sergio Durigan Junior wrote:
> On Friday, January 10 2014, Pedro Alves wrote:
>
>> Hmm, OK, I think I see. I don't think the register block
>> size in the header needs to be correct for this test -- tfile.c
>> portability sounds like a red herring to me. (I don't think 500
>> is correct for x86/x86_64 either?) There's no register block
>> in the trace file anyway. Only memory blocks.
>>
>> tfile knows to infer the PC from the tracepoint's address if
>> the PC wasn't collected (tfile_fetch_registers) but,
>> (guessing) the issue is that PC register in s390 is a
>> pseudo-register. tfile_fetch_registers guesses the PC from the
>> tracepoint's address and stores it in the regcache, but
>> when reading a (non-readonly) pseudo register from a regcache,
>> we never use the stored value in the cache -- we always
>> recompute it. In fact, writing the pseudo PC to the regcache
>> in tfile_fetch_registers with regcache_raw_supply seems reachable
>> when regnum==-1, and I'm surprised this isn't triggering the assertion
>> that makes sure the regnum being written to is a raw register
>> (as the regcache's buffer only holds the raw registers -- see
>> regcache_xmalloc_1 and regcache_raw_write).
>
> Thanks for the investigation.
>
> 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.
> The pseudo PC register number on s390x is 90, and "gdbarch_num_regs
> (gdbarch)" returns exactly 90.
>
> What happens is that when one requests the pseudo PC on s390x, it reads
> S390_PSWM_REGNUM, which is 1:
...
> Because regno == 1 and pc_regno == 90. Maybe you knew all that, but I
> thought it would be better to explicitly mention.
Right.
>> I.e., this is not a bug specific to s390, but to the
>> tracing/unavailable machinery itself, for all targets
>> whose PC is a pseudo register.
>
> I see. I haven't had the chance to test on other targets where PC is a
> pseud register.
>
>> 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.
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="<unavailable>",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 <unavailable> in ?? ()
That is, frame #0 instead of -1.
This is just the minimal necessary for
<unavailable> 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.
---
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