kbuild: Add option to turn incompatible pointer check into error

Commit Message

Daniel Wagner March 8, 2016, 8:29 a.m. UTC

From: Daniel Wagner <[e-mail]>

With the introduction of the simple wait API we have two very
similar APIs in the kernel. For example wake_up() and swake_up()
is only one character away. Although the compiler will warn
happily the wrong usage it keeps on going an even links the kernel.
Thomas and Peter would rather like to see early missuses reported
as error early on.

In a first attempt we tried to wrap all swait and wait calls
into a macro which has an compile time type assertion. The result
was pretty ugly and wasn't able to catch all wrong usages.
woken_wake_function(), autoremove_wake_function() and wake_bit_function()
are assigned as function pointers. Wrapping them with a macro around is
not possible. Prefixing them with '_' was also not a real option
because there some users in the kernel which do use them as well.
All in all this attempt looked to intrusive and too ugly.

An alternative is to turn the pointer type check into an error which
catches wrong type uses. Obviously not only the swait/wait ones. That
isn't a bad thing either.

Signed-off-by: Daniel Wagner <[e-mail]>
Acked-by: Peter Zijlstra (Intel) <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
---

Hi,

This patch was part of the swait patchset [1]. Ingo reverted it later
because:

"So adding -Werror=incompatible-pointer-types wasn't a bad idea, but it
should really not be done in the scheduler tree: it exposes us to a
number of pre-existing warnings (most of them harmless), now upgraded
to build failures...

This should be done via the kbuild tree."

I tried to build a bunch of different architectures and configurations
to find any hold offs. For those architecture I had a working
toolchain it looks good. All but one are fixed or on the way to
mainline.

Here is the list of archs and config I tried out. First run was with the
config listed, followed by a allyesconfig and allmodconfig build. 

#    name      ARCH     CROSS_COMPILE        config
declare -a config=(
    "alpha     alpha    alpha-linux-gnu-     defconfig"
    "arm32     arm      arm-linux-gnu-       versatile_defconfig"
    "arm64     arm64    aarch64-linux-gnu-   defconfig"
    "cris10    cris     cris-linux-gnu-      etrax-100lx_defconfig"
    "frv       frv      frv-linux-gnu-       defconfig"
    "ia64      ia64     ia64-linux-gnu-      generic_defconfig"
    "m68k      m68k     m68k-linux-gnu-      amiga_defconfig"
    "mips32    mips     mips64-linux-gnu-    ar7_defconfig"
    "mips64    mips     mips64-linux-gnu-    bigsur_defconfig"
    "ppc64     powerpc  powerpc64-linux-gnu- pseries_defconfig"
    "s390      s390     s390x-linux-gnu-     defconfig"
    "sparc32   sparc    sparc64-linux-gnu-   sparc32_defconfig"
    "sparc64   sparc    sparc64-linux-gnu-   sparc64_defconfig"
    "um_x86_64 um       gcc                  defconfig"
    "x86_64    x86_64   gcc                  defconfig"

As said above for the missing arch I didn't get hold on a working
toolchain with gcc >= 5.2.

On alpha I found [2]. So far no feedback on the patch. Propoably missing
the right CCs.

x86 is breaking for allyesconfig in gma500. The fix is linus-next [3].

cheers,
daniel

[1] http://thread.gmane.org/gmane.linux.kernel/2156371
[2] http://thread.gmane.org/gmane.linux.ports.alpha/3494
[3] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/diff/drivers/gpu/drm/gma500/mdfld_dsi_output.c?id=075f82f5db32848f5cbcdf5d7d6668b7c087b49f


Makefile | 3 +++
 1 file changed, 3 insertions(+)

Comments

Thomas Gleixner March 8, 2016, 9:10 a.m. UTC | #1

On Tue, 8 Mar 2016, Daniel Wagner wrote:

> From: Daniel Wagner <[e-mail]>
> 
> With the introduction of the simple wait API we have two very
> similar APIs in the kernel. For example wake_up() and swake_up()
> is only one character away. Although the compiler will warn
> happily the wrong usage it keeps on going an even links the kernel.
> Thomas and Peter would rather like to see early missuses reported
> as error early on.
> 
> In a first attempt we tried to wrap all swait and wait calls
> into a macro which has an compile time type assertion. The result
> was pretty ugly and wasn't able to catch all wrong usages.
> woken_wake_function(), autoremove_wake_function() and wake_bit_function()
> are assigned as function pointers. Wrapping them with a macro around is
> not possible. Prefixing them with '_' was also not a real option
> because there some users in the kernel which do use them as well.
> All in all this attempt looked to intrusive and too ugly.
> 
> An alternative is to turn the pointer type check into an error which
> catches wrong type uses. Obviously not only the swait/wait ones. That
> isn't a bad thing either.
> 
> Signed-off-by: Daniel Wagner <[e-mail]>
> Acked-by: Peter Zijlstra (Intel) <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>

Acked-by: Thomas Gleixner <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ingo Molnar March 8, 2016, 9:19 a.m. UTC | #2

* Thomas Gleixner <[e-mail]> wrote:

> On Tue, 8 Mar 2016, Daniel Wagner wrote:
> 
> > From: Daniel Wagner <[e-mail]>
> > 
> > With the introduction of the simple wait API we have two very
> > similar APIs in the kernel. For example wake_up() and swake_up()
> > is only one character away. Although the compiler will warn
> > happily the wrong usage it keeps on going an even links the kernel.
> > Thomas and Peter would rather like to see early missuses reported
> > as error early on.
> > 
> > In a first attempt we tried to wrap all swait and wait calls
> > into a macro which has an compile time type assertion. The result
> > was pretty ugly and wasn't able to catch all wrong usages.
> > woken_wake_function(), autoremove_wake_function() and wake_bit_function()
> > are assigned as function pointers. Wrapping them with a macro around is
> > not possible. Prefixing them with '_' was also not a real option
> > because there some users in the kernel which do use them as well.
> > All in all this attempt looked to intrusive and too ugly.
> > 
> > An alternative is to turn the pointer type check into an error which
> > catches wrong type uses. Obviously not only the swait/wait ones. That
> > isn't a bad thing either.
> > 
> > Signed-off-by: Daniel Wagner <[e-mail]>
> > Acked-by: Peter Zijlstra (Intel) <[e-mail]>
> > Cc: Thomas Gleixner <[e-mail]>
> 
> Acked-by: Thomas Gleixner <[e-mail]>

Note that there will be a few build failures triggered by this, for example this 
fix from linux-next is needed:

> commit db9b60400f9253c25ae639797df2d0ff7a35d9d8
> Author: Sudip Mukherjee <[e-mail]>
> Date:   Tue Feb 2 11:35:55 2016 +0530
>
>     drm/gma500: remove helper function

Other than that:

Acked-by: Ingo Molnar <[e-mail]>

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michal Marek March 15, 2016, 9:34 p.m. UTC | #3

Dne 8.3.2016 v 10:19 Ingo Molnar napsal(a):
> 
> * Thomas Gleixner <[e-mail]> wrote:
> 
>> On Tue, 8 Mar 2016, Daniel Wagner wrote:
>>
>>> From: Daniel Wagner <[e-mail]>
>>>
>>> With the introduction of the simple wait API we have two very
>>> similar APIs in the kernel. For example wake_up() and swake_up()
>>> is only one character away. Although the compiler will warn
>>> happily the wrong usage it keeps on going an even links the kernel.
>>> Thomas and Peter would rather like to see early missuses reported
>>> as error early on.
>>>
>>> In a first attempt we tried to wrap all swait and wait calls
>>> into a macro which has an compile time type assertion. The result
>>> was pretty ugly and wasn't able to catch all wrong usages.
>>> woken_wake_function(), autoremove_wake_function() and wake_bit_function()
>>> are assigned as function pointers. Wrapping them with a macro around is
>>> not possible. Prefixing them with '_' was also not a real option
>>> because there some users in the kernel which do use them as well.
>>> All in all this attempt looked to intrusive and too ugly.
>>>
>>> An alternative is to turn the pointer type check into an error which
>>> catches wrong type uses. Obviously not only the swait/wait ones. That
>>> isn't a bad thing either.
>>>
>>> Signed-off-by: Daniel Wagner <[e-mail]>
>>> Acked-by: Peter Zijlstra (Intel) <[e-mail]>
>>> Cc: Thomas Gleixner <[e-mail]>
>>
>> Acked-by: Thomas Gleixner <[e-mail]>

Applied to kbuild.git#kbuild.

> 
> Note that there will be a few build failures triggered by this, for example this 
> fix from linux-next is needed:
> 
>> commit db9b60400f9253c25ae639797df2d0ff7a35d9d8
>> Author: Sudip Mukherjee <[e-mail]>
>> Date:   Tue Feb 2 11:35:55 2016 +0530
>>
>>     drm/gma500: remove helper function

OK. I will send a pull request to Linus only after at least
x86_64/allyescofnig is fixed.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michal Marek March 15, 2016, 9:39 p.m. UTC | #4

Dne 15.3.2016 v 22:34 Michal Marek napsal(a):
> Dne 8.3.2016 v 10:19 Ingo Molnar napsal(a):
>> Note that there will be a few build failures triggered by this, for example this 
>> fix from linux-next is needed:
>>
>>> commit db9b60400f9253c25ae639797df2d0ff7a35d9d8
>>> Author: Sudip Mukherjee <[e-mail]>
>>> Date:   Tue Feb 2 11:35:55 2016 +0530
>>>
>>>     drm/gma500: remove helper function
> 
> OK. I will send a pull request to Linus only after at least
> x86_64/allyescofnig is fixed.

... which, incidentally, the gma500 file is not part of :).

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Makefile b/Makefile
index 2d519d2..a0ff507 100644
--- a/Makefile
+++ b/Makefile
@@ -773,6 +773,9 @@  KBUILD_CFLAGS   += $(call cc-option,-Werror=strict-prototypes)
 # Prohibit date/time macros, which would make the build non-deterministic
 KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)
 
+# enforce correct pointer usage
+KBUILD_CFLAGS   += $(call cc-option,-Werror=incompatible-pointer-types)
+
 # use the deterministic mode of AR if available
 KBUILD_ARFLAGS := $(call ar-option,D)