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

  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(+)
  

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.
  

* 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
  

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 :)
  

On 04/10/2018 09:25 PM, Palmer Dabbelt wrote:
> On Mon, 09 Apr 2018 15:26:07 PDT (-0700), andrew.burgess@embecosm.com wrote:

>> 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 :)

This is OK, then.

For the register lists, I wonder whether a better option would be to
instead let gdb determine which registers should be compared, instead
of hard coding in the testcase.  Like, look at the save/restore
reggroups instead of all registers.

Thanks,
Pedro Alves