Re: [PATCH 2/3] gdb/testsuite: Filter out some registers for riscv

[Palmer Dabbelt <palmer@sifive.com>]

On Mon, 09 Apr 2018 15:26:07 PDT (-0700), andrew.burgess@embecosm.com wrote:
> * Palmer Dabbelt <palmer@sifive.com> [2018-04-09 14:28:33 -0700]:
>
>> On Mon, 09 Apr 2018 08:15:28 PDT (-0700), andrew.burgess@embecosm.com wrote:
>> > On riscv the cycle counter, and instructions retired counter CSRs are
>> > read only, this causes problems in the gdb.base/callfuncs.exp test, as
>> > the values in these CSRs change after an inferior call, the check that
>> > no target registers have been modified then fails.
>> >
>> > Luckily the test already has a mechanism in place for filtering out
>> > registers that are modified (and can't be restored) by an inferior call,
>> > so this commit adds the problem registers into this list for riscv.
>> >
>> > In the future we may end up needing to filter out more CSRs, but right
>> > now, for the targets I have access too, these are the only ones causing
>> > problems.
>> >
>> > gdb/testsuite/ChangeLog:
>> >
>> > 	* gdb.base/callfuncs.exp (fetch_all_registers): Add riscv register
>> > 	filter pattern.
>> > ---
>> >  gdb/testsuite/ChangeLog              |  5 +++++
>> >  gdb/testsuite/gdb.base/callfuncs.exp | 10 ++++++++++
>> >  2 files changed, 15 insertions(+)
>> >
>> > diff --git a/gdb/testsuite/gdb.base/callfuncs.exp b/gdb/testsuite/gdb.base/callfuncs.exp
>> > index 94636938752..c5e39918c2a 100644
>> > --- a/gdb/testsuite/gdb.base/callfuncs.exp
>> > +++ b/gdb/testsuite/gdb.base/callfuncs.exp
>> > @@ -285,6 +285,16 @@ proc fetch_all_registers {test} {
>> >  	    }
>> >  	    exp_continue
>> >  	}
>> > +	-re "^\(?:cycle\|instret\)\[ \t\]+\[^\r\n\]+\r\n" {
>> > +	    if [istarget "riscv*-*-*"] {
>> > +		# Filter out the cycle counter and instructions
>> > +		# retired counter CSRs which are read-only, giving
>> > +		# spurious differences.
>> > +	    } else {
>> > +		lappend all_registers_lines $expect_out(0,string)
>> > +	    }
>> > +	    exp_continue
>> > +	}
>> >  	-re "^\[^ \t\]+\[ \t\]+\[^\r\n\]+\r\n" {
>> >  	    lappend all_registers_lines $expect_out(0,string)
>> >  	    exp_continue
>>
>> I think we only want to check the X and F registers here -- essentially
>> every CSR is a special register where you can't really rely on the value not
>> being changed somewhere by hardware.  For example:
>>
>> * The interrupt pending bits could flip at any point, even if interrupts are
>> disabled.
>> * The floating-point dirty and exception state bits could change if a
>>  floating-point instruction executes.
>> * The various trap CSRs (epc, badaddr, cause, etc) get set whenever a trap
>> is  executed.
>
> I'm not sure about the second case.  If GDB is stopped and we perform
> an inferior call, then ideally the entire floating point state,
> including contents of things like fcsr would be reset, otherwise, when
> we continue the behaviour might not be as we expect.
>
> I do agree with you that the two registers I've filtered so far
> probably aren't enough, but I'm really reluctant to _only_ check X and
> F registers.  I think a better selection would be X, F, and read/write
> user CSRs.  Which means I need to build the list of CSRs to filter, I
> was hoping to put that off for another day for now...
>
> Let me know how you'd feel about leaving this as it is for now, and
> extending the filter list at a later date.

I think it's fine for now, we can fix it when another test fails :)