[Bug sim/31181] New: sim: cris: decode unused base_insn variable warnings

[Bug sim/31181] New: sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

            Bug ID: 31181
           Summary: sim: cris: decode unused base_insn variable warnings
           Product: gdb
           Version: unknown
            Status: NEW
          Severity: normal
          Priority: P2
         Component: sim
          Assignee: unassigned at sourceware dot org
          Reporter: vapier at gentoo dot org
                CC: hp at bitrange dot com, vapier at gentoo dot org
  Target Milestone: ---

can't figure out what needs to change in the sources to fix the generated code

  CC       cris/decodev32.o
../../../sim/cris/decodev32.c: In function ‘crisv32f_decode’:
../../../sim/cris/decodev32.c:4526:20: error: unused variable ‘insn’
[-Werror=unused-variable]
 4526 |     CGEN_INSN_WORD insn = base_insn;
      |                    ^~~~
cc1: all warnings being treated as errors
make[2]: *** [Makefile:3959: cris/decodev32.o] Error 1

  CC       cris/decodev10.o
../../../sim/cris/decodev10.c: In function ‘crisv10f_decode’:
../../../sim/cris/decodev10.c:3857:20: error: unused variable ‘insn’
[-Werror=unused-variable]
 3857 |     CGEN_INSN_WORD insn = base_insn;
      |                    ^~~~
../../../sim/cris/decodev10.c:4949:20: error: unused variable ‘insn’
[-Werror=unused-variable]
 4949 |     CGEN_INSN_WORD insn = base_insn;
      |                    ^~~~
../../../sim/cris/decodev10.c:5357:20: error: unused variable ‘insn’
[-Werror=unused-variable]
 5357 |     CGEN_INSN_WORD insn = base_insn;
      |                    ^~~~
../../../sim/cris/decodev10.c:5645:20: error: unused variable ‘insn’
[-Werror=unused-variable]
 5645 |     CGEN_INSN_WORD insn = base_insn;
      |                    ^~~~
cc1: all warnings being treated as errors
make[2]: *** [Makefile:3959: cris/decodev10.o] Error 1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #1 from Hans-Peter Nilsson <hp at sourceware dot org> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #2 from Mike Frysinger <vapier at gentoo dot org> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #3 from Hans-Peter Nilsson <hp at sourceware dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #4 from Mike Frysinger <vapier at gentoo dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #5 from Hans-Peter Nilsson <hp at sourceware dot org> ---
(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.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

--- Comment #6 from Tom Tromey <tromey at sourceware dot org> ---
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'.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #7 from Hans-Peter Nilsson <hp at sourceware dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #8 from Mike Frysinger <vapier at gentoo dot org> ---
(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"))

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #9 from Hans-Peter Nilsson <hp at sourceware dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #10 from Mike Frysinger <vapier at gentoo dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

Hans-Peter Nilsson <hp at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|hp at bitrange dot com             |hp at sourceware dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[hp at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

--- Comment #11 from Hans-Peter Nilsson <hp at sourceware dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

Mike Frysinger <vapier at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at sourceware dot org   |vapier at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug sim/31181] sim: cris: decode unused base_insn variable warnings

[vapier at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31181

Mike Frysinger <vapier at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Mike Frysinger <vapier at gentoo dot org> ---
pushed in cgen & regen our sim tree

-- 
You are receiving this mail because:
You are on the CC list for the bug.