Re: [PATCH] gdb.trace: Move more target dependencies to trace-support.exp

["Marcin Kościelnicki" <koriakin@0x04.net>]

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
>