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

Antoine Tremblay
2016-02-19T15:05:18Z

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