RFC: ARM simulator coredump

RFC: ARM simulator coredump

[Fred Fish]

The ARM simulator is dumping core during gdb testing for arm-elf:

  Program received signal SIGSEGV, Segmentation fault.
  0x08147074 in XScale_cp14_read_reg (state=0x8299ff0, reg=0, value=0x0) at /src/sourceware/gdb/src/sim/arm/armcopro.c:981
  981       * value = read_cp14_reg (reg);

This patch seems to work, but I've not really checked it too carefully
for correctness.  Can whomever is responsible for the ARM simulator
look it over please?  Thanks.

-Fred

Index: armemu.c
===================================================================
RCS file: /cvs/src/src/sim/arm/armemu.c,v
retrieving revision 1.25
diff -u -p -r1.25 armemu.c
--- armemu.c	2001/10/18 12:20:47	1.25
+++ armemu.c	2002/01/09 22:59:41
@@ -544,15 +544,16 @@ ARMul_Emulate26 (ARMul_State * state)
       /* Handle the Clock counter here.  */
       if (state->is_XScale)
 	{
-	  ARMword cp14r0 = state->CPRead[14] (state, 0, 0);
+	  ARMword cp14r0;
+	  int ok = state->CPRead[14] (state, 0, &cp14r0);
 
-	  if (cp14r0 && ARMul_CP14_R0_ENABLE)
+	  if (ok && ARMul_CP14_R0_ENABLE)
 	    {
 	      unsigned long newcycles, nowtime = ARMul_Time(state);
 
 	      newcycles = nowtime - state->LastTime;
 	      state->LastTime = nowtime;
-	      if (cp14r0 && ARMul_CP14_R0_CCD)
+	      if (ok && ARMul_CP14_R0_CCD)
 	        {
 		  if (state->CP14R0_CCD == -1)
 		    state->CP14R0_CCD = newcycles;
@@ -576,7 +577,7 @@ check_PMUintr:
 		  cp14r0 |= ARMul_CP14_R0_FLAG2;
 		  (void) state->CPWrite[14] (state, 0, cp14r0);
 
-		  cp14r1 = state->CPRead[14] (state, 1, 0);
+		  ok = state->CPRead[14] (state, 1, &cp14r1);
 
 		  /* Coded like this for portability.  */
 		  while (newcycles)
@@ -593,7 +594,8 @@ check_PMUintr:
 		  (void) state->CPWrite[14] (state, 1, cp14r1);
 		  if (do_int && (cp14r0 & ARMul_CP14_R0_INTEN2))
 		    {
-		      if (state->CPRead[13] (state, 8, 0)
+		      ARMword temp;
+		      if (state->CPRead[13] (state, 8, &temp)
 			&& ARMul_CP13_R8_PMUS)
 		        ARMul_Abort (state, ARMul_FIQV);
 		      else
Index: arminit.c
===================================================================
RCS file: /cvs/src/src/sim/arm/arminit.c,v
retrieving revision 1.7
diff -u -p -r1.7 arminit.c
--- arminit.c	2001/04/18 16:39:37	1.7
+++ arminit.c	2002/01/09 22:59:41
@@ -302,14 +302,20 @@ ARMul_Abort (ARMul_State * state, ARMwor
       SETABORT (IBIT, SVC26MODE, isize);
       break;
     case ARMul_IRQV:		/* IRQ */
-      if (!state->is_XScale
-	  || (state->CPRead[13](state, 0, 0) & ARMul_CP13_R0_IRQ))
-        SETABORT (IBIT, state->prog32Sig ? IRQ32MODE : IRQ26MODE, esize);
+      {
+	ARMword temp;
+	(void) state->CPRead[13](state, 0, &temp);
+	if (!state->is_XScale || (temp & ARMul_CP13_R0_IRQ))
+	  SETABORT (IBIT, state->prog32Sig ? IRQ32MODE : IRQ26MODE, esize);
+      }
       break;
     case ARMul_FIQV:		/* FIQ */
-      if (!state->is_XScale
-	  || (state->CPRead[13](state, 0, 0) & ARMul_CP13_R0_FIQ))
-        SETABORT (INTBITS, state->prog32Sig ? FIQ32MODE : FIQ26MODE, esize);
+      {
+	ARMword temp;
+	(void) state->CPRead[13](state, 0, &temp);
+	if (!state->is_XScale || (temp & ARMul_CP13_R0_FIQ))
+	  SETABORT (INTBITS, state->prog32Sig ? FIQ32MODE : FIQ26MODE, esize);
+      }
       break;
     }
   if (ARMul_MODE32BIT)

Re: RFC: ARM simulator coredump

[Andrew Cagney]

Fred,


To be honest, the best person I can think of is Nick (added to CC) who 
has poked around the internals once or twice.  If you don't otherwize 
get any responses just check it in.  The Arm isn't maintained and long 
ago diverged from the original armulator.

enjoy,
Andrew


> The ARM simulator is dumping core during gdb testing for arm-elf:
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   0x08147074 in XScale_cp14_read_reg (state=0x8299ff0, reg=0, value=0x0) at /src/sourceware/gdb/src/sim/arm/armcopro.c:981
>   981       * value = read_cp14_reg (reg);
> 
> This patch seems to work, but I've not really checked it too carefully
> for correctness.  Can whomever is responsible for the ARM simulator
> look it over please?  Thanks.
> 
> -Fred
> 
> Index: armemu.c
> ===================================================================
> RCS file: /cvs/src/src/sim/arm/armemu.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 armemu.c
> --- armemu.c	2001/10/18 12:20:47	1.25
> +++ armemu.c	2002/01/09 22:59:41
> @@ -544,15 +544,16 @@ ARMul_Emulate26 (ARMul_State * state)
>        /* Handle the Clock counter here.  */
>        if (state->is_XScale)
>  	{
> -	  ARMword cp14r0 = state->CPRead[14] (state, 0, 0);
> +	  ARMword cp14r0;
> +	  int ok = state->CPRead[14] (state, 0, &cp14r0);
>  
> -	  if (cp14r0 && ARMul_CP14_R0_ENABLE)
> +	  if (ok && ARMul_CP14_R0_ENABLE)
>  	    {
>  	      unsigned long newcycles, nowtime = ARMul_Time(state);
>  
>  	      newcycles = nowtime - state->LastTime;
>  	      state->LastTime = nowtime;
> -	      if (cp14r0 && ARMul_CP14_R0_CCD)
> +	      if (ok && ARMul_CP14_R0_CCD)
>  	        {
>  		  if (state->CP14R0_CCD == -1)
>  		    state->CP14R0_CCD = newcycles;
> @@ -576,7 +577,7 @@ check_PMUintr:
>  		  cp14r0 |= ARMul_CP14_R0_FLAG2;
>  		  (void) state->CPWrite[14] (state, 0, cp14r0);
>  
> -		  cp14r1 = state->CPRead[14] (state, 1, 0);
> +		  ok = state->CPRead[14] (state, 1, &cp14r1);
>  
>  		  /* Coded like this for portability.  */
>  		  while (newcycles)
> @@ -593,7 +594,8 @@ check_PMUintr:
>  		  (void) state->CPWrite[14] (state, 1, cp14r1);
>  		  if (do_int && (cp14r0 & ARMul_CP14_R0_INTEN2))
>  		    {
> -		      if (state->CPRead[13] (state, 8, 0)
> +		      ARMword temp;
> +		      if (state->CPRead[13] (state, 8, &temp)
>  			&& ARMul_CP13_R8_PMUS)
>  		        ARMul_Abort (state, ARMul_FIQV);
>  		      else
> Index: arminit.c
> ===================================================================
> RCS file: /cvs/src/src/sim/arm/arminit.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 arminit.c
> --- arminit.c	2001/04/18 16:39:37	1.7
> +++ arminit.c	2002/01/09 22:59:41
> @@ -302,14 +302,20 @@ ARMul_Abort (ARMul_State * state, ARMwor
>        SETABORT (IBIT, SVC26MODE, isize);
>        break;
>      case ARMul_IRQV:		/* IRQ */
> -      if (!state->is_XScale
> -	  || (state->CPRead[13](state, 0, 0) & ARMul_CP13_R0_IRQ))
> -        SETABORT (IBIT, state->prog32Sig ? IRQ32MODE : IRQ26MODE, esize);
> +      {
> +	ARMword temp;
> +	(void) state->CPRead[13](state, 0, &temp);
> +	if (!state->is_XScale || (temp & ARMul_CP13_R0_IRQ))
> +	  SETABORT (IBIT, state->prog32Sig ? IRQ32MODE : IRQ26MODE, esize);
> +      }
>        break;
>      case ARMul_FIQV:		/* FIQ */
> -      if (!state->is_XScale
> -	  || (state->CPRead[13](state, 0, 0) & ARMul_CP13_R0_FIQ))
> -        SETABORT (INTBITS, state->prog32Sig ? FIQ32MODE : FIQ26MODE, esize);
> +      {
> +	ARMword temp;
> +	(void) state->CPRead[13](state, 0, &temp);
> +	if (!state->is_XScale || (temp & ARMul_CP13_R0_FIQ))
> +	  SETABORT (INTBITS, state->prog32Sig ? FIQ32MODE : FIQ26MODE, esize);
> +      }
>        break;
>      }
>    if (ARMul_MODE32BIT)
> 
> 
> 

Re: RFC: ARM simulator coredump

[Nick Clifton]

Hi Fred,

> The ARM simulator is dumping core during gdb testing for arm-elf:

Which gdb test(s) in particular ?

> This patch seems to work, but I've not really checked it too
> carefully for correctness.  Can whomever is responsible for the ARM
> simulator look it over please?  Thanks.

Sure - that would be me.  The patch has the right idea, but also a few
mistakes.  Mind you seeing it prompted me to dig up an old patch that
I had for this and apply it.  Please try updating your sources and let
me know if this had fixed things.


In case you are interested here are a few comments on the patch.

>  -	  if (cp14r0 && ARMul_CP14_R0_ENABLE)
> +	  if (ok && ARMul_CP14_R0_ENABLE)

This was a bug in the original code.  The intention was to test the
ARMul_CP14_R0_ENABLE bit in the r0 register of co-processor 14, so '&'
should have been used instead of '&&'.  Hence the correct version of
this line would be:

          if (ok && (cp14r0 & ARMul_CP14_R0_ENABLE))


> -	      if (cp14r0 && ARMul_CP14_R0_CCD)
> +	      if (ok && ARMul_CP14_R0_CCD)

A similar comment applied here, plus there is no need to retest 'ok'.

> -		      if (state->CPRead[13] (state, 8, 0)
> +		      ARMword temp;
> +		      if (state->CPRead[13] (state, 8, &temp)
>  			&& ARMul_CP13_R8_PMUS)

And here.

> +      {
> +	ARMword temp;

There is no need to declare a 'temp' variable here.  There is a
variable of the same type and name declared at the top of the
function, and it is no longer being used by this point.

Cheers
        Nick

Re: RFC: ARM simulator coredump

[Andrew Cagney]

> +      {
>> +	ARMword temp;
> 
> 
> There is no need to declare a 'temp' variable here.  There is a
> variable of the same type and name declared at the top of the
> function, and it is no longer being used by this point.


Er, perhaphs here.  In general, keeping declarations as local as 
possible is a good thing :-)

enjoy,
Andrew

Re: RFC: ARM simulator coredump

[Nick Clifton]

Hi Andrew,

> > +      {
> >> +	ARMword temp;
> > There is no need to declare a 'temp' variable here.  There is a
> > variable of the same type and name declared at the top of the
> > function, and it is no longer being used by this point.
> 
> Er, perhaphs here.  In general, keeping declarations as local as
> possible is a good thing :-)

True, but in this case the new variable was shadowing a local variable
declared at the function scope.  Either it should have been given a
new name, or since it was just being used as a temporary placeholder
the function scoped variable should have been reused.

Cheers
        Nick

ARM simulator maintainer

[Nick Clifton]

Hi Guys,

  I have added my name to the sim/MAINTAINERS file as the maintainer
  for the ARM simulator.  I hope that nobody minds.

Cheers
        Nick

Re: ARM simulator maintainer

[Andrew Cagney]

> Hi Guys,
> 
>   I have added my name to the sim/MAINTAINERS file as the maintainer
>   for the ARM simulator.  I hope that nobody minds.
> 
> Cheers
>         Nick


Probably not.  It is sort of a question for Frank E but I don't see 
Frank E doing any sim/* simulator work.

Andrew

Re: ARM simulator maintainer

[Nick Clifton]

Hi Andrew,

> >   I have added my name to the sim/MAINTAINERS file as the maintainer
> >   for the ARM simulator.  I hope that nobody minds.
> 
> Probably not.  It is sort of a question for Frank E but I don't see
> Frank E doing any sim/* simulator work.

I am happy to share. :-)

Cheers
        Nick

Re: ARM simulator maintainer

[Frank Ch. Eigler]

cagney wrote:

> >   I have added my name to the sim/MAINTAINERS file as the maintainer
> >   for the ARM simulator.  I hope that nobody minds.
> 
> Probably not.  It is sort of a question for Frank E but I don't see
> Frank E doing any sim/* simulator work.

True, there has been little general sim/* stuff to do.  The various
experts (arm/ppc/etc.) have been doing just fine, and are welcome to
put themselves forward.


- FChE

Re: ARM simulator maintainer

[Andrew Cagney]

> cagney wrote:
> 
> 
>> > I have added my name to the sim/MAINTAINERS file as the maintainer
>> > for the ARM simulator.  I hope that nobody minds.
> 
>> 
>> Probably not.  It is sort of a question for Frank E but I don't see
>> Frank E doing any sim/* simulator work.
> 
> 
> True, there has been little general sim/* stuff to do.  The various
> experts (arm/ppc/etc.) have been doing just fine, and are welcome to
> put themselves forward.


As overall sim maintainer/co-ordinator?  Ok, I'll put my self forward.

Andrew

Re: ARM simulator maintainer

[Frank Ch. Eigler]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

Hi -

cagney wrote:
> > True, there has been little general sim/* stuff to do.  The various
> > experts (arm/ppc/etc.) have been doing just fine, and are welcome to
> > put themselves forward.
> 
> As overall sim maintainer/co-ordinator?  Ok, I'll put my self forward.

Certainly -- adding yourself to the list would be an "obvious fix".


- FChE

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: ARM simulator maintainer

[Andrew Cagney]

> Hi -
> 
> cagney wrote:
> 
>> > True, there has been little general sim/* stuff to do.  The various
>> > experts (arm/ppc/etc.) have been doing just fine, and are welcome to
>> > put themselves forward.
> 
>> 
>> As overall sim maintainer/co-ordinator?  Ok, I'll put my self forward.
> 
> 
> Certainly -- adding yourself to the list would be an "obvious fix".


Add or replace.  I'm reading your suggestion as ``... put themselves 
forward [as the co-ordinator]''.  I believe others are as well.

Andrew

Re: ARM simulator maintainer

[Frank Ch. Eigler]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

Hi -

cagney wrote:
> [...]
> >> As overall sim maintainer/co-ordinator?  Ok, I'll put my self forward.
> > Certainly -- adding yourself to the list would be an "obvious fix".
> 
> Add or replace.  I'm reading your suggestion as ``... put themselves 
> forward [as the co-ordinator]''.  I believe others are as well.

I meant simply to restate the long-standing invitation for people to
take interest in not-sufficiently-attended-to pieces of code.  I
wonder what hidden intent you might read into this statement though.


- FChE

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: ARM simulator maintainer

[Andrew Cagney]

> Hi -
> 
> cagney wrote:
> 
>> [...]
> 
>> >> As overall sim maintainer/co-ordinator?  Ok, I'll put my self forward.
> 
>> > Certainly -- adding yourself to the list would be an "obvious fix".
> 
>> 
>> Add or replace.  I'm reading your suggestion as ``... put themselves 
>> forward [as the co-ordinator]''.  I believe others are as well.
> 
> 
> I meant simply to restate the long-standing invitation for people to
> take interest in not-sufficiently-attended-to pieces of code.  I
> wonder what hidden intent you might read into this statement though.


Hidden intent?  I would like an active GDB/SIM developer co-ordinating 
sim.  You must admit you're not exactly active so I'm stepping forward.

Andrew

Re: ARM simulator maintainer

[Frank Ch. Eigler]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Hi -

cagney wrote:
> [...]
> Hidden intent?  I would like an active GDB/SIM developer co-ordinating 
> sim.  You must admit you're not exactly active so I'm stepping forward.

I didn't realize that my performance of this relatively ceremonial and
unofficial role was measured thusly.  I might understand the purpose
of this discussion better if you were to clarify what is the set of
powers / responsibilities that you believe: I have AND you don't have
AND you should have AND I shouldn't have.

- FChE

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: ARM simulator maintainer

[Andrew Cagney]

> Hi -
> 
> cagney wrote:
> 
>> [...]
>> Hidden intent?  I would like an active GDB/SIM developer co-ordinating 
>> sim.  You must admit you're not exactly active so I'm stepping forward.
> 
> 
> I didn't realize that my performance of this relatively ceremonial and
> unofficial role was measured thusly.


That is a very interesting.

I'm expecting the person cordinating the sim directory to be activly 
encouraging and contributing to that code's development: setting its 
direction, anticipating problems with GDB, adding new features, and so 
on.  You appear to be describing it as something that looks good at the 
bottom of a resume :-(

It could be a misunderstanding of the expected role or perhaphs that the 
  expectations of the role are changing.

Andrew

Re: RFC: ARM simulator coredump

[Richard Earnshaw]

> +	  int ok = state->CPRead[14] (state, 0, &cp14r0);

As a coding nit, I can't say I'm madly keen on a variable that's called 
"ok".  At any point other than its assignment, it isn't going to be very 
clear what is "ok".  This variable is used over more than 50 lines of code 
which, at least for me, means that I won't be able to see the assignment 
at the same time as it's final use.

R.