Re: [PATCH v4] gdbserver/s390: Switch on tracepoint support.

Marcin Kościelnicki
2016-03-24T01:15:53Z

On 23/03/16 16:25, Andreas Arnez wrote:
> On Tue, Mar 15 2016, Marcin KoÅ›cielnicki wrote:
>
>> Also adds s390 support to gdb.trace testsuite.
>>
>> gdb/gdbserver/ChangeLog:
>>
>> * linux-s390-low.c (s390_supports_tracepoints): New function.
>> (struct linux_target_ops): Fill supports_tracepoints hook.
>>
>> gdb/testsuite/ChangeLog:
>>
>> * gdb.trace/ftrace.exp: Set arg0exp for s390.
>> * gdb.trace/mi-trace-frame-collected.exp: Expect 4 registers on s390.
>> * gdb.trace/mi-trace-unavailable.exp: Set pcnum for s390, add gpr0num
>> variable for GPR 0 instead of assuming it is register 0.
>> * gdb.trace/trace-common.h: Add s390 fast tracepoint placeholder.
>> * lib/trace-support.exp: Add s390 registers.
>> ---
>> This fixes a minor conflict with the powerpc regular tracepoint support
>> pushed in the meantime (mi-trace-unavailable.exp now needs gpr0num in
>> powerpc branch).
>
> Please also document this new feature in the NEWS file.

OK, I'll submit another patch adding the NEWS entry (it should go after
the fast tracepoints).
>
> [...]
>
>> diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
>> index 372a595..b307f3f 100644
>> --- a/gdb/testsuite/lib/trace-support.exp
>> +++ b/gdb/testsuite/lib/trace-support.exp
>> @@ -40,6 +40,10 @@ if [is_amd64_regs_target] {
>> set fpreg "r31"
>> set spreg "r1"
>> set pcreg "pc"
>> +} elseif { [istarget "s390*-*-*"] } {
>> + set fpreg "r11"
>> + set spreg "r15"
>> + set pcreg "pc"
>> } else {
>> set fpreg "fp"
>> set spreg "sp"
>
> Without having looked into the details I wonder why we can't use the
> default, fp, sp, and pc. Also, I'd slightly prefer initializing these
> variables with their default values first and then adjusting them per
> target as required. (This is just a matter of taste.)

These are user registers, which are not supported by the
target-independent tracepoint code:

'sp' is a user-register; GDB cannot yet trace user-register contents.

>
> Otherwise the patch looks good to me. But I think a global- or
> testsuite-maintainer should approve the changes to the testsuite.
>
> Thanks,
> Andreas
>