gdb.trace: Move more target dependencies to trace-support.exp

  While groveling through the old PPC64 tracepoint support patch, I've
noticed a few target dependencies in the testsuite that both me and
Antoine missed for s390 and ARM tracepoints, respectively.  This patch
moves them all to one place, so that anyone working on a new target
will hopefully see the whole set of needed changes.

I've also removed a check for fast tracepoint support in ftrace.exp -
it used hardcoded targets and wasn't doing anything useful anyway,
since unsupported architectures blow up on link due to missing
the IPA library before they ever get to that check.

For some strange reason, the call_insn setting code already knew about
arm, powerpc, s390, and mips - I went ahead and added the remaining
information about those.  I'm not particularly sure if I got mips right,
but that won't matter anyway until someone actually writes tracepoint
support for that.

In addition, the PPC64 tracepoint patch added \y at the end of the call_insn
pattern - without that, it embarassed itself and matched the 'bl' in
"Dump of assem*bl*er code for function" as the powerpc call opcode.  Since
that sounds like a generally good idea, I've added \y before and after
call_insn for every target.  As a result, I had to change x86_64's mnemonic
to 'callq'.

Tested on x86_64, i386, ppc, ppc64, ppc64le, s390, s390x.  Would be good
to test it on aarch64.

gdb/testsuite/ChangeLog:

	* gdb.trace/entry-values.exp: Move call_insn setting to
	trace-support.exp, surround $call_insn with \y.
	* gdb.trace/ftrace.exp (test_fast_tracepoints): Don't bother
	checking for a target that supports tracepoints (compile would
	blow up first).
	(toplevel): Move arg0exp setting to trace-support.exp.
	* gdb.trace/mi-trace-unavailable.exp (proc_trace_unavailable): Move
	pcnum setting to trace-support.exp, change fixed register 0 to
	gpr0num variable.
	* lib/trace-support.exp: Add setting pcnum, gpr0num, arg0exp,
	call_insn; add powerpc, s390, and mips support.
---
 gdb/testsuite/ChangeLog                          |  14 +++
 gdb/testsuite/gdb.trace/entry-values.exp         |  28 +----
 gdb/testsuite/gdb.trace/ftrace.exp               | 148 +++++++++++------------
 gdb/testsuite/gdb.trace/mi-trace-unavailable.exp |  25 ++--
 gdb/testsuite/lib/trace-support.exp              |  77 +++++++++++-
 5 files changed, 167 insertions(+), 125 deletions(-)
  

Marcin Kościelnicki writes:

> While groveling through the old PPC64 tracepoint support patch, I've
> noticed a few target dependencies in the testsuite that both me and
> Antoine missed for s390 and ARM tracepoints, respectively.  This patch
> moves them all to one place, so that anyone working on a new target
> will hopefully see the whole set of needed changes.

Thanks for this, it seems I indeed had missed a few, it also fixes
another issue in the arm tracepoints were I was using is_aarch32_target
rather then istarget[*arm*].

It's much more clear like that.

>
> I've also removed a check for fast tracepoint support in ftrace.exp -
> it used hardcoded targets and wasn't doing anything useful anyway,
> since unsupported architectures blow up on link due to missing
> the IPA library before they ever get to that check.
>

Maybe this should be another patch ? , Obvious ?

> For some strange reason, the call_insn setting code already knew about
> arm, powerpc, s390, and mips - I went ahead and added the remaining
> information about those.  I'm not particularly sure if I got mips right,
> but that won't matter anyway until someone actually writes tracepoint
> support for that.
>
> In addition, the PPC64 tracepoint patch added \y at the end of the call_insn
> pattern - without that, it embarassed itself and matched the 'bl' in
> "Dump of assem*bl*er code for function" as the powerpc call opcode.  Since
> that sounds like a generally good idea, I've added \y before and after
> call_insn for every target.  As a result, I had to change x86_64's mnemonic
> to 'callq'.
>

Also this could be another patch ?

> Tested on x86_64, i386, ppc, ppc64, ppc64le, s390, s390x.  Would be good
> to test it on aarch64.
>


> diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
> index f593c43..829786b 100644
> --- a/gdb/testsuite/lib/trace-support.exp
> +++ b/gdb/testsuite/lib/trace-support.exp
> @@ -20,26 +20,99 @@
>  
>  
>  #
> -# Program counter / stack pointer / frame pointer for supported targets.
> -# Used in many tests, kept here to avoid duplication.
> +# Target-specific information.  Used in many tests, kept here
> +# to avoid duplication and make it easier to add a new target.
>  #
>  
>  if [is_amd64_regs_target] {
> +    # Frame pointer.
>      set fpreg "rbp"
> +    # Stack pointer.
>      set spreg "rsp"
> +    # Program counter.
>      set pcreg "rip"
> +    # How to collect the first argument to a function.  Used to test
> +    # register usage in tracepoint conditions.
> +    set arg0exp "\$rdi"
> +    # The mnemonic of the usual, unconditional call instruction.
> +    set call_insn "callq"
> +    # Number of the PC register.
> +    set pcnum 16
> +    # Number of any GPR (it's supposed not to be some register that's not
> +    # collected by default).
> +    set gpr0num 0
>  } elseif [is_x86_like_target] {
>      set fpreg "ebp"
>      set spreg "esp"
>      set pcreg "eip"
> +    set arg0exp "*(int *) (\$ebp + 8)"
> +    set call_insn "call"
> +    set pcnum 8
> +    set gpr0num 0
>  } elseif [is_aarch64_target] {
>      set fpreg "x29"
>      set spreg "sp"
>      set pcreg "pc"
> +    set arg0exp "\$x0"
> +    set call_insn "bl"
> +    set pcnum 32
> +    set gpr0num 0
> +} elseif [istarget "arm*-*-*"]  {
> +    set fpreg "r11"

There is no fp as far as I can tell looking at arm aapcs maybe we can
use sp here ? It's just to collect a register that has a value in the
tests.

> +    set spreg "r13"

Need to use sp here, as r13 is not defined in arm-core.xml, using r13
causes a 'r13' is a user-register; GDB cannot yet trace user-register
contents. error when collecting.

> +    set pcreg "r15"

Use pc here for the same reasons as above.

> +    set arg0exp "\$r0"
> +    set call_insn "bl"
> +    set pcnum 15
> +    set gpr0num 0

Also in general since arm tracepoints are not there yet, this should be
part of my tracepoint patch I think, so you can ommit it here and I will
add it.

> +} elseif [istarget "powerpc*-*-*"] {
> +    set fpreg "r31"
> +    set spreg "r1"
> +    set pcreg "pc"
> +    set arg0exp "\$r3"
> +    set call_insn "bl"
> +    set pcnum 64
> +    set gpr0num 0
> +} elseif [istarget "s390*-*-*"] {
> +    set fpreg "r11"
> +    set spreg "r15"
> +    set pcreg "pc"
> +    set arg0exp "\$r2"
> +    set call_insn "brasl"
> +    # Strictly speaking, this is PSWA, not PC.
> +    set pcnum 1
> +    set gpr0num 2
> +} elseif { [istarget "mips*-*-*"] } {
> +    set fpreg "s8"
> +    set spreg "sp"
> +    set pcreg "pc"
> +    set arg0exp "\$a0"
> +    # Skip the delay slot after the instruction used to make a call
> +    # (which can be a jump or a branch) if it has one.
> +    #
> +    #  JUMP (or BRANCH) foo
> +    #  insn1
> +    #  insn2
> +    #
> +    # Most MIPS instructions used to make calls have a delay slot.
> +    # These include JAL, JALS, JALX, JALR, JALRS, BAL and BALS.
> +    # In this case the program continues from `insn2' when `foo'
> +    # returns.  The only exception is JALRC, in which case execution
> +    # resumes from `insn1' instead.
> +    set call_insn {jalrc|[jb]al[sxr]*[ \t][^\r\n]+\r\n}
> +    set pcnum 37
> +    set gpr0num 0
>  } else {
> +    # Defaults.  Probably won't work, but we don't want to error out
> +    # here on unsupported platforms, since this file is imported to check
> +    # for supported platforms in the first place.
>      set fpreg "fp"
>      set spreg "sp"
>      set pcreg "pc"
> +    set arg0exp "\$a0"
> +    set call_insn "call"
> +    set pcnum -1
> +    set gpr0num -1
>  }
>  
>  #

Otherwise I've tested this on arm with the sp/fp/pc changes and the
tracepoint patch , and it passes tests except for collecting some
registers locals but I think this is unrelated and will investigate.

Thanks,
Antoine
  

On 19/02/16 16:05, Antoine Tremblay wrote:
>
> Marcin Kościelnicki writes:
>
>> While groveling through the old PPC64 tracepoint support patch, I've
>> noticed a few target dependencies in the testsuite that both me and
>> Antoine missed for s390 and ARM tracepoints, respectively.  This patch
>> moves them all to one place, so that anyone working on a new target
>> will hopefully see the whole set of needed changes.
>
> Thanks for this, it seems I indeed had missed a few, it also fixes
> another issue in the arm tracepoints were I was using is_aarch32_target
> rather then istarget[*arm*].
>
> It's much more clear like that.
>
>>
>> I've also removed a check for fast tracepoint support in ftrace.exp -
>> it used hardcoded targets and wasn't doing anything useful anyway,
>> since unsupported architectures blow up on link due to missing
>> the IPA library before they ever get to that check.
>>
>
> Maybe this should be another patch ? , Obvious ?

Very well, I'll cut it up.
>
>> For some strange reason, the call_insn setting code already knew about
>> arm, powerpc, s390, and mips - I went ahead and added the remaining
>> information about those.  I'm not particularly sure if I got mips right,
>> but that won't matter anyway until someone actually writes tracepoint
>> support for that.
>>
>> In addition, the PPC64 tracepoint patch added \y at the end of the call_insn
>> pattern - without that, it embarassed itself and matched the 'bl' in
>> "Dump of assem*bl*er code for function" as the powerpc call opcode.  Since
>> that sounds like a generally good idea, I've added \y before and after
>> call_insn for every target.  As a result, I had to change x86_64's mnemonic
>> to 'callq'.
>>
>
> Also this could be another patch ?

Likewise.
>
>> Tested on x86_64, i386, ppc, ppc64, ppc64le, s390, s390x.  Would be good
>> to test it on aarch64.
>>
>
>
>> diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
>> index f593c43..829786b 100644
>> --- a/gdb/testsuite/lib/trace-support.exp
>> +++ b/gdb/testsuite/lib/trace-support.exp
>> @@ -20,26 +20,99 @@
>>
>>
>>   #
>> -# Program counter / stack pointer / frame pointer for supported targets.
>> -# Used in many tests, kept here to avoid duplication.
>> +# Target-specific information.  Used in many tests, kept here
>> +# to avoid duplication and make it easier to add a new target.
>>   #
>>
>>   if [is_amd64_regs_target] {
>> +    # Frame pointer.
>>       set fpreg "rbp"
>> +    # Stack pointer.
>>       set spreg "rsp"
>> +    # Program counter.
>>       set pcreg "rip"
>> +    # How to collect the first argument to a function.  Used to test
>> +    # register usage in tracepoint conditions.
>> +    set arg0exp "\$rdi"
>> +    # The mnemonic of the usual, unconditional call instruction.
>> +    set call_insn "callq"
>> +    # Number of the PC register.
>> +    set pcnum 16
>> +    # Number of any GPR (it's supposed not to be some register that's not
>> +    # collected by default).
>> +    set gpr0num 0
>>   } elseif [is_x86_like_target] {
>>       set fpreg "ebp"
>>       set spreg "esp"
>>       set pcreg "eip"
>> +    set arg0exp "*(int *) (\$ebp + 8)"
>> +    set call_insn "call"
>> +    set pcnum 8
>> +    set gpr0num 0
>>   } elseif [is_aarch64_target] {
>>       set fpreg "x29"
>>       set spreg "sp"
>>       set pcreg "pc"
>> +    set arg0exp "\$x0"
>> +    set call_insn "bl"
>> +    set pcnum 32
>> +    set gpr0num 0
>> +} elseif [istarget "arm*-*-*"]  {
>> +    set fpreg "r11"
>
> There is no fp as far as I can tell looking at arm aapcs maybe we can
> use sp here ? It's just to collect a register that has a value in the
> tests.

There isn't really a fp according to s390 ABI either, but r11 is what 
gcc uses when it uses a frame pointer (coincidentally for both s390 and 
arm).  I figured I'd play it safe, but might not be necessary.

>
>> +    set spreg "r13"
>
> Need to use sp here, as r13 is not defined in arm-core.xml, using r13
> causes a 'r13' is a user-register; GDB cannot yet trace user-register
> contents. error when collecting.
>
>> +    set pcreg "r15"
>
> Use pc here for the same reasons as above.

Whoops, thanks for noticing that.
>
>> +    set arg0exp "\$r0"
>> +    set call_insn "bl"
>> +    set pcnum 15
>> +    set gpr0num 0
>
> Also in general since arm tracepoints are not there yet, this should be
> part of my tracepoint patch I think, so you can ommit it here and I will
> add it.

Alright, I'll drop that part and leave it to you.
>
>> +} elseif [istarget "powerpc*-*-*"] {
>> +    set fpreg "r31"
>> +    set spreg "r1"
>> +    set pcreg "pc"
>> +    set arg0exp "\$r3"
>> +    set call_insn "bl"
>> +    set pcnum 64
>> +    set gpr0num 0
>> +} elseif [istarget "s390*-*-*"] {
>> +    set fpreg "r11"
>> +    set spreg "r15"
>> +    set pcreg "pc"
>> +    set arg0exp "\$r2"
>> +    set call_insn "brasl"
>> +    # Strictly speaking, this is PSWA, not PC.
>> +    set pcnum 1
>> +    set gpr0num 2
>> +} elseif { [istarget "mips*-*-*"] } {
>> +    set fpreg "s8"
>> +    set spreg "sp"
>> +    set pcreg "pc"
>> +    set arg0exp "\$a0"
>> +    # Skip the delay slot after the instruction used to make a call
>> +    # (which can be a jump or a branch) if it has one.
>> +    #
>> +    #  JUMP (or BRANCH) foo
>> +    #  insn1
>> +    #  insn2
>> +    #
>> +    # Most MIPS instructions used to make calls have a delay slot.
>> +    # These include JAL, JALS, JALX, JALR, JALRS, BAL and BALS.
>> +    # In this case the program continues from `insn2' when `foo'
>> +    # returns.  The only exception is JALRC, in which case execution
>> +    # resumes from `insn1' instead.
>> +    set call_insn {jalrc|[jb]al[sxr]*[ \t][^\r\n]+\r\n}
>> +    set pcnum 37
>> +    set gpr0num 0
>>   } else {
>> +    # Defaults.  Probably won't work, but we don't want to error out
>> +    # here on unsupported platforms, since this file is imported to check
>> +    # for supported platforms in the first place.
>>       set fpreg "fp"
>>       set spreg "sp"
>>       set pcreg "pc"
>> +    set arg0exp "\$a0"
>> +    set call_insn "call"
>> +    set pcnum -1
>> +    set gpr0num -1
>>   }
>>
>>   #
>
> Otherwise I've tested this on arm with the sp/fp/pc changes and the
> tracepoint patch , and it passes tests except for collecting some
> registers locals but I think this is unrelated and will investigate.
>
> Thanks,
> Antoine
>
  

On 02/19/2016 12:23 PM, Marcin Kościelnicki wrote:

> For some strange reason, the call_insn setting code already knew about
> arm, powerpc, s390, and mips - I went ahead and added the remaining
> information about those.  I'm not particularly sure if I got mips right,
> but that won't matter anyway until someone actually writes tracepoint
> support for that.

The gdb.trace/entry-values.exp testcase runs some tests before restarting gdb
for a trace session.  That first part of the testcase runs on all targets.

I'm not sure I really like this moving of test-specific variables that aren't
shared by more tests, to the shared trace support file.  Several of these are
only used after gdb_target_supports_trace checks.  So we could also say that
the problem is that the tests are skipped.  E.g., in ftrace.exp:

# This expression is used for testing emit_reg.
if [is_amd64_regs_target] {
    set arg0exp "\$rdi"
} elseif [is_x86_like_target] {
    set arg0exp "*(int *) (\$ebp + 8)"
} elseif { [istarget "aarch64*-*-*"] } {
    set arg0exp "\$x0"
} else {
    set arg0exp ""
}

if { "$arg0exp" != "" } {
    test_ftrace_condition "($arg0exp > 500)" "globvar" { 6 7 8 9 10 }
}

If we removed the else branch or set arg0exp to "port me", someone porting
fast tracepoints to a new architecture would notice it, instead of silently
getting the test skipped.

Thanks,
Pedro Alves