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

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

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
>