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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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.

thanks,
Andrew