[PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Sergio Durigan Junior]

Hi,

This patch is related to one of the "regressions" that I reported on the
GDB 7.7 testing thread.  While investigating the issue, I found that
gdb.trace/mi-traceframe-changed.exp was running without actually
checking if the target supported tracing or not.  So I wrote this patch
to fix the issue.

It is now reporting the test as unsupported on s390x when run without
gdbserver.  When I run it with gdbserver, yet another error happens that
prevents the testcase to test if the target supports tracing, but it is
unrelated to the "regression" and happens with other testcases as well.

OK to apply?

-- 
Sergio

gdb/testsuite
2014-01-09  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.trace/mi-traceframe-changed.exp: Test if the target supports
	tracing before running the test.  Use prepare_for_testing instead
	of gdb_compile.

diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
index 4bcf379..b5380c5 100644
--- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
@@ -14,8 +14,6 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 load_lib trace-support.exp
-load_lib mi-support.exp
-set MIFLAGS "-i=mi"
 
 standard_testfile tfile.c
 set executable $testfile
@@ -30,15 +28,30 @@ if {![is_remote host] && ![is_remote target]} {
     set purely_local 0
 }
 
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
-	   executable \
-	   [list debug nowarnings \
-		"additional_flags=-DTFILE_DIR=\"$tfile_dir\""]] \
-	  != "" } {
-     untested ${testfile}.exp
-     return -1
+if { [prepare_for_testing $testfile.exp $testfile $srcfile \
+    [list debug nowarnings additional_flags=-DTFILE_DIR="${tfile_dir}"]] } {
+    untested $testfile.exp
+    return -1
 }
 
+# Testing if the target supports tracing.
+if { ![runto_main] } {
+    fail "Cannot test if target supports tracing"
+    return -1
+}
+
+if { ![gdb_target_supports_trace] } {
+    unsupported "Current target does not support trace"
+    return -1
+}
+
+gdb_exit
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+mi_gdb_start
+
 # Make sure we are starting fresh.
 remote_file host delete $tfile_basic
 remote_file target delete $tfile_basic

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Yao Qi]

On 01/10/2014 05:21 AM, Sergio Durigan Junior wrote:
> gdb.trace/mi-traceframe-changed.exp was running without actually
> checking if the target supported tracing or not.  So I wrote this patch
> to fix the issue.

The patch looks a right fix.  Any tracepoint related tests should check
whether target supports tracing or not at first.

I did it through an oversight when I wrote this case.

-- 
Yao (齐尧)

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Yao Qi]

On 01/10/2014 09:22 AM, Yao Qi wrote:
> On 01/10/2014 05:21 AM, Sergio Durigan Junior wrote:
>> > gdb.trace/mi-traceframe-changed.exp was running without actually
>> > checking if the target supported tracing or not.  So I wrote this patch
>> > to fix the issue.
> The patch looks a right fix.  Any tracepoint related tests should check
> whether target supports tracing or not at first.
> 
> I did it through an oversight when I wrote this case.

Ah, I read the patch and mi-traceframe-change.exp again, and find my
last comment is wrong.  Sorry for the confusion.

The first half of mi-traceframe-changed.exp (test_tfind_tfile) is to
test "=traceframe-changed" on tfile target, which is produced by
tfile.c.  It is expected to run on native debugging.  The second half
of mi-traceframe-changed.exp (test_tfile_remote) is to test
"=traceframe-changed" on remote target with a gdbserver connected.  We
can see mi-traceframe-changed.exp has already have the code to check
target supports tracing or not.

The root cause is that tfile.c isn't portable and unable to produce
trace file properly for s390x.  Search FIXME in it.

We should skip test_find_tfile for targets other than x86-linux or
x86_64-linux.  Alternatively, we can modify tfile.c for s390x, but I
think "generating tfile on a unsupported-tracing target" isn't useful.

-- 
Yao (齐尧)

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Sergio Durigan Junior]

On Friday, January 10 2014, Yao Qi wrote:

> Ah, I read the patch and mi-traceframe-change.exp again, and find my
> last comment is wrong.  Sorry for the confusion.

Thank you!  I was secretly wondering whether my patch was correct or
not, because I thought the testcase was intended to be run partially on
native debugging, as you explained.  I was going to e-mail something
about it tomorrow, but you were faster :-).

> The first half of mi-traceframe-changed.exp (test_tfind_tfile) is to
> test "=traceframe-changed" on tfile target, which is produced by
> tfile.c.  It is expected to run on native debugging.  The second half
> of mi-traceframe-changed.exp (test_tfile_remote) is to test
> "=traceframe-changed" on remote target with a gdbserver connected.  We
> can see mi-traceframe-changed.exp has already have the code to check
> target supports tracing or not.
>
> The root cause is that tfile.c isn't portable and unable to produce
> trace file properly for s390x.  Search FIXME in it.

Indeed, thanks for pointing that.

> We should skip test_find_tfile for targets other than x86-linux or
> x86_64-linux.  Alternatively, we can modify tfile.c for s390x, but I
> think "generating tfile on a unsupported-tracing target" isn't useful.

OK, WDYT of this version then?

-- 
Sergio

2014-01-10  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.trace/mi-traceframe-changed.exp: Only run test_find_tfile
	if the target is x86_64 or i*86 running the Linux kernel.

diff --git a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
index 4bcf379..f2f5224 100644
--- a/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
+++ b/gdb/testsuite/gdb.trace/mi-traceframe-changed.exp
@@ -88,7 +88,11 @@ proc test_tfind_tfile { } {
     }
 }
 
-test_tfind_tfile
+if { [istarget "x86_64-*-linux*"] || [istarget "i*86-*-linux*"] } {
+    # This test only works on x86_64 and i*86 targets running the Linux
+    # kernel.  See the FIXME's on gdb.trace/tfile.c for more details.
+    test_tfind_tfile
+}
 
 # Change to a different test case in order to run it on target, and get
 # several traceframes.

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Pedro Alves]

On 01/10/2014 03:57 AM, Sergio Durigan Junior wrote:
> On Friday, January 10 2014, Yao Qi wrote:
> 
>> Ah, I read the patch and mi-traceframe-change.exp again, and find my
>> last comment is wrong.  Sorry for the confusion.
> 
> Thank you!  I was secretly wondering whether my patch was correct or
> not, because I thought the testcase was intended to be run partially on
> native debugging, as you explained.  I was going to e-mail something
> about it tomorrow, but you were faster :-).
> 
>> The first half of mi-traceframe-changed.exp (test_tfind_tfile) is to
>> test "=traceframe-changed" on tfile target, which is produced by
>> tfile.c.  It is expected to run on native debugging.  The second half
>> of mi-traceframe-changed.exp (test_tfile_remote) is to test
>> "=traceframe-changed" on remote target with a gdbserver connected.  We
>> can see mi-traceframe-changed.exp has already have the code to check
>> target supports tracing or not.
>>
>> The root cause is that tfile.c isn't portable and unable to produce
>> trace file properly for s390x.  Search FIXME in it.
> 
> Indeed, thanks for pointing that.

Is it the register block size?  What's the actual error that
causes / you're seeing?

>> We should skip test_find_tfile for targets other than x86-linux or
>> x86_64-linux.  Alternatively, we can modify tfile.c for s390x, but I
>> think "generating tfile on a unsupported-tracing target" isn't useful.
> 
> OK, WDYT of this version then?
-- 
Pedro Alves

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Sergio Durigan Junior]

On Friday, January 10 2014, Pedro Alves wrote:

>>> The root cause is that tfile.c isn't portable and unable to produce
>>> trace file properly for s390x.  Search FIXME in it.
>> 
>> Indeed, thanks for pointing that.
>
> Is it the register block size?  What's the actual error that
> causes / you're seeing?

The error is:

-trace-find frame-number 0^M
&"PC not available\n"^M
^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M
(gdb) ^M
FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: -trace-find frame-number 0

There is also another error before, but it was already failing for
7.6.2.

-- 
Sergio

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Pedro Alves]

On 01/10/2014 05:30 PM, Sergio Durigan Junior wrote:
> On Friday, January 10 2014, Pedro Alves wrote:
> 
>>>> The root cause is that tfile.c isn't portable and unable to produce
>>>> trace file properly for s390x.  Search FIXME in it.
>>>
>>> Indeed, thanks for pointing that.
>>
>> Is it the register block size?  What's the actual error that
>> causes / you're seeing?
> 
> The error is:
> 
> -trace-find frame-number 0^M
> &"PC not available\n"^M
> ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M
> (gdb) ^M
> FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: -trace-find frame-number 0

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).

When debugging a live traceframe, i.e., with gdbserver,
it's gdbserver itself that does this same PC guessing.  And
of course, that also only works when the PC isn't a pseudo
register, as the target backend only ever sees raw registers
accesses.

Seems like this PC guessing should move out of
target_fetch_registers to somewhere higher level...

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.

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.

> There is also another error before, but it was already failing for
> 7.6.2.

Hard to say anything about it if you don't show it.  :-)

-------
Subject: [PATCH] tfile: Don't infer the PC from the tracepoint if the PC is a
 pseudo-register.

This PC guessing can't work when the PC is a pseudo-register.
Pseudo-register values don't end up stored in the regcache, they're
always recomputed.  And, it's actually wrong to try to write a
pseudo-register with regcache_raw_supply.  Skip it and add a comment.

gdb/
2014-01-10  Pedro Alves  <palves@redhat.com>

	* tracepoint.c (tfile_fetch_registers): Don't infer the PC from
	the tracepoint if the PC is a pseudo-register.
---
 gdb/tracepoint.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 582c284..4b47282 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -5060,7 +5060,17 @@ tfile_fetch_registers (struct target_ops *ops,
   /* 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))
+
+  /* XXX This guessing code below only works if the PC register isn't
+     a pseudo-register.  The value of a pseudo-register isn't stored
+     in the (non-readonly) regcache -- instead it's recomputed
+     (probably from some other cached raw register) whenever the
+     register is read.  This guesswork should probably move to some
+     higher layer.  */
+  if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch))
+    return;
+
+  if (regno == -1 || regno == pc_regno)
     {
       struct tracepoint *tp = get_tracepoint (tracepoint_number);

-- 
1.7.11.7

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Sergio Durigan Junior]

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);

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:

  static enum register_status
  s390_pseudo_register_read (struct gdbarch *gdbarch, struct regcache *regcache,
                             int regnum, gdb_byte *buf)
  {
    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
    int regsize = register_size (gdbarch, regnum);
    ULONGEST val;

    if (regnum == tdep->pc_regnum)
      {
        enum register_status status;

        status = regcache_raw_read_unsigned (regcache, S390_PSWA_REGNUM, &val);

Then, this check on tfile_fetch_registers evaluates to false:

  /* 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))

Because regno == 1 and pc_regno == 90.  Maybe you knew all that, but I
thought it would be better to explicitly mention.

> When debugging a live traceframe, i.e., with gdbserver,
> it's gdbserver itself that does this same PC guessing.  And
> of course, that also only works when the PC isn't a pseudo
> register, as the target backend only ever sees raw registers
> accesses.
>
> Seems like this PC guessing should move out of
> target_fetch_registers to somewhere higher level...
>
> 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.

>> There is also another error before, but it was already failing for
>> 7.6.2.
>
> Hard to say anything about it if you don't show it.  :-)

The same error:

&"tfind 0\n"^M
&"PC not available\n"^M
~"Found trace frame 0, tracepoint 1\n"^M
~"#-1 <unavailable> in ?? ()\n"^M
^done^M
(gdb) ^M
FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: tfind 0 again

So I think the same explanation is valid.

> -------
> Subject: [PATCH] tfile: Don't infer the PC from the tracepoint if the PC is a
>  pseudo-register.
>
> This PC guessing can't work when the PC is a pseudo-register.
> Pseudo-register values don't end up stored in the regcache, they're
> always recomputed.  And, it's actually wrong to try to write a
> pseudo-register with regcache_raw_supply.  Skip it and add a comment.
>
> gdb/
> 2014-01-10  Pedro Alves  <palves@redhat.com>
>
> 	* tracepoint.c (tfile_fetch_registers): Don't infer the PC from
> 	the tracepoint if the PC is a pseudo-register.
> ---
>  gdb/tracepoint.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 582c284..4b47282 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -5060,7 +5060,17 @@ tfile_fetch_registers (struct target_ops *ops,
>    /* 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))
> +
> +  /* XXX This guessing code below only works if the PC register isn't
> +     a pseudo-register.  The value of a pseudo-register isn't stored
> +     in the (non-readonly) regcache -- instead it's recomputed
> +     (probably from some other cached raw register) whenever the
> +     register is read.  This guesswork should probably move to some
> +     higher layer.  */
> +  if (pc_regno < 0 || pc_regno >= gdbarch_num_regs (gdbarch))
> +    return;
> +
> +  if (regno == -1 || regno == pc_regno)
>      {
>        struct tracepoint *tp = get_tracepoint (tracepoint_number);
>
> -- 
> 1.7.11.7

-- 
Sergio

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Pedro Alves]

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

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Sergio Durigan Junior]

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="<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.

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

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support

[Pedro Alves]

On 01/15/2014 08:37 PM, Sergio Durigan Junior wrote:

> 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.

Thank you.  Patch is now pushed, as below.

------
Fix gdb.trace/mi-traceframe-changed.exp on s390.

The test fails on s390 with:

  -trace-find frame-number 0^M
  &"PC not available\n"^M
  ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M
  (gdb) ^M
  FAIL: gdb.trace/mi-traceframe-changed.exp: tfile: -trace-find frame-number 0

tfile knows to infer the PC from the tracepoint's address if the PC
wasn't collected (tfile_fetch_registers) but, that only works on
targets whose PC register is a raw register, and on s390, the PC
register is a pseudo register.

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
                                                       ^^^^^^^^^

'-1' is the level of the sentinel frame, which should never be visible.

This is caused by the s390's heuristic unwinder accepting the frame
(the fallback heuristic unwinders _always_ accept the frame), but then
the unwind->this_id method throws that "PC not available\n" error.

IOW, the s390's heuristic unwinder was never adjusted to handle
unavailable register values gracefully, which can happen with e.g., a
trimmed core file too.

This is just the minimal necessary for
<unavailable> frames, which at least gets us:

  (gdb) tfind
  Found trace frame 0, tracepoint 1
  #0  <unavailable> in ?? ()

That is, frame #0 instead of -1.

We could get better info out of "info frame" (this patch makes us show
"outermost"), but this change would still be necessary.

gdb/
2014-01-16  Pedro Alves  <palves@redhat.com>

	* s390-linux-tdep.c (s390_frame_unwind_cache): Swallow
	NOT_AVAILABLE_ERROR errors while parsing the prologue or reading
	the backchain.
---
 gdb/ChangeLog         |  6 ++++++
 gdb/s390-linux-tdep.c | 15 +++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f614f1b..6eef81a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-01-16  Pedro Alves  <palves@redhat.com>
+
+	* s390-linux-tdep.c (s390_frame_unwind_cache): Swallow
+	NOT_AVAILABLE_ERROR errors while parsing the prologue or reading
+	the backchain.
+
 2014-01-16  Doug Evans  <dje@google.com>
 
 	* dwarf2read.c (open_and_init_dwp_file): Fix typo in comment.
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