[PATCH] sim: avoid shadowing vars when decoding insns

[PATCH] sim: avoid shadowing vars when decoding insns

[Mike Frysinger]

When generating switch decode tables, the switch statements all use
the same variable name "val".  This causes nested switches to shadow
earlier ones.  Use the existing "switch-num" to generate unique names
when nesting since it's otherwise unused.

Previously we'd have:

...
    {
      unsigned int val = (((insn >> 4) & (255 << 0)));
      switch (val)
      {
...
      case 15 :
        {
          unsigned int val = (((insn >> 12) & (15 << 0)));
          switch (val)
          {
...

Leading to:
.../sim/cris/decodev10.c: In function ‘crisv10f_decode’:
.../sim/cris/decodev10.c:351:24: error: declaration of ‘val’ shadows a previous local [-Werror=shadow=compatible-local]
  351 |           unsigned int val = (((insn >> 12) & (15 << 0)));
      |                        ^~~
.../sim/cris/decodev10.c:331:20: note: shadowed declaration is here
  331 |       unsigned int val = (((insn >> 4) & (255 << 0)));
      |                    ^~~

Now we have:
...
    {
      unsigned int val0 = (((insn >> 4) & (255 << 0)));
      switch (val0)
      {
...
      case 15 :
        {
          unsigned int val1 = (((insn >> 12) & (15 << 0)));
          switch (val1)
          {
...
---
 utils-sim.scm | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/utils-sim.scm b/utils-sim.scm
index 07d776d286d8..dca0f0705036 100644
--- a/utils-sim.scm
+++ b/utils-sim.scm
@@ -995,8 +995,8 @@
 )
 
 ; Generate switch statement to decode TABLE-GUTS.
-; SWITCH-NUM is for compatibility with the computed goto decoder and
-; isn't used.
+; SWITCH-NUM is for compatibility with the computed goto decoder and tracks the
+; nesting depth.
 ; STARTBIT is the bit offset of the instruction value that C variable `insn'
 ; holds (note that this is independent of LSB0?).
 ; DECODE-BITSIZE is the number of bits of the insn that `insn' holds.
@@ -1017,7 +1017,8 @@
 			     table-guts table-guts-thus-far
 			     indent lsb0? invalid-insn fn?)
 
-  (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts))))
+  (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts)))
+        (varname (string-append "val" (number->string switch-num))))
 
     (string-list
      indent "{\n"
@@ -1027,19 +1028,19 @@
 	   (set! startbit (dtable-guts-startbit table-guts))
 	   (set! decode-bitsize (dtable-guts-bitsize table-guts))
 	 ;; FIXME: Bits may get fetched again during extraction.
-	   (string-append indent "  unsigned int val;\n"
+	   (string-append indent "  unsigned int " varname ";\n"
 			  indent "  /* Must fetch more bits.  */\n"
 			  indent "  insn = "
 			  (gen-ifetch "pc" startbit decode-bitsize)
 			  ";\n"
-			  indent "  val = "))
-	 (string-append indent "  unsigned int val = "))
+			  indent "  " varname " = "))
+	 (string-append indent "  unsigned int " varname " = "))
      (/gen-decode-bits (dtable-guts-bitnums table-guts)
 		       (dtable-guts-startbit table-guts)
 		       (dtable-guts-bitsize table-guts)
 		       "insn" "entire_insn" lsb0?)
      ";\n"
-     indent "  switch (val)\n"
+     indent "  switch (" varname ")\n"
      indent "  {\n"
 
      ;; The code is more readable, and icache use is improved, if we collapse
@@ -1067,7 +1068,7 @@
 		     (/gen-decode-expr-entry (car entries) indent invalid-insn fn?))
 		    ((table)
 		     (/gen-decode-table-entry (car entries) (cdr entries)
-					      switch-num startbit decode-bitsize
+					      (+ switch-num 1) startbit decode-bitsize
 					      new-table-guts-thus-far
 					      indent lsb0? invalid-insn fn?))
 		    )
@@ -1105,7 +1106,7 @@
 
     ; Now print it out.
 
-    (/gen-decoder-switch "0" 0 decode-bitsize
+    (/gen-decoder-switch 0 0 decode-bitsize
 			 table-guts nil
 			 indent lsb0? invalid-insn fn?))
 )
-- 
2.43.0

Re: [PATCH] sim: avoid shadowing vars when decoding insns

[Jose E. Marchesi]

Hi Mike.

>  ; Generate switch statement to decode TABLE-GUTS.
> -; SWITCH-NUM is for compatibility with the computed goto decoder and
> -; isn't used.
> +; SWITCH-NUM is for compatibility with the computed goto decoder and tracks the
> +; nesting depth.

I wonder if it would be better to not mention the "computed goto
decoder" at all in the comment block, since AFAIK it doesn't
exist.

Other than that the patch is OK.
Thanks.

>  ; STARTBIT is the bit offset of the instruction value that C variable `insn'
>  ; holds (note that this is independent of LSB0?).
>  ; DECODE-BITSIZE is the number of bits of the insn that `insn' holds.
> @@ -1017,7 +1017,8 @@
>  			     table-guts table-guts-thus-far
>  			     indent lsb0? invalid-insn fn?)
>  
> -  (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts))))
> +  (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts)))
> +        (varname (string-append "val" (number->string switch-num))))
>  
>      (string-list
>       indent "{\n"
> @@ -1027,19 +1028,19 @@
>  	   (set! startbit (dtable-guts-startbit table-guts))
>  	   (set! decode-bitsize (dtable-guts-bitsize table-guts))
>  	 ;; FIXME: Bits may get fetched again during extraction.
> -	   (string-append indent "  unsigned int val;\n"
> +	   (string-append indent "  unsigned int " varname ";\n"
>  			  indent "  /* Must fetch more bits.  */\n"
>  			  indent "  insn = "
>  			  (gen-ifetch "pc" startbit decode-bitsize)
>  			  ";\n"
> -			  indent "  val = "))
> -	 (string-append indent "  unsigned int val = "))
> +			  indent "  " varname " = "))
> +	 (string-append indent "  unsigned int " varname " = "))
>       (/gen-decode-bits (dtable-guts-bitnums table-guts)
>  		       (dtable-guts-startbit table-guts)
>  		       (dtable-guts-bitsize table-guts)
>  		       "insn" "entire_insn" lsb0?)
>       ";\n"
> -     indent "  switch (val)\n"
> +     indent "  switch (" varname ")\n"
>       indent "  {\n"
>  
>       ;; The code is more readable, and icache use is improved, if we collapse
> @@ -1067,7 +1068,7 @@
>  		     (/gen-decode-expr-entry (car entries) indent invalid-insn fn?))
>  		    ((table)
>  		     (/gen-decode-table-entry (car entries) (cdr entries)
> -					      switch-num startbit decode-bitsize
> +					      (+ switch-num 1) startbit decode-bitsize
>  					      new-table-guts-thus-far
>  					      indent lsb0? invalid-insn fn?))
>  		    )
> @@ -1105,7 +1106,7 @@
>  
>      ; Now print it out.
>  
> -    (/gen-decoder-switch "0" 0 decode-bitsize
> +    (/gen-decoder-switch 0 0 decode-bitsize
>  			 table-guts nil
>  			 indent lsb0? invalid-insn fn?))
>  )

Re: [PATCH] sim: avoid shadowing vars when decoding insns

[Mike Frysinger]

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

On 22 Dec 2023 11:45, Jose E. Marchesi wrote:
> >  ; Generate switch statement to decode TABLE-GUTS.
> > -; SWITCH-NUM is for compatibility with the computed goto decoder and
> > -; isn't used.
> > +; SWITCH-NUM is for compatibility with the computed goto decoder and tracks the
> > +; nesting depth.
> 
> I wonder if it would be better to not mention the "computed goto
> decoder" at all in the comment block, since AFAIK it doesn't
> exist.

i have no idea what it is, so i deleted it as suggested
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]