diff mbox series

kbuild: Disable -Wpointer-to-enum-cast

Message ID 20200308073400.23398-1-natechancellor@gmail.com (mailing list archive)
State New, archived
Headers show
Series kbuild: Disable -Wpointer-to-enum-cast | expand

Commit Message

Nathan Chancellor March 8, 2020, 7:34 a.m. UTC 
Clang's -Wpointer-to-int-cast deviates from GCC in that it warns when
casting to enums. The kernel does this in certain places, such as device
tree matches to set the version of the device being used, which allows
the kernel to avoid using a gigantic union.

https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L428
https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L402
https://elixir.bootlin.com/linux/v5.5.8/source/include/linux/mod_devicetable.h#L264

To avoid a ton of false positive warnings, disable this particular part
of the warning, which has been split off into a separate diagnostic so
that the entire warning does not need to be turned off for clang.

Cc: stable@vger.kernel.org
Link: https://github.com/ClangBuiltLinux/linux/issues/887
Link: https://github.com/llvm/llvm-project/commit/2a41b31fcdfcb67ab7038fc2ffb606fd50b83a84
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 Makefile | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Masahiro Yamada March 9, 2020, 2:11 a.m. UTC | #1 
Hi Nathan,

On Sun, Mar 8, 2020 at 4:34 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang's -Wpointer-to-int-cast deviates from GCC in that it warns when
> casting to enums. The kernel does this in certain places, such as device
> tree matches to set the version of the device being used, which allows
> the kernel to avoid using a gigantic union.
>
> https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L428
> https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L402
> https://elixir.bootlin.com/linux/v5.5.8/source/include/linux/mod_devicetable.h#L264
>
> To avoid a ton of false positive warnings, disable this particular part
> of the warning, which has been split off into a separate diagnostic so
> that the entire warning does not need to be turned off for clang.
>
> Cc: stable@vger.kernel.org
> Link: https://github.com/ClangBuiltLinux/linux/issues/887
> Link: https://github.com/llvm/llvm-project/commit/2a41b31fcdfcb67ab7038fc2ffb606fd50b83a84
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 86035d866f2c..90e56d5657c9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -748,6 +748,10 @@ KBUILD_CFLAGS += -Wno-tautological-compare
>  # source of a reference will be _MergedGlobals and not on of the whitelisted names.
>  # See modpost pattern 2
>  KBUILD_CFLAGS += -mno-global-merge
> +# clang's -Wpointer-to-int-cast warns when casting to enums, which does not match GCC.
> +# Disable that part of the warning because it is very noisy across the kernel and does
> +# not point out any real bugs.
> +KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
>  else



I'd rather want to fix all the call-sites (97 drivers?)
instead of having -Wno-pointer-to-enum-cast forever.

If it is tedious to fix them all for now, can we add it
into scripts/Makefile.extrawarn so that this is disabled
by default, but shows up with W=1 builds?

(When we fix most of them, we will be able to
make it a real warning.)


What do you think?

Thanks.




>  # These warnings generated too much noise in a regular build.
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200308073400.23398-1-natechancellor%40gmail.com.
Sasha Levin March 9, 2020, 12:25 p.m. UTC | #2 
Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.5.8, v5.4.24, v4.19.108, v4.14.172, v4.9.215, v4.4.215.

v5.5.8: Build OK!
v5.4.24: Build OK!
v4.19.108: Failed to apply! Possible dependencies:
    076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
    a1494304346a ("kbuild: add all Clang-specific flags unconditionally")

v4.14.172: Failed to apply! Possible dependencies:
    076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
    a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
    c6d6f4c55f5c ("MIPS: Always specify -EB or -EL when using clang")
    ee67855ecd9d ("MIPS: vdso: Allow clang's --target flag in VDSO cflags")

v4.9.215: Failed to apply! Possible dependencies:
    076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
    0e5e7f5e9700 ("powerpc/64: Reclaim CPU_FTR_SUBCORE")
    1421dc6d4829 ("powerpc/kbuild: Use flags variables rather than overriding LD/CC/AS")
    250122baed29 ("powerpc64/module: Tighten detection of mcount call sites with -mprofile-kernel")
    2a056f58fd33 ("powerpc: consolidate -mno-sched-epilog into FTRACE flags")
    3a849815a136 ("powerpc/book3s64: Always build for power4 or later")
    43c9127d94d6 ("powerpc: Add option to use thin archives")
    471d7ff8b51b ("powerpc/64s: Remove POWER4 support")
    5a61ef74f269 ("powerpc/64s: Support new device tree binding for discovering CPU features")
    6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on GCC 4.9 and newer")
    73aca179d78e ("powerpc/modules: Fix crashes by adding CONFIG_RELOCATABLE to vermagic")
    951eedebcdea ("powerpc/64: Handle linker stubs in low .text code")
    a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
    a73657ea19ae ("powerpc/64: Add GENERIC_CPU support for little endian")
    abba759796f9 ("powerpc/kbuild: move -mprofile-kernel check to Kconfig")
    b40b2386bce9 ("powerpc/Makefile: Fix ld version check with 64-bit LE-only toolchain")
    b71c9ffb1405 ("powerpc: Add arch/powerpc/tools directory")
    baa25b571a16 ("powerpc/64: Do not link crtsavres.o in vmlinux")
    badf436f6fa5 ("powerpc/Makefiles: Convert ifeq to ifdef where possible")
    c0d64cf9fefd ("powerpc: Use feature bit for RTC presence rather than timebase presence")
    c1807e3f8466 ("powerpc/64: Free up CPU_FTR_ICSWX")
    c6d6f4c55f5c ("MIPS: Always specify -EB or -EL when using clang")
    cf43d3b26452 ("powerpc: Enable pkey subsystem")
    ee67855ecd9d ("MIPS: vdso: Allow clang's --target flag in VDSO cflags")
    efe0160cfd40 ("powerpc/64: Linker on-demand sfpr functions for modules")
    f188d0524d7e ("powerpc: Use the new post-link pass to check relocations")

v4.4.215: Failed to apply! Possible dependencies:
    076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
    07e45c120c9c ("powerpc: Don't disable kernel FP/VMX/VSX MSR bits on context switch")
    152d523e6307 ("powerpc: Create context switch helpers save_sprs() and restore_sprs()")
    20dbe6706206 ("powerpc: Call restore_sprs() before _switch()")
    2a056f58fd33 ("powerpc: consolidate -mno-sched-epilog into FTRACE flags")
    3f3b5dc14c25 ("powerpc/pseries: PACA save area fix for general exception vs MCE")
    579e633e764e ("powerpc: create flush_all_to_thread()")
    57f266497d81 ("powerpc: Use gas sections for arranging exception vectors")
    6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on GCC 4.9 and newer")
    70fe3d980f5f ("powerpc: Restore FPU/VEC/VSX if previously used")
    8c50b72a3b4f ("powerpc/ftrace: Add Kconfig & Make glue for mprofile-kernel")
    951eedebcdea ("powerpc/64: Handle linker stubs in low .text code")
    a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
    a74599a50419 ("powerpc/pseries: PACA save area fix for MCE vs MCE")
    abba759796f9 ("powerpc/kbuild: move -mprofile-kernel check to Kconfig")
    c208505900b2 ("powerpc: create giveup_all()")
    c6d6f4c55f5c ("MIPS: Always specify -EB or -EL when using clang")
    caca285e5ab4 ("powerpc/mm/radix: Use STD_MMU_64 to properly isolate hash related code")
    d1e1cf2e38de ("powerpc: clean up asm/switch_to.h")
    d8d42b0511fe ("powerpc/64: Do load of PACAKBASE in LOAD_HANDLER")
    ee67855ecd9d ("MIPS: vdso: Allow clang's --target flag in VDSO cflags")
    f0f558b131db ("powerpc/mm: Preserve CFAR value on SLB miss caused by access to bogus address")
    f3d885ccba85 ("powerpc: Rearrange __switch_to()")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
Nathan Chancellor March 10, 2020, 1:14 a.m. UTC | #3 
On Mon, Mar 09, 2020 at 12:25:04PM +0000, Sasha Levin wrote:
> Hi
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
> 
> The bot has tested the following trees: v5.5.8, v5.4.24, v4.19.108, v4.14.172, v4.9.215, v4.4.215.
> 
> v5.5.8: Build OK!
> v5.4.24: Build OK!
> v4.19.108: Failed to apply! Possible dependencies:
>     076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
>     a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
> 
> v4.14.172: Failed to apply! Possible dependencies:
>     076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
>     a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
>     c6d6f4c55f5c ("MIPS: Always specify -EB or -EL when using clang")
>     ee67855ecd9d ("MIPS: vdso: Allow clang's --target flag in VDSO cflags")
> 
> v4.9.215: Failed to apply! Possible dependencies:
>     076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
>     0e5e7f5e9700 ("powerpc/64: Reclaim CPU_FTR_SUBCORE")
>     1421dc6d4829 ("powerpc/kbuild: Use flags variables rather than overriding LD/CC/AS")
>     250122baed29 ("powerpc64/module: Tighten detection of mcount call sites with -mprofile-kernel")
>     2a056f58fd33 ("powerpc: consolidate -mno-sched-epilog into FTRACE flags")
>     3a849815a136 ("powerpc/book3s64: Always build for power4 or later")
>     43c9127d94d6 ("powerpc: Add option to use thin archives")
>     471d7ff8b51b ("powerpc/64s: Remove POWER4 support")
>     5a61ef74f269 ("powerpc/64s: Support new device tree binding for discovering CPU features")
>     6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on GCC 4.9 and newer")
>     73aca179d78e ("powerpc/modules: Fix crashes by adding CONFIG_RELOCATABLE to vermagic")
>     951eedebcdea ("powerpc/64: Handle linker stubs in low .text code")
>     a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
>     a73657ea19ae ("powerpc/64: Add GENERIC_CPU support for little endian")
>     abba759796f9 ("powerpc/kbuild: move -mprofile-kernel check to Kconfig")
>     b40b2386bce9 ("powerpc/Makefile: Fix ld version check with 64-bit LE-only toolchain")
>     b71c9ffb1405 ("powerpc: Add arch/powerpc/tools directory")
>     baa25b571a16 ("powerpc/64: Do not link crtsavres.o in vmlinux")
>     badf436f6fa5 ("powerpc/Makefiles: Convert ifeq to ifdef where possible")
>     c0d64cf9fefd ("powerpc: Use feature bit for RTC presence rather than timebase presence")
>     c1807e3f8466 ("powerpc/64: Free up CPU_FTR_ICSWX")
>     c6d6f4c55f5c ("MIPS: Always specify -EB or -EL when using clang")
>     cf43d3b26452 ("powerpc: Enable pkey subsystem")
>     ee67855ecd9d ("MIPS: vdso: Allow clang's --target flag in VDSO cflags")
>     efe0160cfd40 ("powerpc/64: Linker on-demand sfpr functions for modules")
>     f188d0524d7e ("powerpc: Use the new post-link pass to check relocations")
> 
> v4.4.215: Failed to apply! Possible dependencies:
>     076f421da5d4 ("kbuild: replace cc-name test with CONFIG_CC_IS_CLANG")
>     07e45c120c9c ("powerpc: Don't disable kernel FP/VMX/VSX MSR bits on context switch")
>     152d523e6307 ("powerpc: Create context switch helpers save_sprs() and restore_sprs()")
>     20dbe6706206 ("powerpc: Call restore_sprs() before _switch()")
>     2a056f58fd33 ("powerpc: consolidate -mno-sched-epilog into FTRACE flags")
>     3f3b5dc14c25 ("powerpc/pseries: PACA save area fix for general exception vs MCE")
>     579e633e764e ("powerpc: create flush_all_to_thread()")
>     57f266497d81 ("powerpc: Use gas sections for arranging exception vectors")
>     6977f95e63b9 ("powerpc: avoid -mno-sched-epilog on GCC 4.9 and newer")
>     70fe3d980f5f ("powerpc: Restore FPU/VEC/VSX if previously used")
>     8c50b72a3b4f ("powerpc/ftrace: Add Kconfig & Make glue for mprofile-kernel")
>     951eedebcdea ("powerpc/64: Handle linker stubs in low .text code")
>     a1494304346a ("kbuild: add all Clang-specific flags unconditionally")
>     a74599a50419 ("powerpc/pseries: PACA save area fix for MCE vs MCE")
>     abba759796f9 ("powerpc/kbuild: move -mprofile-kernel check to Kconfig")
>     c208505900b2 ("powerpc: create giveup_all()")
>     c6d6f4c55f5c ("MIPS: Always specify -EB or -EL when using clang")
>     caca285e5ab4 ("powerpc/mm/radix: Use STD_MMU_64 to properly isolate hash related code")
>     d1e1cf2e38de ("powerpc: clean up asm/switch_to.h")
>     d8d42b0511fe ("powerpc/64: Do load of PACAKBASE in LOAD_HANDLER")
>     ee67855ecd9d ("MIPS: vdso: Allow clang's --target flag in VDSO cflags")
>     f0f558b131db ("powerpc/mm: Preserve CFAR value on SLB miss caused by access to bogus address")
>     f3d885ccba85 ("powerpc: Rearrange __switch_to()")
> 
> 
> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?
> 
> -- 
> Thanks
> Sasha

Hi Sasha,

If/when this patch hits mainline and I get the rejected emails from
Greg, I will send a manual backport.

Cheers,
Nathan
Nathan Chancellor March 10, 2020, 1:25 a.m. UTC | #4 
On Mon, Mar 09, 2020 at 11:11:05AM +0900, Masahiro Yamada wrote:
> Hi Nathan,
> 
> On Sun, Mar 8, 2020 at 4:34 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang's -Wpointer-to-int-cast deviates from GCC in that it warns when
> > casting to enums. The kernel does this in certain places, such as device
> > tree matches to set the version of the device being used, which allows
> > the kernel to avoid using a gigantic union.
> >
> > https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L428
> > https://elixir.bootlin.com/linux/v5.5.8/source/drivers/ata/ahci_brcm.c#L402
> > https://elixir.bootlin.com/linux/v5.5.8/source/include/linux/mod_devicetable.h#L264
> >
> > To avoid a ton of false positive warnings, disable this particular part
> > of the warning, which has been split off into a separate diagnostic so
> > that the entire warning does not need to be turned off for clang.
> >
> > Cc: stable@vger.kernel.org
> > Link: https://github.com/ClangBuiltLinux/linux/issues/887
> > Link: https://github.com/llvm/llvm-project/commit/2a41b31fcdfcb67ab7038fc2ffb606fd50b83a84
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  Makefile | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Makefile b/Makefile
> > index 86035d866f2c..90e56d5657c9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -748,6 +748,10 @@ KBUILD_CFLAGS += -Wno-tautological-compare
> >  # source of a reference will be _MergedGlobals and not on of the whitelisted names.
> >  # See modpost pattern 2
> >  KBUILD_CFLAGS += -mno-global-merge
> > +# clang's -Wpointer-to-int-cast warns when casting to enums, which does not match GCC.
> > +# Disable that part of the warning because it is very noisy across the kernel and does
> > +# not point out any real bugs.
> > +KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
> >  else
> 
> 
> 
> I'd rather want to fix all the call-sites (97 drivers?)
> instead of having -Wno-pointer-to-enum-cast forever.

Yes, there are 97 unique warnings across my builds, which are mainly
arm, arm64, and x86_64 defconfig/allmodconfig/allyesconfig:

https://github.com/ClangBuiltLinux/linux/issues/887#issuecomment-587938406

> If it is tedious to fix them all for now, can we add it
> into scripts/Makefile.extrawarn so that this is disabled
> by default, but shows up with W=1 builds?

Sure, I can send v2 to do that but I think that sending 97 patches just
casting the small values (usually less than twenty) to unsigned long
then to the enum is rather frivolous. I audited at least ten to fifteen
of these call sites when creating the clang patch and they are all
basically false positives.

I believe Nick discussed this with some other developers off list, maybe
he has some other feedback to give. I'll wait to send a v2 until
tomorrow in case anyone else has further comments.

> (When we fix most of them, we will be able to
> make it a real warning.)
> 
> 
> What do you think?
> 
> Thanks.

Cheers,
Nathan
David Laight March 10, 2020, 11:31 a.m. UTC | #5 
From: Nathan Chancellor
> Sent: 10 March 2020 01:26
...
> Sure, I can send v2 to do that but I think that sending 97 patches just
> casting the small values (usually less than twenty) to unsigned long
> then to the enum is rather frivolous. I audited at least ten to fifteen
> of these call sites when creating the clang patch and they are all
> basically false positives.

Such casts just make the code hard to read.
If misused casts can hide horrid bugs.
IMHO sprinkling the code with casts just to remove
compiler warnings will bite back one day.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Masahiro Yamada March 10, 2020, 3:30 p.m. UTC | #6 
On Tue, Mar 10, 2020 at 8:31 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Nathan Chancellor
> > Sent: 10 March 2020 01:26
> ...
> > Sure, I can send v2 to do that but I think that sending 97 patches just
> > casting the small values (usually less than twenty) to unsigned long
> > then to the enum is rather frivolous. I audited at least ten to fifteen
> > of these call sites when creating the clang patch and they are all
> > basically false positives.
>
> Such casts just make the code hard to read.
> If misused casts can hide horrid bugs.
> IMHO sprinkling the code with casts just to remove
> compiler warnings will bite back one day.
>

I agree that too much casts make the code hard to read,
but irrespective of this patch, there is no difference
in the fact that we need a cast to convert
(const void *) to a non-pointer value.

The difference is whether we use
(uintptr_t) or (enum foo).




If we want to avoid casts completely,
we could use union in struct of_device_id
although this might be rejected.


FWIW:

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 6853dbb4131d..534170bea134 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -415,11 +415,11 @@ static struct scsi_host_template ahci_platform_sht = {
 };

 static const struct of_device_id ahci_of_match[] = {
-       {.compatible = "brcm,bcm7425-ahci", .data = (void *)BRCM_SATA_BCM7425},
-       {.compatible = "brcm,bcm7445-ahci", .data = (void *)BRCM_SATA_BCM7445},
-       {.compatible = "brcm,bcm63138-ahci", .data = (void *)BRCM_SATA_BCM7445},
-       {.compatible = "brcm,bcm-nsp-ahci", .data = (void *)BRCM_SATA_NSP},
-       {.compatible = "brcm,bcm7216-ahci", .data = (void *)BRCM_SATA_BCM7216},
+       {.compatible = "brcm,bcm7425-ahci", .data2 = BRCM_SATA_BCM7425},
+       {.compatible = "brcm,bcm7445-ahci", .data2 = BRCM_SATA_BCM7445},
+       {.compatible = "brcm,bcm63138-ahci", .data2 = BRCM_SATA_BCM7445},
+       {.compatible = "brcm,bcm-nsp-ahci", .data2 = BRCM_SATA_NSP},
+       {.compatible = "brcm,bcm7216-ahci", .data2 = BRCM_SATA_BCM7216},
        {},
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
@@ -442,7 +442,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
        if (!of_id)
                return -ENODEV;

-       priv->version = (enum brcm_ahci_version)of_id->data;
+       priv->version = of_id->data2;
        priv->dev = dev;

        res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "top-ctrl");
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index e3596db077dc..98d44ebf146a 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -261,7 +261,10 @@ struct of_device_id {
        char    name[32];
        char    type[32];
        char    compatible[128];
-       const void *data;
+       union {
+               const void *data;
+               unsigned long data2;
+       };
 };

 /* VIO */
Nick Desaulniers March 10, 2020, 4:16 p.m. UTC | #7 
+ Saravana, who I spoke to briefly about this.


On Tue, Mar 10, 2020 at 8:32 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Mar 10, 2020 at 8:31 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Nathan Chancellor
> > > Sent: 10 March 2020 01:26
> > ...
> > > Sure, I can send v2 to do that but I think that sending 97 patches just
> > > casting the small values (usually less than twenty) to unsigned long
> > > then to the enum is rather frivolous. I audited at least ten to fifteen
> > > of these call sites when creating the clang patch and they are all
> > > basically false positives.
> >
> > Such casts just make the code hard to read.
> > If misused casts can hide horrid bugs.
> > IMHO sprinkling the code with casts just to remove
> > compiler warnings will bite back one day.
> >
>
> I agree that too much casts make the code hard to read,
> but irrespective of this patch, there is no difference
> in the fact that we need a cast to convert
> (const void *) to a non-pointer value.
>
> The difference is whether we use
> (uintptr_t) or (enum foo).
>
>
>
>
> If we want to avoid casts completely,
> we could use union in struct of_device_id
> although this might be rejected.
>
>
> FWIW:
>
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 6853dbb4131d..534170bea134 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -415,11 +415,11 @@ static struct scsi_host_template ahci_platform_sht = {
>  };
>
>  static const struct of_device_id ahci_of_match[] = {
> -       {.compatible = "brcm,bcm7425-ahci", .data = (void *)BRCM_SATA_BCM7425},
> -       {.compatible = "brcm,bcm7445-ahci", .data = (void *)BRCM_SATA_BCM7445},
> -       {.compatible = "brcm,bcm63138-ahci", .data = (void *)BRCM_SATA_BCM7445},
> -       {.compatible = "brcm,bcm-nsp-ahci", .data = (void *)BRCM_SATA_NSP},
> -       {.compatible = "brcm,bcm7216-ahci", .data = (void *)BRCM_SATA_BCM7216},
> +       {.compatible = "brcm,bcm7425-ahci", .data2 = BRCM_SATA_BCM7425},
> +       {.compatible = "brcm,bcm7445-ahci", .data2 = BRCM_SATA_BCM7445},
> +       {.compatible = "brcm,bcm63138-ahci", .data2 = BRCM_SATA_BCM7445},
> +       {.compatible = "brcm,bcm-nsp-ahci", .data2 = BRCM_SATA_NSP},
> +       {.compatible = "brcm,bcm7216-ahci", .data2 = BRCM_SATA_BCM7216},
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, ahci_of_match);
> @@ -442,7 +442,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>         if (!of_id)
>                 return -ENODEV;
>
> -       priv->version = (enum brcm_ahci_version)of_id->data;
> +       priv->version = of_id->data2;
>         priv->dev = dev;
>
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "top-ctrl");
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index e3596db077dc..98d44ebf146a 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -261,7 +261,10 @@ struct of_device_id {
>         char    name[32];
>         char    type[32];
>         char    compatible[128];
> -       const void *data;
> +       union {
> +               const void *data;
> +               unsigned long data2;
> +       };
>  };
>
Joe Perches March 10, 2020, 4:48 p.m. UTC | #8 
On Wed, 2020-03-11 at 00:30 +0900, Masahiro Yamada wrote:
> On Tue, Mar 10, 2020 at 8:31 PM David Laight <David.Laight@aculab.com> wrote:
> > From: Nathan Chancellor
> > > Sent: 10 March 2020 01:26
> > ...
> > > Sure, I can send v2 to do that but I think that sending 97 patches just
> > > casting the small values (usually less than twenty) to unsigned long
> > > then to the enum is rather frivolous. I audited at least ten to fifteen
> > > of these call sites when creating the clang patch and they are all
> > > basically false positives.
> > 
> > Such casts just make the code hard to read.
> > If misused casts can hide horrid bugs.
> > IMHO sprinkling the code with casts just to remove
> > compiler warnings will bite back one day.
> > 
> 
> I agree that too much casts make the code hard to read,
> but irrespective of this patch, there is no difference
> in the fact that we need a cast to convert
> (const void *) to a non-pointer value.
> 
> The difference is whether we use
> (uintptr_t) or (enum foo).

or hide it altogether in a macro like cast_to

#define cast_to(type, val)	\
	etc...

where cast_to could do the appropriate
intermediate cast if type is an enum
and sizeof(tupeof val) is larger than int
Saravana Kannan March 10, 2020, 6:20 p.m. UTC | #9 
On Tue, Mar 10, 2020 at 9:16 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + Saravana, who I spoke to briefly about this.
>
>
> On Tue, Mar 10, 2020 at 8:32 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Tue, Mar 10, 2020 at 8:31 PM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Nathan Chancellor
> > > > Sent: 10 March 2020 01:26
> > > ...
> > > > Sure, I can send v2 to do that but I think that sending 97 patches just
> > > > casting the small values (usually less than twenty) to unsigned long
> > > > then to the enum is rather frivolous. I audited at least ten to fifteen
> > > > of these call sites when creating the clang patch and they are all
> > > > basically false positives.
> > >
> > > Such casts just make the code hard to read.
> > > If misused casts can hide horrid bugs.
> > > IMHO sprinkling the code with casts just to remove
> > > compiler warnings will bite back one day.
> > >
> >
> > I agree that too much casts make the code hard to read,
> > but irrespective of this patch, there is no difference
> > in the fact that we need a cast to convert
> > (const void *) to a non-pointer value.
> >
> > The difference is whether we use
> > (uintptr_t) or (enum foo).
> >
> >
> >
> >
> > If we want to avoid casts completely,
> > we could use union in struct of_device_id
> > although this might be rejected.

The union like you suggested might fly. Maybe the new field data_ulong
or data_u32 might work and even help non-enum non-pointer values to be
stored in this directly too without needing the casting that's needed
today.

I still don't get why the compiler can't be smarter about this. If the
enum would fit inside the pointer, why not leave that alone and throw
a warning only when the enum really can overflow the pointer field?

> >
> >
> > FWIW:
> >
> > diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> > index 6853dbb4131d..534170bea134 100644
> > --- a/drivers/ata/ahci_brcm.c
> > +++ b/drivers/ata/ahci_brcm.c
> > @@ -415,11 +415,11 @@ static struct scsi_host_template ahci_platform_sht = {
> >  };
> >
> >  static const struct of_device_id ahci_of_match[] = {
> > -       {.compatible = "brcm,bcm7425-ahci", .data = (void *)BRCM_SATA_BCM7425},
> > -       {.compatible = "brcm,bcm7445-ahci", .data = (void *)BRCM_SATA_BCM7445},
> > -       {.compatible = "brcm,bcm63138-ahci", .data = (void *)BRCM_SATA_BCM7445},
> > -       {.compatible = "brcm,bcm-nsp-ahci", .data = (void *)BRCM_SATA_NSP},
> > -       {.compatible = "brcm,bcm7216-ahci", .data = (void *)BRCM_SATA_BCM7216},
> > +       {.compatible = "brcm,bcm7425-ahci", .data2 = BRCM_SATA_BCM7425},
> > +       {.compatible = "brcm,bcm7445-ahci", .data2 = BRCM_SATA_BCM7445},
> > +       {.compatible = "brcm,bcm63138-ahci", .data2 = BRCM_SATA_BCM7445},
> > +       {.compatible = "brcm,bcm-nsp-ahci", .data2 = BRCM_SATA_NSP},
> > +       {.compatible = "brcm,bcm7216-ahci", .data2 = BRCM_SATA_BCM7216},
> >         {},
> >  };
> >  MODULE_DEVICE_TABLE(of, ahci_of_match);
> > @@ -442,7 +442,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> >         if (!of_id)
> >                 return -ENODEV;
> >
> > -       priv->version = (enum brcm_ahci_version)of_id->data;
> > +       priv->version = of_id->data2;
> >         priv->dev = dev;
> >
> >         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "top-ctrl");
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index e3596db077dc..98d44ebf146a 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -261,7 +261,10 @@ struct of_device_id {
> >         char    name[32];
> >         char    type[32];
> >         char    compatible[128];
> > -       const void *data;
> > +       union {
> > +               const void *data;
> > +               unsigned long data2;
> > +       };
> >  };
> >

I've never (or long since forgotten) consciously declared a union
without a name and directly accessed it's fields. If this compiles,
this seems reasonable.

-Saravana
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 86035d866f2c..90e56d5657c9 100644
--- a/Makefile
+++ b/Makefile
@@ -748,6 +748,10 @@  KBUILD_CFLAGS += -Wno-tautological-compare
 # source of a reference will be _MergedGlobals and not on of the whitelisted names.
 # See modpost pattern 2
 KBUILD_CFLAGS += -mno-global-merge
+# clang's -Wpointer-to-int-cast warns when casting to enums, which does not match GCC.
+# Disable that part of the warning because it is very noisy across the kernel and does
+# not point out any real bugs.
+KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
 else
 
 # These warnings generated too much noise in a regular build.