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

Marcin Kościelnicki
2016-02-19T15:18:26Z

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
>