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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

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