On 21.11.2015 00:43, Stefan Tauner wrote:
Compilers are free to use an unsigned data type for enumerations without negative elements. Our enumeration of all programmers is one such instance. Also, compilers will warn about comparisons of unsigned enum expressions < 0.
If we enable only a single programmer then PROGRAMMER_INVALID will be 1. We do some run-time checks of loop counters of an unsigned enumaration type against PROGRAMMER_INVALID in the form of if (p < PROGRAMMER_INVALID - 1) which get optimized to if (p < 0) This makes the check always false and the compiler warnings compulsory.
Which gcc version is this?
This patch adds #ifdefs guarding these checks if there is only one programmer enabled.
One can enable a single programmer by running grep CONFIG_ Makefile |fgrep '?= yes'|grep -vi serprog |sed "s/ .*/=no/"|xargs make CONFIG_SERPROG=yes
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
flashrom.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/flashrom.c b/flashrom.c index c9c7e31..abb917b 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1633,10 +1633,13 @@ void list_programmers(const char *delim) enum programmer p; for (p = 0; p < PROGRAMMER_INVALID; p++) { msg_ginfo("%s", programmer_table[p].name);
+#if PROGRAMMER_INVALID > 1 if (p < PROGRAMMER_INVALID - 1)
Does the warning also disappear if you write if ((PROGRAMMER_INVALID > 1) && (p < PROGRAMMER_INVALID - 1) ) instead? AFAICS that should placate gcc (since this is a constant false expression for PROGRAMMER_INVALID==1) even with warnings switched on.
msg_ginfo("%s", delim);+#endif }
- msg_ginfo("\n");
- msg_ginfo("\n");
}
void list_programmers_linebreak(int startcol, int cols, int paren) @@ -1668,6 +1671,7 @@ void list_programmers_linebreak(int startcol, int cols, int paren) } msg_ginfo("%s", pname); remaining -= pnamelen; +#if PROGRAMMER_INVALID > 1 if (p < PROGRAMMER_INVALID - 1) { msg_ginfo(","); remaining--; @@ -1675,6 +1679,9 @@ void list_programmers_linebreak(int startcol, int cols, int paren) if (paren) msg_ginfo(")"); } +#else
msg_ginfo(")");
Please drop the complete #else block. It adds a ")" where none should be. It doesn't just look strange in the code, I also verified the output.
+#endif } }
Regards, Carl-Daniel
On Sat, 21 Nov 2015 09:29:33 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 21.11.2015 00:43, Stefan Tauner wrote:
Compilers are free to use an unsigned data type for enumerations without negative elements. Our enumeration of all programmers is one such instance. Also, compilers will warn about comparisons of unsigned enum expressions < 0.
If we enable only a single programmer then PROGRAMMER_INVALID will be 1. We do some run-time checks of loop counters of an unsigned enumaration type against PROGRAMMER_INVALID in the form of if (p < PROGRAMMER_INVALID - 1) which get optimized to if (p < 0) This makes the check always false and the compiler warnings compulsory.
Which gcc version is this?
clang :) 3.0 At least my gcc 4.6.3 does not bother to warn and/or make the enum unsigned - at least not with our default CFLAGS.
This patch adds #ifdefs guarding these checks if there is only one programmer enabled.
One can enable a single programmer by running grep CONFIG_ Makefile |fgrep '?= yes'|grep -vi serprog |sed "s/ .*/=no/"|xargs make CONFIG_SERPROG=yes
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
flashrom.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/flashrom.c b/flashrom.c index c9c7e31..abb917b 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1633,10 +1633,13 @@ void list_programmers(const char *delim) enum programmer p; for (p = 0; p < PROGRAMMER_INVALID; p++) { msg_ginfo("%s", programmer_table[p].name);
+#if PROGRAMMER_INVALID > 1 if (p < PROGRAMMER_INVALID - 1)
Does the warning also disappear if you write if ((PROGRAMMER_INVALID > 1) && (p < PROGRAMMER_INVALID - 1) ) instead? AFAICS that should placate gcc (since this is a constant false expression for PROGRAMMER_INVALID==1) even with warnings switched on.
That's the first thing I tried, sadly without success.
msg_ginfo("%s", delim);+#endif }
- msg_ginfo("\n");
- msg_ginfo("\n");
}
void list_programmers_linebreak(int startcol, int cols, int paren) @@ -1668,6 +1671,7 @@ void list_programmers_linebreak(int startcol, int cols, int paren) } msg_ginfo("%s", pname); remaining -= pnamelen; +#if PROGRAMMER_INVALID > 1 if (p < PROGRAMMER_INVALID - 1) { msg_ginfo(","); remaining--; @@ -1675,6 +1679,9 @@ void list_programmers_linebreak(int startcol, int cols, int paren) if (paren) msg_ginfo(")"); } +#else
msg_ginfo(")");Please drop the complete #else block. It adds a ")" where none should be. It doesn't just look strange in the code, I also verified the output.
Will re-check that, thanks.
Now the question is, do we want that? or do we want to hack around that? E.g. with one negative value in the enumeration or casts?
Other workarounds:
- adding a dummy value to the enum with a negative value - clang pragmas to disable the warnings - a BUGS file
I am strictly against the pragmas because they litter the code too much. Carl-Daniel is against the -1 hack. I would not mind a BUGS file at all but I am also ok with just ignoring the issue. It does not happen in the default configuration and can easily be worked around by setting the CFLAGS (adding -Wno-tautological-compare), adding another programmer or using gcc instead...
On Sun, 22 Nov 2015 00:19:15 +0100 Stefan Tauner stefan.tauner@alumni.tuwien.ac.at wrote:
Other workarounds:
- adding a dummy value to the enum with a negative value
- clang pragmas to disable the warnings
- a BUGS file
I am strictly against the pragmas because they litter the code too much. Carl-Daniel is against the -1 hack. I would not mind a BUGS file at all but I am also ok with just ignoring the issue. It does not happen in the default configuration and can easily be worked around by setting the CFLAGS (adding -Wno-tautological-compare), adding another programmer or using gcc instead...
Just a quick follow up... the previous patch was complete bullshit... the preprocessor does not know anything about enum values and replaced all those identifiers with 0 removing all #if-guarded lines - which "solves" the wrong warnings... because the lines are removed completely. I'll ignore the issue of clang overreacting in this case.
-
Carl-Daniel Hailfinger -
Stefan Tauner