[PATCH 0/5] gcov fixes and maybe-uninitialized warnings

[PATCH 0/5] gcov fixes and maybe-uninitialized warnings

[Arnd Bergmann]

This series tries to address two related problems:

* getting better "variable may be used uninitialized" warnings
  in allmodconfig and randconfig builds

* improving GCOV_PROFILE support

Surprisingly, these two are related, so I have a single series that
I hope to get merged through the Kbuild tree.

I have built many thousands of ARM randconfig kernels and created
patches for every single warning and error I found, and submitted
most of them for inclusion. The "may be used uninitialized" warnings
are both the most annoying and the most helpful ones that gcc
gives us, so this tries to make them more useful, including three
steps:

1. Limit the false positives as much as we can: Aside from
   CC_OPTIMIZE_FOR_SIZE, three other options (UBSAN_SANITIZE_ALL,
   PROFILE_ALL_BRANCHES and GCOV_PROFILE_ALL) confuse the
   compiler (in versions 4.9 through 5.3, and to a lesser degree
   on older versions) so we get a lot of extra warnings. We
   already disable the warnings for CC_OPTIMIZE_FOR_SIZE, and
   this series also disables them for the other two, which are
   rarely used in practice but do show up in randconfig builds
   all the time.
2. Ensure we actually see them on 'allmodconfig' builds: Today they
   are disabled, because CC_OPTIMIZE_FOR_SIZE disables them.
   UBSAN_SANITIZE_ALL and PROFILE_ALL_BRANCHES are defined in
   a way that they don't get turned on for allmodconfig, and
   we need to do the samem for GCOV_PROFILE_ALL and
   CC_OPTIMIZE_FOR_SIZE.
3. Fix all known such warnings: The warnings should stick out,
   so we can fix them once they first appear. Today they often
   get ignored because of all the false positives.

For GCOV support, another problem showed up, resulting in an increased
risk for kernel stack overflow, aside from getting a number of
warnings about the stack size. I'm also including a patch to
address that here.

	Arnd

Arnd Bergmann (5):
  Kbuild: change CC_OPTIMIZE_FOR_SIZE definition
  Kbuild: disable 'maybe-uninitialized' warning for
    CONFIG_PROFILE_ALL_BRANCHES
  gcov: disable for COMPILE_TEST
  gcov: disable tree-loop-im to reduce stack usage
  gcov: disable -Wmaybe-uninitialized warning

 Makefile            |  8 ++++++--
 init/Kconfig        | 13 +++++++++++++
 kernel/gcov/Kconfig |  1 +
 3 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.7.0

[PATCH 1/5] Kbuild: change CC_OPTIMIZE_FOR_SIZE definition

[Arnd Bergmann]

CC_OPTIMIZE_FOR_SIZE disables the often useful -Wmaybe-unused warning,
because that causes a ridiculous amount of false positives when combined
with -Os.

This means a lot of warnings don't show up in testing by the developers
that should see them with an 'allmodconfig' kernel that has
CC_OPTIMIZE_FOR_SIZE enabled, but only later in randconfig builds
that don't.

This changes the Kconfig logic around CC_OPTIMIZE_FOR_SIZE to make
it a 'choice' statement defaulting to CC_OPTIMIZE_FOR_PERFORMANCE
that gets added for this purpose. The allmodconfig and allyesconfig
kernels now default to -O2 with the maybe-unused warning enabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 init/Kconfig | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 651ec15ecddb..159a542ef3fc 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1305,6 +1305,17 @@ source "usr/Kconfig"
 
 endif
 
+choice
+	prompt "Compiler optimization level"
+	default CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE
+
+config CC_OPTIMIZE_FOR_PERFORMANCE
+	bool "Optimize for performance"
+	help
+	  This is the default optimization level for the kernel, building
+	  with the "-O2" compiler flag for best performance and most
+	  helpful compile-time warnings.
+
 config CC_OPTIMIZE_FOR_SIZE
 	bool "Optimize for size"
 	help
@@ -1313,6 +1324,8 @@ config CC_OPTIMIZE_FOR_SIZE
 
 	  If unsure, say N.
 
+endchoice
+
 config SYSCTL
 	bool
 
-- 
2.7.0

[PATCH 2/5] Kbuild: disable 'maybe-uninitialized' warning for CONFIG_PROFILE_ALL_BRANCHES

[Arnd Bergmann]

CONFIG_PROFILE_ALL_BRANCHES confuses gcc-5.x to the degree that it prints
incorrect warnings about a lot of variables that it thinks can be used
uninitialized, e.g.:

i2c/busses/i2c-diolan-u2c.c: In function 'diolan_usb_xfer':
i2c/busses/i2c-diolan-u2c.c:391:16: warning: 'byte' may be used uninitialized in this function
iio/gyro/itg3200_core.c: In function 'itg3200_probe':
iio/gyro/itg3200_core.c:213:6: warning: 'val' may be used uninitialized in this function
leds/leds-lp55xx-common.c: In function 'lp55xx_update_bits':
leds/leds-lp55xx-common.c:350:6: warning: 'tmp' may be used uninitialized in this function
misc/bmp085.c: In function 'show_pressure':
misc/bmp085.c:363:10: warning: 'pressure' may be used uninitialized in this function
power/ds2782_battery.c: In function 'ds2786_get_capacity':
power/ds2782_battery.c:214:17: warning: 'raw' may be used uninitialized in this function

These are all false positives that either rob someone's time when trying
to figure out whether they are real, or they get people to send wrong
patches to shut up the warnings.

Nobody normally wants to run a CONFIG_PROFILE_ALL_BRANCHES kernel in
production, so disabling the whole class of warnings for this configuration
has no serious downsides either.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 9092c16fa6cf..352f55ccc54e 100644
--- a/Makefile
+++ b/Makefile
@@ -617,7 +617,11 @@ KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 KBUILD_CFLAGS	+= -Os $(call cc-disable-warning,maybe-uninitialized,)
 else
-KBUILD_CFLAGS	+= -O2
+ifdef CONFIG_PROFILE_ALL_BRANCHES
+KBUILD_CFLAGS	+= -O2 $(call cc-disable-warning,maybe-uninitialized,)
+else
+KBUILD_CFLAGS   += -O2
+endif
 endif
 
 # Tell gcc to never replace conditional load with a non-conditional one
-- 
2.7.0

Re: [PATCH 2/5] Kbuild: disable 'maybe-uninitialized' warning for CONFIG_PROFILE_ALL_BRANCHES

[Steven Rostedt]

On Fri, Feb 12, 2016 at 05:06:19PM +0100, Arnd Bergmann wrote:
> CONFIG_PROFILE_ALL_BRANCHES confuses gcc-5.x to the degree that it prints
> incorrect warnings about a lot of variables that it thinks can be used
> uninitialized, e.g.:
> 
> i2c/busses/i2c-diolan-u2c.c: In function 'diolan_usb_xfer':
> i2c/busses/i2c-diolan-u2c.c:391:16: warning: 'byte' may be used uninitialized in this function
> iio/gyro/itg3200_core.c: In function 'itg3200_probe':
> iio/gyro/itg3200_core.c:213:6: warning: 'val' may be used uninitialized in this function
> leds/leds-lp55xx-common.c: In function 'lp55xx_update_bits':
> leds/leds-lp55xx-common.c:350:6: warning: 'tmp' may be used uninitialized in this function
> misc/bmp085.c: In function 'show_pressure':
> misc/bmp085.c:363:10: warning: 'pressure' may be used uninitialized in this function
> power/ds2782_battery.c: In function 'ds2786_get_capacity':
> power/ds2782_battery.c:214:17: warning: 'raw' may be used uninitialized in this function
> 
> These are all false positives that either rob someone's time when trying
> to figure out whether they are real, or they get people to send wrong
> patches to shut up the warnings.
> 
> Nobody normally wants to run a CONFIG_PROFILE_ALL_BRANCHES kernel in
> production, so disabling the whole class of warnings for this configuration
> has no serious downsides either.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> ---
>  Makefile | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 9092c16fa6cf..352f55ccc54e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -617,7 +617,11 @@ KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
>  ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
>  KBUILD_CFLAGS	+= -Os $(call cc-disable-warning,maybe-uninitialized,)
>  else
> -KBUILD_CFLAGS	+= -O2
> +ifdef CONFIG_PROFILE_ALL_BRANCHES
> +KBUILD_CFLAGS	+= -O2 $(call cc-disable-warning,maybe-uninitialized,)
> +else
> +KBUILD_CFLAGS   += -O2
> +endif
>  endif
>  
>  # Tell gcc to never replace conditional load with a non-conditional one
> -- 
> 2.7.0

[PATCH 3/5] gcov: disable for COMPILE_TEST

[Arnd Bergmann]

Enabling gcov is counterproductive to compile testing: it significantly
increases the kernel image size, compile time, and it produces lots
of false positive "may be used uninitialized" warnings as the result
of missed optimizations.

This is in line with how UBSAN_SANITIZE_ALL and PROFILE_ALL_BRANCHES
work, both of which have similar problems.

With an ARM allmodconfig kernel, I see the build time drop from
283 minutes CPU time to 225 minutes, and the vmlinux size drops
from 43MB to 26MB.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 kernel/gcov/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
index c92e44855ddd..1276aabaab55 100644
--- a/kernel/gcov/Kconfig
+++ b/kernel/gcov/Kconfig
@@ -37,6 +37,7 @@ config ARCH_HAS_GCOV_PROFILE_ALL
 
 config GCOV_PROFILE_ALL
 	bool "Profile entire Kernel"
+	depends on !COMPILE_TEST
 	depends on GCOV_KERNEL
 	depends on ARCH_HAS_GCOV_PROFILE_ALL
 	default n
-- 
2.7.0

Re: [PATCH 3/5] gcov: disable for COMPILE_TEST

[Peter Oberparleiter]

On 12.02.2016 17:06, Arnd Bergmann wrote:
> Enabling gcov is counterproductive to compile testing: it significantly
> increases the kernel image size, compile time, and it produces lots
> of false positive "may be used uninitialized" warnings as the result
> of missed optimizations.
> 
> This is in line with how UBSAN_SANITIZE_ALL and PROFILE_ALL_BRANCHES
> work, both of which have similar problems.
> 
> With an ARM allmodconfig kernel, I see the build time drop from
> 283 minutes CPU time to 225 minutes, and the vmlinux size drops
> from 43MB to 26MB.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Agreed that COMPILE_TEST + GCOV_PROFILE_ALL is a very exotic combination
with little added use.

Acked-by: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>

> ---
>  kernel/gcov/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig
> index c92e44855ddd..1276aabaab55 100644
> --- a/kernel/gcov/Kconfig
> +++ b/kernel/gcov/Kconfig
> @@ -37,6 +37,7 @@ config ARCH_HAS_GCOV_PROFILE_ALL
> 
>  config GCOV_PROFILE_ALL
>  	bool "Profile entire Kernel"
> +	depends on !COMPILE_TEST
>  	depends on GCOV_KERNEL
>  	depends on ARCH_HAS_GCOV_PROFILE_ALL
>  	default n
> 


-- 
Peter Oberparleiter
Linux on z Systems Development - IBM Germany

[PATCH 4/5] gcov: disable tree-loop-im to reduce stack usage

[Arnd Bergmann]

Enabling CONFIG_GCOV_PROFILE_ALL produces us a lot of warnings like

lib/lz4/lz4hc_compress.c: In function 'lz4_compresshcctx':
lib/lz4/lz4hc_compress.c:514:1: warning: the frame size of 1504 bytes is larger than 1024 bytes [-Wframe-larger-than=]

After some investigation, I found that this behavior started with gcc-4.9,
and opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69702.
A suggested workaround for it is to use the -fno-tree-loop-im
flag that turns off one of the optimization stages in gcc, so the
code runs a little slower but does not use excessive amounts
of stack.

We could make this conditional on the gcc version, but I could not
find an easy way to do this in Kbuild and the benefit would be
fairly small, given that most of the gcc version in production are
affected now.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 352f55ccc54e..6bb89728a9d1 100644
--- a/Makefile
+++ b/Makefile
@@ -364,7 +364,7 @@ AFLAGS_MODULE   =
 LDFLAGS_MODULE  =
 CFLAGS_KERNEL	=
 AFLAGS_KERNEL	=
-CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage
+CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im
 CFLAGS_KCOV	= -fsanitize-coverage=trace-pc
 
 
-- 
2.7.0

Re: [PATCH 4/5] gcov: disable tree-loop-im to reduce stack usage

[Peter Oberparleiter]

On 12.02.2016 17:06, Arnd Bergmann wrote:
> Enabling CONFIG_GCOV_PROFILE_ALL produces us a lot of warnings like
> 
> lib/lz4/lz4hc_compress.c: In function 'lz4_compresshcctx':
> lib/lz4/lz4hc_compress.c:514:1: warning: the frame size of 1504 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> After some investigation, I found that this behavior started with gcc-4.9,
> and opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69702.
> A suggested workaround for it is to use the -fno-tree-loop-im
> flag that turns off one of the optimization stages in gcc, so the
> code runs a little slower but does not use excessive amounts
> of stack.
> 
> We could make this conditional on the gcc version, but I could not
> find an easy way to do this in Kbuild and the benefit would be
> fairly small, given that most of the gcc version in production are
> affected now.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

-fno-tree-loop-im seems to have been available for long enough in GCC
(starting with GCC4) to make this part of the default gcov compile flags.

Acked-by: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>

> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 352f55ccc54e..6bb89728a9d1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -364,7 +364,7 @@ AFLAGS_MODULE   =
>  LDFLAGS_MODULE  =
>  CFLAGS_KERNEL	=
>  AFLAGS_KERNEL	=
> -CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage
> +CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im
>  CFLAGS_KCOV	= -fsanitize-coverage=trace-pc
> 
> 


-- 
Peter Oberparleiter
Linux on z Systems Development - IBM Germany

[PATCH 5/5] gcov: disable -Wmaybe-uninitialized warning

[Arnd Bergmann]

When gcov profiling is enabled, we see a lot of spurious warnings about
possibly uninitialized variables being used:

arch/arm/mm/dma-mapping.c: In function 'arm_coherent_iommu_map_page':
arch/arm/mm/dma-mapping.c:1085:16: warning: 'start' may be used uninitialized in this function [-Wmaybe-uninitialized]
drivers/clk/st/clk-flexgen.c: In function 'st_of_flexgen_setup':
drivers/clk/st/clk-flexgen.c:323:9: warning: 'num_parents' may be used uninitialized in this function [-Wmaybe-uninitialized]
kernel/cgroup.c: In function 'cgroup_mount':
kernel/cgroup.c:2119:11: warning: 'root' may be used uninitialized in this function [-Wmaybe-uninitialized]

All of these are false positives, so it seems better to just disable
the warnings whenever GCOV is enabled. Most users don't enable GCOV,
and based on a prior patch, it is now also disabled for 'allmodconfig'
builds, so there should be no downsides of doing this.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 6bb89728a9d1..7b6900931a47 100644
--- a/Makefile
+++ b/Makefile
@@ -364,7 +364,7 @@ AFLAGS_MODULE   =
 LDFLAGS_MODULE  =
 CFLAGS_KERNEL	=
 AFLAGS_KERNEL	=
-CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im
+CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im -Wno-maybe-uninitialized
 CFLAGS_KCOV	= -fsanitize-coverage=trace-pc
 
 
-- 
2.7.0

Re: [PATCH 5/5] gcov: disable -Wmaybe-uninitialized warning

[Peter Oberparleiter]

On 12.02.2016 17:06, Arnd Bergmann wrote:
> When gcov profiling is enabled, we see a lot of spurious warnings about
> possibly uninitialized variables being used:
> 
> arch/arm/mm/dma-mapping.c: In function 'arm_coherent_iommu_map_page':
> arch/arm/mm/dma-mapping.c:1085:16: warning: 'start' may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/clk/st/clk-flexgen.c: In function 'st_of_flexgen_setup':
> drivers/clk/st/clk-flexgen.c:323:9: warning: 'num_parents' may be used uninitialized in this function [-Wmaybe-uninitialized]
> kernel/cgroup.c: In function 'cgroup_mount':
> kernel/cgroup.c:2119:11: warning: 'root' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> All of these are false positives, so it seems better to just disable
> the warnings whenever GCOV is enabled. Most users don't enable GCOV,
> and based on a prior patch, it is now also disabled for 'allmodconfig'
> builds, so there should be no downsides of doing this.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Sounds sane.

Acked-by: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>

> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 6bb89728a9d1..7b6900931a47 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -364,7 +364,7 @@ AFLAGS_MODULE   =
>  LDFLAGS_MODULE  =
>  CFLAGS_KERNEL	=
>  AFLAGS_KERNEL	=
> -CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im
> +CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im -Wno-maybe-uninitialized
>  CFLAGS_KCOV	= -fsanitize-coverage=trace-pc
> 
> 


-- 
Peter Oberparleiter
Linux on z Systems Development - IBM Germany