On 2022/9/28 1:02, Nathan Chancellor wrote:
On Tue, Sep 27, 2022 at 09:45:17AM -0700, Nick Desaulniers wrote:
On Tue, Sep 27, 2022 at 8:15 AM Zeng Heng <zengheng4@xxxxxxxxxx> wrote:
Provide implicit enum-conversion warning option
in general build. When it set enabled, it can
detect implicit enum type conversion and find
potential conversion errors like below
(use "allmodconfig"):
drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/display_mode_vba_20.c:3904:46:
error: implicit conversion from ‘enum <anonymous>’ to ‘enum odm_combine_mode’ [-Werror=enum-conversion]
3904 | locals->ODMCombineEnablePerState[i][k] = true;
| ^
The '-Wenum-conversion' could be regarded as
effective check on compile runtime and
call attention on potential mistakes.
Anothor practical example could be referred to:
https://lore.kernel.org/all/CADnq5_OE0yZvEYGu82QJHL9wvVcTFZrmeTgX7URgh7FVA=jqYg@xxxxxxxxxxxxxx
"-Wenum-conversion" was firstly introduced from
GNU gcc-10.
What about clang? ;)
Although "-Wenum-conversion" could be enabled
by "-Wextra" when compiling with 'W=1' option,
there are many warnings generated by '-Wextra'
that cause too much noise in a build.
With clang, I believe that -Wenum-conversion is part of -Wall or
-Wextra; so enabling this explicitly is only necessary for GCC. I
wonder why it's not part of -Wall or -Wextra for GCC? Perhaps worth a
bug report/feature request?
With clang, -Wenum-conversion is just default enabled, not even behind
-Wall:
$ cat test.c
enum enum1 { A = 1 };
enum enum2 { B = 2 };
enum enum1 foo(enum enum2 bar)
{
return bar;
}
$ clang -fsyntax-only test.c
test.c:11:9: warning: implicit conversion from enumeration type 'enum enum2' to different enumeration type 'enum enum1' [-Wenum-conversion]
return bar;
~~~~~~ ^~~
1 warning generated.
On the other hand, GCC does have it under -Wextra:
$ gcc -fsyntax-only test.c
$ gcc -Wextra -fsyntax-only test.c
test.c: In function ‘foo’:
test.c:6:16: warning: implicit conversion from ‘enum enum2’ to ‘enum enum1’ [-Wenum-conversion]
6 | return bar;
| ^~~
But the kernel does not build with -Wextra aside from W=[123], hence
this warning has to be explicitly requested for GCC.
Thanks to your replenish about clang.
Seeing the details from the following link:
https://gcc.gnu.org/onlinedocs/gcc-11.3.0/gcc/Warning-Options.html
Because there are still some concerned warnings
exist, the patch marks the option disabled in default
for avoiding compile failed like using "allmodconfig".
But there is no dependency to avoid this getting enabled by allmodconfig
(such as 'depends on !COMPILE_TEST') so I don't see the point in the
current form; 'default n' does nothing to prevent it. Regardless, I
agree with Nick's sentiment below.
Signed-off-by: Zeng Heng <zengheng4@xxxxxxxxxx>
---
Makefile | 5 +++++
lib/Kconfig.debug | 7 +++++++
2 files changed, 12 insertions(+)
diff --git a/Makefile b/Makefile
index ebd48fc956a3..1790a3624358 100644
--- a/Makefile
+++ b/Makefile
@@ -880,6 +880,11 @@ endif
KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
+# check implicit enum conversion
+ifdef CONFIG_ENUM_CONVERSION
+KBUILD_CFLAGS += -Wenum-conversion
+endif
Having a kconfig for this is overkill. cc-option with a comment about
the compiler default versions is the way to go.
Got it.
Agreed. If there is some reason -Wenum-conversion cannot be enabled for
GCC right now (such as existing warnings, which the commit message
appears to alude to), they should be cleaned up first then
-Wenum-conversion should just be unconditionally enabled for all
compilers that support it via cc-option, not half enabled via Kconfig so
that maybe people will clean up the warnings. That is not how enabling
warnings works:
https://lore.kernel.org/CAHk-=wg-mH-_GYpkhz_psjBWG6ZcjKnPo83fg7YMj_by+-LRTQ@xxxxxxxxxxxxxx/
Have sent the patches to fix the involving warnings.
If all the concerned warnings were repaired, I would send the v2 again.
Thanks all.
+
# These result in bogus false positives
KBUILD_CFLAGS += $(call cc-disable-warning, dangling-pointer)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4f2b81229a2f..a64e06a747d8 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -417,6 +417,13 @@ config FRAME_WARN
Setting this too low will cause a lot of warnings.
Setting it to 0 disables the warning.
+config ENUM_CONVERSION
+ bool "Warn for implicit enum conversion"
+ depends on GCC_VERSION >= 100300
+ default n
+ help
+ Tell gcc to warn at build time for implicit enum conversion.
+
config STRIP_ASM_SYMS
bool "Strip assembler-generated symbols during link"
default n
--
2.25.1
--
Thanks,
~Nick Desaulniers
