I don't think it's worthwhile weeding out unused-variable warnings in generated code.  How about adding -Wno-error=unused-variable for those files?
Git-grepping for Wno-error I see there's partial support for adding that for individual files.

i'd be inclined to agree if things were more widespread, but these 5 are the only unused variable warnings left in the sim tree at this point.  i'm not super keen on adding -Wno-error=xxx to makefiles because we should test the flag is supported by the compiler before adding it, and that makes the build logic quite a bit more complicated.

also, in the process of cleaning the sim code of unused variable warnings, it's uncovered a few actual bugs where people made thinkos/typos in the code -- they intended to use a particular var after setting it up, but then omitted it or used a very similarly named var.  that scenario can come up in the generated code logic too, although it hasn't as of yet.

(In reply to Mike Frysinger from comment #2)
> i'd be inclined to agree if things were more widespread, but these 5 are the
> only unused variable warnings left in the sim tree at this point.  i'm not
> super keen on adding -Wno-error=xxx to makefiles because we should test the
> flag is supported by the compiler before adding it, and that makes the build
> logic quite a bit more complicated.
> 
> also, in the process of cleaning the sim code of unused variable warnings,
> it's uncovered a few actual bugs where people made thinkos/typos in the code
> -- they intended to use a particular var after setting it up, but then
> omitted it or used a very similarly named var.  that scenario can come up in
> the generated code logic too, although it hasn't as of yet.

My point is that *these two files* would be the only files that got -Wno-error=unused-variable.  You say, you've looked for, but not found, a bug-reason why the warnings appear, in this generated code, presumably including looking at the generator.  I don't see the point in going deeper, but feel free.

(In reply to Hans-Peter Nilsson from comment #3)
> My point is that *these two files* would be the only files that got
> -Wno-error=unused-variable.

i understand what you're suggesting.  my point still stands.  this assumes that the toolchain used to compile the sim recognizes & accepts (or silently ignores) "-Wno-error=unused-variable".  the sim, like all binutils+gdb projects, test the toolchain and whether it supports -W flags before using them.  we have to add a test for -Wno-error=unused-variable, a variable for it, and then expand that for these 2 files.  seems a lot easier to fix the origin, especially when enabling warnings has a proven history of uncovering real bugs.

> You say, you've looked for, but not found, a
> bug-reason why the warnings appear, in this generated code, presumably
> including looking at the generator.

the generator appears to be cgen lisp which uses the lisp cpu definitions.  the cgen code is not easy to follow at all, or trace back which funcs generate which lines.  i was hoping you, as author of much of this code, would be able to pick out & fix things much quicker than i.

(In reply to Mike Frysinger from comment #4)
> (In reply to Hans-Peter Nilsson from comment #3)
> > My point is that *these two files* would be the only files that got
> > -Wno-error=unused-variable.
> 
> i understand what you're suggesting.  my point still stands.  this assumes
> that the toolchain used to compile the sim recognizes & accepts (or silently
> ignores) "-Wno-error=unused-variable". 

I don't think that's necessary: warnings are only enabled for gcc, and for developer mode.  Developers can be expected to have a sufficiently modern gcc.  I see it works for gcc-10 (Debian 11).

> the sim, like all binutils+gdb
> projects, test the toolchain and whether it supports -W flags before using
> them.  we have to add a test for -Wno-error=unused-variable, a variable for
> it, and then expand that for these 2 files.

Nah, that seems a bit overthinking it.  Adding it when warnings are enabled should be sufficient.  Alternatively, we can just add a "-Wno-error" like the extant cases.

> seems a lot easier to fix the
> origin, especially when enabling warnings has a proven history of uncovering
> real bugs.
> 
> > You say, you've looked for, but not found, a
> > bug-reason why the warnings appear, in this generated code, presumably
> > including looking at the generator.
> 
> the generator appears to be cgen lisp which uses the lisp cpu definitions. 
> the cgen code is not easy to follow at all, or trace back which funcs
> generate which lines.  i was hoping you, as author of much of this code,
> would be able to pick out & fix things much quicker than i.

Fair enough.  I had a quick look.

First a look at the apparent sites of the warnings (N.B.: only the ones quoted in this PR, I don't have a new enough gcc to emit those particular ones).  The *immediate* causes for each and every one of those, i.e. why "insn" isn't used, is that the insn in the self-named variable is fully decoded for the cases corresponding to the warnings, and no sub-fields need to be inspected.  Any operand is in the next 16- or 32-bit word.  The *immediate* cause is hereby analyzed as innocuous and AFAIU typical for auto-generated code. ;)

CGEN apparently emits a generic template containing "CGEN_INSN_WORD insn = base_insn;".  That template can be improved with a "generic" use of the variable insn, like appending a "(void) insn;" in the generated code, with a suitable comment nearby in the generator, like 'The insn may be fully decoded at this point.  Add an artificial use to avoid compiler warnings about insn not being used.'.

But, for that kind of change, this PR should be redirected to that project, as the template is there, in cgen/sim-decode.scm according to git grep.

(FAOD: the above is merely a suggestion; not volunteering myself.  I'd go with the -Wno-error.)

I tend to think the cris one is a bug in cgen.
In particular /gen-extract-case does:

   (if (> (length (sfmt-iflds sfmt)) 0)
       (string-append
	"    CGEN_INSN_WORD insn = "
	(if (adata-integral-insn? CURRENT-ARCH)
	    "entire_insn;\n"
	    "base_insn;\n"))
       "")

... but I think in this case, because the ifield
is constant, later code doesn't emit any use of 'insn'.

(In reply to Tom Tromey from comment #6)
> I tend to think the cris one is a bug in cgen.

I won't comment on the bugginess part, but it's a valid view, and certainly so if cgen strives to generate warning-free code.

> In particular /gen-extract-case does:
> 
>    (if (> (length (sfmt-iflds sfmt)) 0)
>        (string-append
> 	"    CGEN_INSN_WORD insn = "
> 	(if (adata-integral-insn? CURRENT-ARCH)
> 	    "entire_insn;\n"
> 	    "base_insn;\n"))
>        "")
> 
> ... but I think in this case, because the ifield
> is constant, later code doesn't emit any use of 'insn'.

IOW, but more precise about the location, thanks.
(And actually "more correct", as I looked at /gen-decode-fn.)

To wit along my suggestion above, hacked (or solved) by adding "(void) insn;" as another parameter to the string-append call (if I RTF-guile-M correctly).  Plus at least the suggested comment to aid casual readers.

(In reply to Hans-Peter Nilsson from comment #5)
> I don't think that's necessary: warnings are only enabled for gcc

they're enabled for toolchains that define __GNUC__.  llvm is known to do this too.  i don't know the current status, but Intel's ICC has done this in the past.  neither of those are gcc, and i don't like our build binding to CLI behavior.

> Nah, that seems a bit overthinking it.  Adding it when warnings are enabled
> should be sufficient.  Alternatively, we can just add a "-Wno-error" like
> the extant cases.

-Wno-error makes it even easier for things to slip in.  there are plenty of examples of this across software projects, including the sim.

> Developers can be expected to have a sufficiently modern gcc
> ...
> I don't have a new enough gcc to emit those particular ones

one person's modern is another's too-old ;)

> But, for that kind of change, this PR should be redirected to that project,
> as the template is there, in cgen/sim-decode.scm according to git grep.

sure, if we think the right place is cgen, i can send it there.  i wasn't sure if it was something we'd do entirely there, or if we'd change cpu/cris.cpu, or both.

ideally there would be something in the cpu definition that cgen could key of off (like the arguments passed down, or what implicitly requested more fields be decoded, or something else).  if that's not feasible, then i'd go with:

--- a/sim-decode.scm
+++ b/sim-decode.scm
@@ -436,7 +436,14 @@ void
    "    const IDESC *idesc = &" IDESC-TABLE-VAR "[itype];\n"
    (if (> (length (sfmt-iflds sfmt)) 0)
        (string-append
-        "    CGEN_INSN_WORD insn = "
+        "    "
+        ; Some insns are fully decoded by the time they get here, so they won't
+        ; access the insn variable.  Mark it as unused to avoid warnings.
+        and don't need the i
+        (if (adata-integral-insn? CURRENT-ARCH)
+            ""
+            "ATTRIBUTE_UNUSED ")
+        "CGEN_INSN_WORD insn = "
         (if (adata-integral-insn? CURRENT-ARCH)
             "entire_insn;\n"
             "base_insn;\n"))

(In reply to Mike Frysinger from comment #8)
> (In reply to Hans-Peter Nilsson from comment #5)
> > Developers can be expected to have a sufficiently modern gcc
> > ...
> > I don't have a new enough gcc to emit those particular ones
> 
> one person's modern is another's too-old ;)

Shades of grey and no absolutes...  New, but not new enough for the warnings *you* see.  Some put gcc-10 in the "oldstable", soon enough "oldoldstable".  When I compiled those files, I saw *other* warnings, some for uninitialized variables, but probably false-positive-warning-bugs in that gcc version.

> > But, for that kind of change, this PR should be redirected to that project,
> > as the template is there, in cgen/sim-decode.scm according to git grep.
> 
> sure, if we think the right place is cgen, i can send it there.  i wasn't
> sure if it was something we'd do entirely there, or if we'd change
> cpu/cris.cpu, or both.
> 
> ideally there would be something in the cpu definition that cgen could key
> of off (like the arguments passed down, or what implicitly requested more
> fields be decoded, or something else).  if that's not feasible, then i'd go
> with:
> 
> --- a/sim-decode.scm
> +++ b/sim-decode.scm
> @@ -436,7 +436,14 @@ void
>     "    const IDESC *idesc = &" IDESC-TABLE-VAR "[itype];\n"
>     (if (> (length (sfmt-iflds sfmt)) 0)
>         (string-append
> -        "    CGEN_INSN_WORD insn = "
> +        "    "
> +        ; Some insns are fully decoded by the time they get here, so they
> won't
> +        ; access the insn variable.  Mark it as unused to avoid warnings.
> +        and don't need the i

That last line seems superfluous and should yield errors.  (Accidentally quoted an untested diff?)

> +        (if (adata-integral-insn? CURRENT-ARCH)
> +            ""
> +            "ATTRIBUTE_UNUSED ")

Oh, they have "ATTRIBUTE_UNUSED".  Ok, that settles the question whether the cgen project cares about unused variables: they do.  Beware that (IIRC) not all compilers (old gcc, perhaps *cough* clang) support the same *location* for the attribute (here, before the type).  The canonical position is after; "before the ';'".  I see gcc-10 accepts it where you put it though, so I'm good.

> +        "CGEN_INSN_WORD insn = "
>          (if (adata-integral-insn? CURRENT-ARCH)
>              "entire_insn;\n"
>              "base_insn;\n"))

I don't know whether the "adata-integral-insn?" predicate has bearing on whether insn can be "unused". Still: besides the superfluous line, I'd say time to post it for review, with a link to this PR.

(In reply to Hans-Peter Nilsson from comment #9)
> That last line seems superfluous and should yield errors.  (Accidentally
> quoted an untested diff?)

yeah i didn't delete my partial comment ... already fixed it when i posted to the cgen mailing list

> Beware that (IIRC) not
> all compilers (old gcc, perhaps *cough* clang) support the same *location*
> for the attribute (here, before the type).  The canonical position is after;
> "before the ';'".

ok, easy enough to update

> > +        "CGEN_INSN_WORD insn = "
> >          (if (adata-integral-insn? CURRENT-ARCH)
> >              "entire_insn;\n"
> >              "base_insn;\n"))
> 
> I don't know whether the "adata-integral-insn?" predicate has bearing on
> whether insn can be "unused". Still: besides the superfluous line, I'd say
> time to post it for review, with a link to this PR.

i have no idea about adata-integral-insn here.  i went with it because all the unused warnings are coming from insn=base_insn, not insn=entire_insn, and adata-integral-insn controls whether it's entire_insn or base_insn.

i'll cc you on the v2 patch on the cgen list.

(In reply to Mike Frysinger from comment #10)
> i'll cc you on the v2 patch on the cgen list.

Thanks, for future reference please use either the axis or sourceware address for issues related to CRIS.

Maybe the cgen people will tell you to just insert "ATTRIBUTE_UNUSED" into the string between "base_insn and ;\n", as the new adata-integral-insn? call is unnecessary, iff it's also key to getting the unused-variable effect and isn't just an incidental match.

pushed in cgen & regen our sim tree