All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] Turn off -Wmaybe-uninitialized completely and move it to W=1
@ 2014-06-16 13:20 Borislav Petkov
  2014-06-16 21:14 ` Sam Ravnborg
  0 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2014-06-16 13:20 UTC (permalink / raw)
  To: lkml; +Cc: Michael Matz, linux-kbuild, x86-ml

Hi,

so 3.16-rc1 adds this false positive from gcc, see below (triggers on
4.9 and 4.8.2). Now, it is firing wrong and gcc people tell me there's
no way for the compiler to know that the "from" and "to" values will NOT
be used in the error case, i.e. thus the "maybe" aspect.

So, we've disabled it for -Os already:

	e74fc973b6e5 ("Turn off -Wmaybe-uninitialized when building with -Os")

maybe we want to disable it by default on all and move it to W=1. This
way people can still have it fire but not by default. And from what I've
seen so far, it is mostly firing wrong and it is becoming annoying.

So what do people think, any reasons for keeping it enabled by default?

Thanks.

---

fs/direct-io.c: In function ‘__blockdev_direct_IO’:
fs/direct-io.c:920:9: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   while (from < to) {
         ^
fs/direct-io.c:913:16: note: ‘to’ was declared here
   size_t from, to;
                ^
fs/direct-io.c:1034:9: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    from += this_chunk_bytes;
         ^
fs/direct-io.c:913:10: note: ‘from’ was declared here
   size_t from, to;


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RFC] Turn off -Wmaybe-uninitialized completely and move it to W=1
  2014-06-16 13:20 [RFC] Turn off -Wmaybe-uninitialized completely and move it to W=1 Borislav Petkov
@ 2014-06-16 21:14 ` Sam Ravnborg
  2014-06-24 21:38   ` [PATCH] Kbuild: Move -Wmaybe-uninitialized " Borislav Petkov
  0 siblings, 1 reply; 22+ messages in thread
From: Sam Ravnborg @ 2014-06-16 21:14 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: lkml, Michael Matz, linux-kbuild, x86-ml

On Mon, Jun 16, 2014 at 03:20:45PM +0200, Borislav Petkov wrote:
> Hi,
> 
> so 3.16-rc1 adds this false positive from gcc, see below (triggers on
> 4.9 and 4.8.2). Now, it is firing wrong and gcc people tell me there's
> no way for the compiler to know that the "from" and "to" values will NOT
> be used in the error case, i.e. thus the "maybe" aspect.
> 
> So, we've disabled it for -Os already:
> 
> 	e74fc973b6e5 ("Turn off -Wmaybe-uninitialized when building with -Os")
> 
> maybe we want to disable it by default on all and move it to W=1. This
> way people can still have it fire but not by default. And from what I've
> seen so far, it is mostly firing wrong and it is becoming annoying.
> 
> So what do people think, any reasons for keeping it enabled by default?
Agreed.
The noise ratio is too high - so move it to W=1.

	Sam

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-06-16 21:14 ` Sam Ravnborg
@ 2014-06-24 21:38   ` Borislav Petkov
  2014-07-07 10:53     ` Borislav Petkov
  0 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2014-06-24 21:38 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: lkml, Michael Matz, linux-kbuild, x86-ml

On Mon, Jun 16, 2014 at 11:14:05PM +0200, Sam Ravnborg wrote:
> Agreed.
> The noise ratio is too high - so move it to W=1.

Ok, how about the below?

It still needs this hunk to work:

@@ -33,7 +34,7 @@ warning-1 += $(call cc-disable-warning, format)
 warning-1 += $(call cc-disable-warning, unknown-warning-option)
 warning-1 += $(call cc-disable-warning, sign-compare)
 warning-1 += $(call cc-disable-warning, format-zero-length)
-warning-1 += $(call cc-disable-warning, uninitialized)
+#warning-1 += $(call cc-disable-warning, uninitialized)
 warning-1 += $(call cc-option, -fcatch-undefined-behavior)
 
 warning-2 := -Waggregate-return

but that's another story which is still WIP here:
http://lkml.kernel.org/r/20140616130751.GD8170@pd.tnic

After it is sorted out, we can do something like the below. (Note:
I'm not filtering out the previous -Wno-maybe-uninitialized from
KBUILD_CFLAGS because it is not necessary - apparently the following
-Wmaybe-uninitialized reenables it again).

---
From: Borislav Petkov <bp@suse.de>
Subject: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1

This warning is enabled by -Wall or -Wextra, says the gcc manpage. It
also says that gcc cannot always know whether the warning is issued
correctly:

"These warnings are made optional because GCC is not smart enough to see
all the reasons why the code might be correct in spite of appearing to
have an error."

And, as expected, it fires for perfectly valid use cases, thus making it
not really useful. Let's move it to the W=1 bunch in case people want to
enable it with the additional checks.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 Makefile                   | 2 ++
 scripts/Makefile.extrawarn | 1 +
 2 files changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index b11e2d504a00..63033545d7f6 100644
--- a/Makefile
+++ b/Makefile
@@ -402,6 +402,8 @@ KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
 		   -Werror-implicit-function-declaration \
 		   -Wno-format-security
 
+KBUILD_CFLAGS   += $(call cc-disable-warning, maybe-uninitialized)
+
 KBUILD_AFLAGS_KERNEL :=
 KBUILD_CFLAGS_KERNEL :=
 KBUILD_AFLAGS   := -D__ASSEMBLY__
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 65643506c71c..e61678c40d41 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -25,6 +25,7 @@ warning-1 += -Wold-style-definition
 warning-1 += $(call cc-option, -Wmissing-include-dirs)
 warning-1 += $(call cc-option, -Wunused-but-set-variable)
 warning-1 += $(call cc-disable-warning, missing-field-initializers)
+warning-1 += $(call cc-option, -Wmaybe-uninitialized)
 
 # Clang
 warning-1 += $(call cc-disable-warning, initializer-overrides)
-- 
2.0.0

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-06-24 21:38   ` [PATCH] Kbuild: Move -Wmaybe-uninitialized " Borislav Petkov
@ 2014-07-07 10:53     ` Borislav Petkov
  2014-07-08  9:25       ` Paul Bolle
  2016-07-28  4:20       ` Borislav Petkov
  0 siblings, 2 replies; 22+ messages in thread
From: Borislav Petkov @ 2014-07-07 10:53 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: lkml, Michael Matz, linux-kbuild, x86-ml

Ok, here's a v2 which goes ontop of

http://lkml.kernel.org/r/1404175346-12330-1-git-send-email-behanw@converseincode.com

After this, all warnings stuff should be back to normal.

---
From: Borislav Petkov <bp@suse.de>
Subject: [PATCH -v2] Kbuild: Move -Wmaybe-uninitialized to W=1

This warning is enabled by -Wall or -Wextra, says the gcc manpage. It
also says that gcc cannot always know whether the warning is issued
correctly:

"These warnings are made optional because GCC is not smart enough to see
all the reasons why the code might be correct in spite of appearing to
have an error."

And, as expected, it fires for perfectly valid use cases, thus making it
not really useful. Let's move it to the W=1 bunch in case people want to
enable it with the additional checks.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 Makefile                   | 5 ++++-
 scripts/Makefile           | 1 +
 scripts/Makefile.extrawarn | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4d75b4bceedd..b58c94261960 100644
--- a/Makefile
+++ b/Makefile
@@ -613,7 +613,7 @@ include $(srctree)/arch/$(SRCARCH)/Makefile
 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,)
+KBUILD_CFLAGS	+= -Os
 else
 KBUILD_CFLAGS	+= -O2
 endif
@@ -742,6 +742,9 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)
 # use the deterministic mode of AR if available
 KBUILD_ARFLAGS := $(call ar-option,D)
 
+# disable -Wmaybe-uninitialized as too noisy, see Makefile.extrawarn instead
+KBUILD_CFLAGS   += $(call cc-disable-warning, maybe-uninitialized)
+
 # check for 'asm goto'
 ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y)
 	KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
diff --git a/scripts/Makefile b/scripts/Makefile
index 890df5c6adfb..a483d9988a2e 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -19,6 +19,7 @@ hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
 hostprogs-$(CONFIG_ASN1)	 += asn1_compiler
 
 HOSTCFLAGS_sortextable.o = -I$(srctree)/tools/include
+HOSTCFLAGS_sortextable.o += $(call cc-disable-warning, maybe-uninitialized)
 HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
 
 always		:= $(hostprogs-y) $(hostprogs-m)
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index e3501272cd93..9657d5778e06 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -25,6 +25,7 @@ warning-1 += -Wold-style-definition
 warning-1 += $(call cc-option, -Wmissing-include-dirs)
 warning-1 += $(call cc-option, -Wunused-but-set-variable)
 warning-1 += $(call cc-disable-warning, missing-field-initializers)
+warning-1 += $(call cc-option, -Wmaybe-uninitialized)
 
 ifeq ($(COMPILER),clang)
 warning-1 += $(call cc-disable-warning, initializer-overrides)
-- 
2.0.0

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-07-07 10:53     ` Borislav Petkov
@ 2014-07-08  9:25       ` Paul Bolle
  2014-07-08 11:37         ` Borislav Petkov
  2016-07-28  4:20       ` Borislav Petkov
  1 sibling, 1 reply; 22+ messages in thread
From: Paul Bolle @ 2014-07-08  9:25 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Sam Ravnborg, lkml, Michael Matz, linux-kbuild, x86-ml

On Mon, 2014-07-07 at 12:53 +0200, Borislav Petkov wrote:
> This warning is enabled by -Wall or -Wextra, says the gcc manpage. It
> also says that gcc cannot always know whether the warning is issued
> correctly:
> 
> "These warnings are made optional because GCC is not smart enough to see
> all the reasons why the code might be correct in spite of appearing to
> have an error."
> 
> And, as expected, it fires for perfectly valid use cases, thus making it
> not really useful. Let's move it to the W=1 bunch in case people want to
> enable it with the additional checks.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Is the fact that this generates false positives by itself enough to
downgrade this check to W=1?

I do not have any hard numbers to back up my claims, but I'd like to
point out that it is possible that we never see many of the warnings
that GCC correctly issues because they might only occur during local
development. Ie, the developer involved cleans up the patch before
sending out.

My experience with the warnings that actually do make it into mainline
is that quite a few are correct while the false positives tend to be
generated by a pieces of code that are complicated (I think I've seen
the warning labeled as a "code smell").

For example, in my local builds this warning popped up three times in
the current development cycle:
    0) fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    1) drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    2) drivers/usb/misc/usb3503.c:195:11: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Warning 0) occurs in a 150 line function, that grows over 200 lines when
the inline functions it calls are included. And I do think it's not easy
to see hwat that code does. Anyhow, see
https://lkml.org/lkml/2014/7/1/496 for my attempt to silence this
warning by simplifying this function.

See https://lkml.org/lkml/2014/7/1/150 for my patch that silences 1) by,
again, simplifying the code.

And warning 2) is correct. See https://lkml.org/lkml/2014/6/2/511 for a
possible solution.

So, in summary, I believe that the signal/noise ratio actually isn't too
bad. Moreover I think that the noise isn't merely noise, as it points to
possibly problematic sections of code.


Paul Bolle


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-07-08  9:25       ` Paul Bolle
@ 2014-07-08 11:37         ` Borislav Petkov
  2014-07-10 10:42           ` Borislav Petkov
  0 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2014-07-08 11:37 UTC (permalink / raw)
  To: Paul Bolle; +Cc: Sam Ravnborg, lkml, Michael Matz, linux-kbuild, x86-ml

On Tue, Jul 08, 2014 at 11:25:20AM +0200, Paul Bolle wrote:
> Is the fact that this generates false positives by itself enough to
> downgrade this check to W=1?
> 
> I do not have any hard numbers to back up my claims, but I'd like to
> point out that it is possible that we never see many of the warnings
> that GCC correctly issues because they might only occur during local
> development. Ie, the developer involved cleans up the patch before
> sending out.

You're assuming that a developer doesn't use W=<num> to test her/his
patches.

> My experience with the warnings that actually do make it into mainline

Which warnings? All warnings or only the maybe-uninitialized ones?

> is that quite a few are correct while the false positives tend to be
> generated by a pieces of code that are complicated (I think I've seen
> the warning labeled as a "code smell").
> 
> For example, in my local builds this warning popped up three times in
> the current development cycle:
>     0) fs/direct-io.c:1011:12: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        fs/direct-io.c:1011:12: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     1) drivers/platform/x86/eeepc-laptop.c:279:10: warning: ‘value’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     2) drivers/usb/misc/usb3503.c:195:11: warning: ‘err’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> Warning 0) occurs in a 150 line function, that grows over 200 lines when
> the inline functions it calls are included. And I do think it's not easy
> to see hwat that code does.

Have you actually tried to understand what the code does and also to see
that gcc simply cannot see that the to and from will never be used in
the error case? It is not that hard actually.

> Anyhow, see
> https://lkml.org/lkml/2014/7/1/496 for my attempt to silence this
> warning by simplifying this function.

Terrible idea - misdesign the code just because gcc doesn't say that two
variables *might* not be initialized.

> See https://lkml.org/lkml/2014/7/1/150 for my patch that silences 1) by,
> again, simplifying the code.

Again, this is wrong on *every* level! This is perfectly sane code where
value is used *only* in the success case.

> And warning 2) is correct. See https://lkml.org/lkml/2014/6/2/511 for a
> possible solution.

That warning should actually be not "maybe" but really -Wuninitialized.

> So, in summary, I believe that the signal/noise ratio actually isn't
> too bad. Moreover I think that the noise isn't merely noise, as it
> points to possibly problematic sections of code.

It seems you're missing the point so let me explain to you what I mean:

Here's the relevant snippet from the gcc manpage:

       -Wmaybe-uninitialized
           For an automatic variable, if there exists a path from the function entry to
           a use of the variable that is initialized, but there exist some other paths
           for which the variable is not initialized, the compiler emits a warning if
           it cannot prove the uninitialized paths are not executed at run time. These
           warnings are made optional because GCC is not smart enough to see all the
           reasons why the code might be correct in spite of appearing to have an
           error.

Now read it again: "gcc cannot prove the uninitialized paths are not
executed at run time. These warnings are made optional because GCC is
not smart enough ..."

And this is exactly what I'm proposing: keep the warning but downgrade
it so that it doesn't generate false positives noise. I'm not arguing it
is a bad check - I'm just saying it is more of a heuristic and should
belong to the rest of the W= checks.

Oh, moving it to W=1 has another reason - to hide it from overeager
people who really want to *fix* the code and thus obfuscate it for no
sensible reason what*so*f*ckin*ever! Just so that they can please the
compiler which says itself it cannot be that smart!

You have given perfect examples in 0) and 1) above for exactly that.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-07-08 11:37         ` Borislav Petkov
@ 2014-07-10 10:42           ` Borislav Petkov
  2014-07-10 11:03             ` Paul Bolle
  0 siblings, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2014-07-10 10:42 UTC (permalink / raw)
  To: linux-kbuild; +Cc: Paul Bolle, Sam Ravnborg, lkml, Michael Matz, x86-ml

And,

I haven't even wished for it but just to prove everybody my point: just
built rc4+ from Linus' repo with gcc 4.9.0, see below.

Now all of a sudden there's more noise, maybe because this is a
different .config. However, I doubt those are real bugs.

A cursory look through those shows that they're not really bugs -
gcc simply can't know with all the ifdeffery, partial usage based on
conditionals, etc, etc.

---
In file included from scripts/sortextable.c:194:0:
scripts/sortextable.c: In function ‘main’:
scripts/sortextable.h:176:3: warning: ‘relocs_size’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   memset(relocs, 0, relocs_size);
   ^
scripts/sortextable.h:106:6: note: ‘relocs_size’ was declared here
  int relocs_size;
      ^
In file included from scripts/sortextable.c:192:0:
scripts/sortextable.h:176:3: warning: ‘relocs_size’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   memset(relocs, 0, relocs_size);
   ^
scripts/sortextable.h:106:6: note: ‘relocs_size’ was declared here
  int relocs_size;
      ^
fs/namespace.c: In function ‘SyS_mount’:
fs/namespace.c:2647:8: warning: ‘kernel_dev’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  ret = do_mount(kernel_dev, kernel_dir->name, kernel_type, flags,
        ^
fs/namespace.c:2626:8: note: ‘kernel_dev’ was declared here
  char *kernel_dev;
        ^
fs/namespace.c:2647:8: warning: ‘kernel_type’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  ret = do_mount(kernel_dev, kernel_dir->name, kernel_type, flags,
        ^
fs/namespace.c:2624:8: note: ‘kernel_type’ was declared here
  char *kernel_type;
        ^
fs/direct-io.c: In function ‘do_blockdev_direct_IO’:
fs/direct-io.c:920:9: warning: ‘to’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   while (from < to) {
         ^
fs/direct-io.c:913:16: note: ‘to’ was declared here
   size_t from, to;
                ^
fs/direct-io.c:1034:9: warning: ‘from’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    from += this_chunk_bytes;
         ^
fs/direct-io.c:913:10: note: ‘from’ was declared here
   size_t from, to;
          ^
In file included from include/net/inetpeer.h:15:0,
                 from net/ipv4/tcp_metrics.c:16:
net/ipv4/tcp_metrics.c: In function ‘tcp_peer_is_proven’:
include/net/ipv6.h:422:38: warning: ‘*((void *)&daddr+8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  return ((ul1[0] ^ ul2[0]) | (ul1[1] ^ ul2[1])) == 0UL;
                                      ^
net/ipv4/tcp_metrics.c:231:30: note: ‘*((void *)&daddr+8)’ was declared here
  struct inetpeer_addr saddr, daddr;
                              ^
In file included from include/net/inetpeer.h:15:0,
                 from net/ipv4/tcp_metrics.c:16:
include/net/ipv6.h:422:38: warning: ‘*((void *)&saddr+8)’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  return ((ul1[0] ^ ul2[0]) | (ul1[1] ^ ul2[1])) == 0UL;
                                      ^
net/ipv4/tcp_metrics.c:231:23: note: ‘*((void *)&saddr+8)’ was declared here
  struct inetpeer_addr saddr, daddr;
                       ^

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-07-10 10:42           ` Borislav Petkov
@ 2014-07-10 11:03             ` Paul Bolle
  0 siblings, 0 replies; 22+ messages in thread
From: Paul Bolle @ 2014-07-10 11:03 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: linux-kbuild, Sam Ravnborg, lkml, Michael Matz, x86-ml

On Thu, 2014-07-10 at 12:42 +0200, Borislav Petkov wrote:
> I haven't even wished for it but just to prove everybody my point: just
> built rc4+ from Linus' repo with gcc 4.9.0, see below.
> 
> Now all of a sudden there's more noise, maybe because this is a
> different .config. However, I doubt those are real bugs.

Would you mind sharing that .config?

> A cursory look through those shows that they're not really bugs -
> gcc simply can't know with all the ifdeffery, partial usage based on

Nit: I'd expect ifdeffery to make the code only difficult to understand
for the human reader. By the time gcc will be doing flow analysis the
preprocessor has long handled all ifdeffery.

> conditionals, etc, etc.

Thanks,


Paul Bolle


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2014-07-07 10:53     ` Borislav Petkov
  2014-07-08  9:25       ` Paul Bolle
@ 2016-07-28  4:20       ` Borislav Petkov
  2016-07-28  8:29         ` Ingo Molnar
  1 sibling, 1 reply; 22+ messages in thread
From: Borislav Petkov @ 2016-07-28  4:20 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Sam Ravnborg, lkml, Michael Matz, linux-kbuild, x86-ml

Hi Linus,

about 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally"):

Good! Finally!

This thing was really getting on my nerves.

Btw, I tried at the time to move it to W=1 insted of completely
disabling it, see below. That got nowhere though.

On Mon, Jul 07, 2014 at 12:53:39PM +0200, Borislav Petkov wrote:
> Ok, here's a v2 which goes ontop of
> 
> http://lkml.kernel.org/r/1404175346-12330-1-git-send-email-behanw@converseincode.com
> 
> After this, all warnings stuff should be back to normal.
> 
> ---
> From: Borislav Petkov <bp@suse.de>
> Subject: [PATCH -v2] Kbuild: Move -Wmaybe-uninitialized to W=1
> 
> This warning is enabled by -Wall or -Wextra, says the gcc manpage. It
> also says that gcc cannot always know whether the warning is issued
> correctly:
> 
> "These warnings are made optional because GCC is not smart enough to see
> all the reasons why the code might be correct in spite of appearing to
> have an error."
> 
> And, as expected, it fires for perfectly valid use cases, thus making it
> not really useful. Let's move it to the W=1 bunch in case people want to
> enable it with the additional checks.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  Makefile                   | 5 ++++-
>  scripts/Makefile           | 1 +
>  scripts/Makefile.extrawarn | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 4d75b4bceedd..b58c94261960 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -613,7 +613,7 @@ include $(srctree)/arch/$(SRCARCH)/Makefile
>  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,)
> +KBUILD_CFLAGS	+= -Os
>  else
>  KBUILD_CFLAGS	+= -O2
>  endif
> @@ -742,6 +742,9 @@ KBUILD_CFLAGS   += $(call cc-option,-Werror=date-time)
>  # use the deterministic mode of AR if available
>  KBUILD_ARFLAGS := $(call ar-option,D)
>  
> +# disable -Wmaybe-uninitialized as too noisy, see Makefile.extrawarn instead
> +KBUILD_CFLAGS   += $(call cc-disable-warning, maybe-uninitialized)
> +
>  # check for 'asm goto'
>  ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y)
>  	KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 890df5c6adfb..a483d9988a2e 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -19,6 +19,7 @@ hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
>  hostprogs-$(CONFIG_ASN1)	 += asn1_compiler
>  
>  HOSTCFLAGS_sortextable.o = -I$(srctree)/tools/include
> +HOSTCFLAGS_sortextable.o += $(call cc-disable-warning, maybe-uninitialized)
>  HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
>  
>  always		:= $(hostprogs-y) $(hostprogs-m)
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index e3501272cd93..9657d5778e06 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -25,6 +25,7 @@ warning-1 += -Wold-style-definition
>  warning-1 += $(call cc-option, -Wmissing-include-dirs)
>  warning-1 += $(call cc-option, -Wunused-but-set-variable)
>  warning-1 += $(call cc-disable-warning, missing-field-initializers)
> +warning-1 += $(call cc-option, -Wmaybe-uninitialized)
>  
>  ifeq ($(COMPILER),clang)
>  warning-1 += $(call cc-disable-warning, initializer-overrides)
> -- 
> 2.0.0
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28  4:20       ` Borislav Petkov
@ 2016-07-28  8:29         ` Ingo Molnar
  2016-07-28  8:46           ` Borislav Petkov
  2016-07-28 19:03           ` Linus Torvalds
  0 siblings, 2 replies; 22+ messages in thread
From: Ingo Molnar @ 2016-07-28  8:29 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Linus Torvalds, Sam Ravnborg, lkml, Michael Matz, linux-kbuild, x86-ml


* Borislav Petkov <bp@alien8.de> wrote:

> Hi Linus,
> 
> about 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally"):
> 
> Good! Finally!
> 
> This thing was really getting on my nerves.

So I'm worried about this description in the changelog:

| Looking at the warnings produced, every single one I looked at was a false 
| positive, and the warnings are frequent enough (and big enough) that they can 
| easily hide real problems that you don't notice in the noise generated by 
| -Wmaybe-uninitialized.

BUT, isn't this the natural state of things, that the 'final' warnings that don't 
get fixed are the obnoxious, false positive ones - because anyone who looks at 
them will say "oh crap, idiotic compiler!"?

But over the last couple of years I think we probably had hundreds of bugs avoided 
due to the warning (both at the development and at the integration stage) - and 
now we are upset about a dozen residual warnings and declare it's all crap?

I am happy to bash GCC's cavalier attitude to warnings any day of the week, but 
this argumentation for this particular option looks fishy to me.

Why cannot we say something like:

 "a false_positive/false_negative ratio above 10% is considered obnoxious and
  won't be tolerated."

?

BTW., one related GCC property I'm pretty upset about: obvious false negatives for 
clear unused-variable bugs, which caused pain and hours of debugging. An example 
is this recent fix:

  commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
  Author: Peter Zijlstra <peterz@infradead.org>
  Date:   Wed Jan 27 23:24:29 2016 +0100

    perf/x86: Fix uninitialized value usage

  ...

  Only took 6 hours of painful debugging to find this. Neither GCC nor
  Smatch warnings flagged this bug.

... and my worry here is that we are now telling GCC: "don't you dare generate a 
false positive warning!" - at which point GCC folks will add even MORE heuristics 
to avoid false positives that generate even more false negatives which are hurting 
us asymmetrically more than the couple of low intensity minutes spent per false 
positive could ever hurt us ... ??

So what's the logic here? What am I missing?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28  8:29         ` Ingo Molnar
@ 2016-07-28  8:46           ` Borislav Petkov
  2016-07-28 16:49             ` Ingo Molnar
  2016-07-28 17:56             ` Markus Trippelsdorf
  2016-07-28 19:03           ` Linus Torvalds
  1 sibling, 2 replies; 22+ messages in thread
From: Borislav Petkov @ 2016-07-28  8:46 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Sam Ravnborg, lkml, Michael Matz, linux-kbuild, x86-ml

On Thu, Jul 28, 2016 at 10:29:15AM +0200, Ingo Molnar wrote:
> BUT, isn't this the natural state of things, that the 'final' warnings
> that don't get fixed are the obnoxious, false positive ones - because
> anyone who looks at them will say "oh crap, idiotic compiler!"?

Hmm, so my experience is like Linus' - that -Wmaybe thing generates too
much noise and a lot of false positives. The thing is, as Micha (on CC)
explained it to me, that warning simply says that GCC sometimes *cannot*
know whether the variable will be used uninitialized or not and eagerly
issues the warning message, just in case.

> But over the last couple of years I think we probably had hundreds of
> bugs avoided due to the warning (both at the development and at the
> integration stage) - and

Really?

And I've yet to see an example where it actually helped :-\

>   commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
>   Author: Peter Zijlstra <peterz@infradead.org>
>   Date:   Wed Jan 27 23:24:29 2016 +0100
> 
>     perf/x86: Fix uninitialized value usage
> 
>   ...
> 
>   Only took 6 hours of painful debugging to find this. Neither GCC nor
>   Smatch warnings flagged this bug.

So that warning didn't help here either.

> ... and my worry here is that we are now telling GCC: "don't you dare
> generate a false positive warning!" - at which point GCC folks will
> add even MORE heuristics to avoid false positives that generate even
> more false negatives

Why?

I think we should enable only the real warnings and turn off the stuff
which generates a lot of false positives. Or, we could put them behind
the -W= switch, so that people can still build the kernel with it but
not have them enabled by default.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28  8:46           ` Borislav Petkov
@ 2016-07-28 16:49             ` Ingo Molnar
  2016-07-28 17:04               ` Ingo Molnar
  2016-07-28 17:56             ` Markus Trippelsdorf
  1 sibling, 1 reply; 22+ messages in thread
From: Ingo Molnar @ 2016-07-28 16:49 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Linus Torvalds, Sam Ravnborg, lkml, Michael Matz, linux-kbuild,
	x86-ml, Arnaldo Carvalho de Melo, Thomas Gleixner,
	Peter Zijlstra


Firstly, I'd like to stress it that I know that the commit is upstream and there's 
a less than 1% chance for it to be reverted. I'm not arguing with the expectation 
to see it reverted, I'm arguing because I'm not convinced about the underlying 
technical arguments.

Secondly, I'd like to stress it that there are a number of truly crappy GCC 
warnings that not only are obnoxious but also actively create the *wrong* kind of 
'fixes': -Wsign-compare or -fno-strict-aliasing for example.

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Jul 28, 2016 at 10:29:15AM +0200, Ingo Molnar wrote:
> > BUT, isn't this the natural state of things, that the 'final' warnings
> > that don't get fixed are the obnoxious, false positive ones - because
> > anyone who looks at them will say "oh crap, idiotic compiler!"?
> 
> Hmm, so my experience is like Linus' - that -Wmaybe thing generates too
> much noise and a lot of false positives. [...]

But it's only the *residual* warnings we get to see in a huge, 1000+ commits per 
week, 20+ MLOC code base, every 3 months.

We are only seeing a very small minority of warnings, and are seeing them again 
and again in the allmodconfig output because nobody is fixing them because 'GCC is 
obviously wrong'.

So I might be wrong about this, but by its very nature this warning, even if it's 
implemented well, has the natural property that 'crappy corner cases coalesce at 
the bottom'.

> > But over the last couple of years I think we probably had hundreds of
> > bugs avoided due to the warning (both at the development and at the
> > integration stage) - and
> 
> Really?

Yes, really - even many of the false positives were useful to me, because they 
flagged questionable coding practices and overly complex code.

> And I've yet to see an example where it actually helped :-\

I think partly because you are used to working on x86 architecture code where we 
take a look at every warning ASAP.

Check out:

 commit 8e8a6b23f115906678252190c8fcf4168cc60fd5
 Author: Hans Verkuil <hverkuil@xs4all.nl>
 Date:   Tue Jul 28 05:33:55 2015 -0300

    [media] mt9v032: fix uninitialized variable warning

    It can indeed be uninitialized in one corner case. Initialize to NULL.

... we now turn these warnings into false negatives.

This one's a real fix too I think:

  c30400fcffb7 drm/i915: set FDI translations to NULL on SKL

and these were literally the first two random examples I took with a bit of 
grepping.

Plus I'd say that in many cases even if it's a false positive, these warnings 
often flag borderline code complexity bug: code flow should essentially never be 
so complex as to confuse a compiler. If it confuses a compiler it will confuse 9 
coders out of 10.

These two commits:

  3354781a2184 sched/numa: Reflow task_numa_group() to avoid a compiler warning
  719038de98bc x86/intel/cacheinfo: Shut up last long-standing warning

show cases where it flagged actively confusing code.

But these are only those rare warnings that make it upstream - I'd expect more 
than 90% of them to be caught by developers.

> >   commit e01d8718de4170373cd7fbf5cf6f9cb61cebb1e9
> >   Author: Peter Zijlstra <peterz@infradead.org>
> >   Date:   Wed Jan 27 23:24:29 2016 +0100
> > 
> >     perf/x86: Fix uninitialized value usage
> > 
> >   ...
> > 
> >   Only took 6 hours of painful debugging to find this. Neither GCC nor
> >   Smatch warnings flagged this bug.
> 
> So that warning didn't help here either.

Exactly, and now we'll get more such false negatives!

> > ... and my worry here is that we are now telling GCC: "don't you dare
> > generate a false positive warning!" - at which point GCC folks will
> > add even MORE heuristics to avoid false positives that generate even
> > more false negatives
> 
> Why?

Because people react to incentives, and the incentives we are creating here is for 
compiler writers to choose between two options:

1) Solve a really difficult code flow analysis problem and implement the perfect 
   warning detector.

2) Slap even more heuristics on top of existing heuristics to make warnings go
   away.

Guess which one is more likely to be picked?

Now granted, GCC folks will probably not react to 'incentives' created by the 
kernel project (we are one project out of thousands), but still.

> I think we should enable only the real warnings and turn off the stuff which 
> generates a lot of false positives. Or, we could put them behind the -W= switch, 
> so that people can still build the kernel with it but not have them enabled by 
> default.

But that's my point, I believe the false positive rate is pretty low in fact, due 
to three factors:

 - 90% of the warnings get fixed by developers, we never see them upstream

 - I'd say a majority (say 70%) of the remaining warnings are flagging 'complexity 
   bugs'
 
 - only a residual 3% are obnoxious ones.

But these remaining 3% are the ones we are seeing again and again in various 
compiler output, so we tend to get a subjective impression that this warning 
produces countless false positives.

So I *think* the better option would be to do what we are doing in the perf 
tooling: force a build error for these warnings (by default, with an option 
available to make it build). That flushes them out and also makes it sure that 
those questionable sequences of code never get upstream to begin with.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28 16:49             ` Ingo Molnar
@ 2016-07-28 17:04               ` Ingo Molnar
  0 siblings, 0 replies; 22+ messages in thread
From: Ingo Molnar @ 2016-07-28 17:04 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Linus Torvalds, Sam Ravnborg, lkml, Michael Matz, linux-kbuild,
	x86-ml, Arnaldo Carvalho de Melo, Thomas Gleixner,
	Peter Zijlstra


* Ingo Molnar <mingo@kernel.org> wrote:

> But that's my point, I believe the false positive rate is pretty low in fact, due 
> to three factors:
> 
>  - 90% of the warnings get fixed by developers, we never see them upstream
> 
>  - I'd say a majority (say 70%) of the remaining warnings are flagging 'complexity 
>    bugs'
>  
>  - only a residual 3% are obnoxious ones.
> 
> But these remaining 3% are the ones we are seeing again and again in various 
> compiler output, so we tend to get a subjective impression that this warning 
> produces countless false positives.

And note that I am well aware of the real risk this poses: people will ignore real 
warnings if there are so many residual false positives.

I think this approach worked pretty well for perf:

> So I *think* the better option would be to do what we are doing in the perf 
> tooling: force a build error for these warnings (by default, with an option 
> available to make it build). That flushes them out and also makes it sure that 
> those questionable sequences of code never get upstream to begin with.

... but might not be appropriate for the kernel which is a 2 orders of magnitude 
larger code base.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28  8:46           ` Borislav Petkov
  2016-07-28 16:49             ` Ingo Molnar
@ 2016-07-28 17:56             ` Markus Trippelsdorf
  1 sibling, 0 replies; 22+ messages in thread
From: Markus Trippelsdorf @ 2016-07-28 17:56 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Ingo Molnar, Linus Torvalds, Sam Ravnborg, lkml, Michael Matz,
	linux-kbuild, x86-ml

On 2016.07.28 at 10:46 +0200, Borislav Petkov wrote:
> On Thu, Jul 28, 2016 at 10:29:15AM +0200, Ingo Molnar wrote:
> > BUT, isn't this the natural state of things, that the 'final' warnings
> > that don't get fixed are the obnoxious, false positive ones - because
> > anyone who looks at them will say "oh crap, idiotic compiler!"?
> 
> Hmm, so my experience is like Linus' - that -Wmaybe thing generates too
> much noise and a lot of false positives. The thing is, as Micha (on CC)
> explained it to me, that warning simply says that GCC sometimes *cannot*
> know whether the variable will be used uninitialized or not and eagerly
> issues the warning message, just in case.

Another issue is that the number of warnings you get depend on the
optimization level. So -Os may be different from -O2 and once you use
-O3 (I know it is not officially supported) you will drown in false
positives...

-- 
Markus

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28  8:29         ` Ingo Molnar
  2016-07-28  8:46           ` Borislav Petkov
@ 2016-07-28 19:03           ` Linus Torvalds
  2016-07-28 19:08             ` Linus Torvalds
  2016-07-28 21:22             ` Linus Torvalds
  1 sibling, 2 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-07-28 19:03 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Borislav Petkov, Sam Ravnborg, lkml, Michael Matz,
	Linux Kbuild mailing list, x86-ml

On Thu, Jul 28, 2016 at 1:29 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> So I'm worried about this description in the changelog:
>
> | Looking at the warnings produced, every single one I looked at was a false
> | positive, and the warnings are frequent enough (and big enough) that they can
> | easily hide real problems that you don't notice in the noise generated by
> | -Wmaybe-uninitialized.
>
> BUT, isn't this the natural state of things, that the 'final' warnings that don't
> get fixed are the obnoxious, false positive ones - because anyone who looks at
> them will say "oh crap, idiotic compiler!"?

No. The *natural* state of things is very simple:

   Zero warnings.

End of story. No excuses.

We were there at 4.6 for me with an "allmodconfig" build.

In 4.7, we had over a hundred lines of warnings. That is SIMPLY UNACCEPTABLE.

And the new warnings were actually not so much due to new code in 4.7,
as the fact that in between I did a user-space upgrade, and gcc 6.1.1
has regressed to the point of the warnings being an unusable mess.

> But over the last couple of years I think we probably had hundreds of bugs avoided
> due to the warning (both at the development and at the integration stage) - and
> now we are upset about a dozen residual warnings and declare it's all crap?

Nobody fixed the warnings, and looking at them, they were largely
unfixable without making the code worse.

Complain to the gcc people.

Remember: zero warnings.

> I am happy to bash GCC's cavalier attitude to warnings any day of the week, but
> this argumentation for this particular option looks fishy to me.

It has nothing to do with "that particular option".  It has everything
to do with bad warnings in general.

Also, if you actually look at the patch, you'll see that that option
has already been disabled for a _lot_ of configurations because it was
already bad for most cases.

> Why cannot we say something like:
>
>  "a false_positive/false_negative ratio above 10% is considered obnoxious and
>   won't be tolerated."

Sure. I'm happy to do that.

What part of "100% of all the warnings were false positives, and won't
be tolerated" did you not get?

Once we get to the point that the warning is no longer useful, and is
more pain than gain, it gets disabled.

If you open a bugzilla and the gcc people actually react to it, we can
revisit it. But as it is, that warning had largely been disabled for
other configurations anyway, and I just made it go away entirely.

Because false positives that we can't fix without making the code
worse are not acceptable.

                 Linus

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28 19:03           ` Linus Torvalds
@ 2016-07-28 19:08             ` Linus Torvalds
  2016-07-28 20:28               ` Ingo Molnar
  2016-07-28 21:22             ` Linus Torvalds
  1 sibling, 1 reply; 22+ messages in thread
From: Linus Torvalds @ 2016-07-28 19:08 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Borislav Petkov, Sam Ravnborg, lkml, Michael Matz,
	Linux Kbuild mailing list, x86-ml

On Thu, Jul 28, 2016 at 12:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Once we get to the point that the warning is no longer useful, and is
> more pain than gain, it gets disabled.

Btw, I have a suspicion that you didn't realize that
"-Wmaybe-uninitialized" is separate from "-Wuninitialized" (which is
*not* disabled).

The "maybe-uninitialized" warning is literally gcc saying "I haven't
really followed all the logic, but from my broken understanding it
isn't _obvious_ that it is initialized".

And the problem is that a lot of gcc optimization choices basically
move the pointer of "obvious". So the warning is a bit random to begin
with. And when the gcc people screw thigns up, things go to hell in a
handbasket.

             Linus

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28 19:08             ` Linus Torvalds
@ 2016-07-28 20:28               ` Ingo Molnar
  0 siblings, 0 replies; 22+ messages in thread
From: Ingo Molnar @ 2016-07-28 20:28 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Borislav Petkov, Sam Ravnborg, lkml, Michael Matz,
	Linux Kbuild mailing list, x86-ml


* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Jul 28, 2016 at 12:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Once we get to the point that the warning is no longer useful, and is more 
> > pain than gain, it gets disabled.
> 
> Btw, I have a suspicion that you didn't realize that "-Wmaybe-uninitialized" is 
> separate from "-Wuninitialized" (which is *not* disabled).

I very much know the difference, as you can see from the commit IDs I cited.

> The "maybe-uninitialized" warning is literally gcc saying "I haven't really 
> followed all the logic, but from my broken understanding it isn't _obvious_ that 
> it is initialized".
> 
> And the problem is that a lot of gcc optimization choices basically move the 
> pointer of "obvious". So the warning is a bit random to begin with. And when the 
> gcc people screw thigns up, things go to hell in a handbasket.

Fair enough.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28 19:03           ` Linus Torvalds
  2016-07-28 19:08             ` Linus Torvalds
@ 2016-07-28 21:22             ` Linus Torvalds
  2016-07-29 10:08               ` Arnd Bergmann
  1 sibling, 1 reply; 22+ messages in thread
From: Linus Torvalds @ 2016-07-28 21:22 UTC (permalink / raw)
  To: Ingo Molnar, Arnd Bergmann, Michal Marek
  Cc: Borislav Petkov, Sam Ravnborg, lkml, Michael Matz,
	Linux Kbuild mailing list, x86-ml

On Thu, Jul 28, 2016 at 12:03 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And the new warnings were actually not so much due to new code in 4.7,
> as the fact that in between I did a user-space upgrade, and gcc 6.1.1
> has regressed to the point of the warnings being an unusable mess.

Actually, thinking more about this, I'm not convinced it's a gcc
regression, because older gcc's have defainitely had the same problem
with that warning causing tons of spurious issues.

So it might actually mostly be due commit 877417e6ffb9 ("Kbuild:
change CC_OPTIMIZE_FOR_SIZE definition"). As a result of that, we now
end up not using CC_OPTIMIZE_FOR_SIZE for allmodconfig builds.

And since for us, -Os always disabled that warning anyway (because gcc
has always made a bad job of it), the bogus warnings didn't use to be
so annoying and hide the real things.

Of course, a big part of the reasoning for that commit was apparently
because Arnd liked the warning. It back-fired. Now that warning is
gone for everybody, because it's so broken.

                   Linus

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-28 21:22             ` Linus Torvalds
@ 2016-07-29 10:08               ` Arnd Bergmann
  2016-07-29 10:19                 ` Borislav Petkov
  0 siblings, 1 reply; 22+ messages in thread
From: Arnd Bergmann @ 2016-07-29 10:08 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Ingo Molnar, Michal Marek, Borislav Petkov, Sam Ravnborg, lkml,
	Michael Matz, Linux Kbuild mailing list, x86-ml

On Thursday, July 28, 2016 2:22:02 PM CEST Linus Torvalds wrote:
> On Thu, Jul 28, 2016 at 12:03 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > And the new warnings were actually not so much due to new code in 4.7,
> > as the fact that in between I did a user-space upgrade, and gcc 6.1.1
> > has regressed to the point of the warnings being an unusable mess.
> 
> Actually, thinking more about this, I'm not convinced it's a gcc
> regression, because older gcc's have defainitely had the same problem
> with that warning causing tons of spurious issues.

Let me try to get to the bottom of this, maybe we can get the warning
back in the future. It has found a number of actual bugs. The majority
of -Wmaybe-uninitialized warnings that I fixed in linux-next were
false positives (maybe four out of five) but I would think the reason
for this is that most developers that get the warning for an actual
bug fix it right away, but some may have seen it and ignored it
as an obvious false positive.

> So it might actually mostly be due commit 877417e6ffb9 ("Kbuild:
> change CC_OPTIMIZE_FOR_SIZE definition"). As a result of that, we now
> end up not using CC_OPTIMIZE_FOR_SIZE for allmodconfig builds.

Correct, that was the intention. I tried my best to ensure that
all existing 'randconfig' warnings on ARM as well as 'defconfig' and
'allmodconfig' on x86 and powerpc got fixed before that patch
was merged, but evidently something went wrong there.

I'm normally testing with gcc-6.1 on ARM and did not find it to
be any worse than 4.9 or 5.3 for this warning here. I also did
some tests with gcc-6 on x86, but that showed a lot of /other/
warnings so I didn't look too closely at that. I'm installing
the latest gcc-6-branch version now to do some more tests on x86,
to see if gcc itself regressed, or something else caused the warnings
you see.

> And since for us, -Os always disabled that warning anyway (because gcc
> has always made a bad job of it), the bogus warnings didn't use to be
> so annoying and hide the real things.
> 
> Of course, a big part of the reasoning for that commit was apparently
> because Arnd liked the warning. It back-fired. Now that warning is
> gone for everybody, because it's so broken.

Makes sense. Some more background on this, as I've spent some significant
time on figuring out individual false-positive warnings:

The warning has always been enabled traditionally, and it was only
/disabled/ on allmodconfig by accident after my patch to turn it
off for -Os got merged after gcc-4.9 came out.

With gcc-4.8 and earlier, we had a number of false positives. This got
a lot better with "gcc-4.9 -O2" but worse with "gcc-4.9 -Os". On ARM,
I've managed to address all those warnings for randconfig builds,
in most cases by making the code more readable at the same time
(which helps both humans and the compiler to figure out when things
are initialized or not).

I am aware of two issues that made things worse again:

- The new jump-label-for-dynamic-debug patch caused one new warning
  on ARM (in media/dvb)
- x86 uses CONFIG_OPTIMIZE_INLINING on both defconfig and allmodconfig,
  whereas ARM does not. As this changes the inlining decisions, the
  compiler sees different initializations and reports a slightly differnet
  set of -Wmaybe-uninitialized warnings, both correct and incorrect.

Testing with today's linux tree and today's gcc-6.1 snapshot, I see
exactly these gcc warnings on x86 allmodconfig:x

arch/x86/crypto/aesni-intel_glue.c: In function 'helper_rfc4106_decrypt':
include/linux/scatterlist.h:124:36: error: 'dst_sg_walk.sg' may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/media/dvb-frontends/cxd2841er.c: In function 'cxd2841er_tune_tc':
drivers/media/dvb-frontends/cxd2841er.c:3408:40: error: 'carrier_offset' may be used uninitialized in this function [-Werror=maybe-uninitialized]
drivers/sfi/sfi_core.c:175:53: error: self-comparison always evaluates to true [-Werror=tautological-compare]
drivers/sfi/sfi_core.c:195:71: error: self-comparison always evaluates to true [-Werror=tautological-compare]

The cxd2841er.c warning was introduced by the jump-label change
I mentioned, and I submitted a patch for that on the day it appeared.

I can certainly do another patch for the aesni-intel_glue.c warning
(I think avoiding the warning will also make the code more efficient),
but I don't see the pile of other warnings you mentioned, so I wonder
what else differs between our configurations.

	Arnd

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-29 10:08               ` Arnd Bergmann
@ 2016-07-29 10:19                 ` Borislav Petkov
  2016-07-29 10:35                   ` Arnd Bergmann
  2016-07-29 18:26                   ` Linus Torvalds
  0 siblings, 2 replies; 22+ messages in thread
From: Borislav Petkov @ 2016-07-29 10:19 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Linus Torvalds, Ingo Molnar, Michal Marek, Sam Ravnborg, lkml,
	Michael Matz, Linux Kbuild mailing list, x86-ml

On Fri, Jul 29, 2016 at 12:08:51PM +0200, Arnd Bergmann wrote:
> Let me try to get to the bottom of this, maybe we can get the warning
> back in the future. It has found a number of actual bugs. The majority
> of -Wmaybe-uninitialized warnings that I fixed in linux-next were
> false positives (maybe four out of five) but I would think the reason

So this is exactly the problem: we should not fix perfectly fine code
just so that gcc remains quiet. So when you say "fixed false positives"
you actually mean, "changed it so that gcc -Wmaybe-u... doesn't fire"
right?

And we should not do that.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-29 10:19                 ` Borislav Petkov
@ 2016-07-29 10:35                   ` Arnd Bergmann
  2016-07-29 18:26                   ` Linus Torvalds
  1 sibling, 0 replies; 22+ messages in thread
From: Arnd Bergmann @ 2016-07-29 10:35 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Linus Torvalds, Ingo Molnar, Michal Marek, Sam Ravnborg, lkml,
	Michael Matz, Linux Kbuild mailing list, x86-ml

On Friday, July 29, 2016 12:19:32 PM CEST Borislav Petkov wrote:
> On Fri, Jul 29, 2016 at 12:08:51PM +0200, Arnd Bergmann wrote:
> > Let me try to get to the bottom of this, maybe we can get the warning
> > back in the future. It has found a number of actual bugs. The majority
> > of -Wmaybe-uninitialized warnings that I fixed in linux-next were
> > false positives (maybe four out of five) but I would think the reason
> 
> So this is exactly the problem: we should not fix perfectly fine code
> just so that gcc remains quiet. So when you say "fixed false positives"
> you actually mean, "changed it so that gcc -Wmaybe-u... doesn't fire"
> right?
> 
> And we should not do that.

As I said elsewhere in the mail, in general the code becomes more
readable in the process and/or the compiler gets to optimize it better.

What typically happens here is that something prevents the compiler
from seeing that a condition is always true, so it has to evaluate
it at runtime when it should have noticed that it can never hit.

If the code is written in a way that the compiler can actually see
that the condition is known based on what happened earlier, we save
an extra branch, or in some cases duplication of object code.

There have been a small number of cases where this was not possible
and I actually ended up adding a fake initialization because rearranging
the code for the compiler would have made it less readable for humans
(e.g. b268c34e5ee92a [1]), but that has been the rare exception because
of the reasons that Rusty nicely described in [2].

	Arnd

[1] https://patchwork.kernel.org/patch/9212881/
[2] https://rusty.ozlabs.org/?p=232

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] Kbuild: Move -Wmaybe-uninitialized to W=1
  2016-07-29 10:19                 ` Borislav Petkov
  2016-07-29 10:35                   ` Arnd Bergmann
@ 2016-07-29 18:26                   ` Linus Torvalds
  1 sibling, 0 replies; 22+ messages in thread
From: Linus Torvalds @ 2016-07-29 18:26 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Arnd Bergmann, Ingo Molnar, Michal Marek, Sam Ravnborg, lkml,
	Michael Matz, Linux Kbuild mailing list, x86-ml

On Fri, Jul 29, 2016 at 3:19 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> So this is exactly the problem: we should not fix perfectly fine code
> just so that gcc remains quiet. So when you say "fixed false positives"
> you actually mean, "changed it so that gcc -Wmaybe-u... doesn't fire"
> right?
>
> And we should not do that.

It's perfectly fine to do that when it makes sense and doesn't make
the code worse. Adding a few unnecessary initializations to make the
compiler shut up is not a problem.

But in the cases I looked at, that *really* didn't make sense. The
pattern was along the lines of

  struct something var;

  if (initialize_var(&var) < 0)
     return error;

  .. use "var.xyz" .. - gcc complains that "var.xyz" may be uninitialized

and quite frankly,. the code made sense, and adding crazy
initializations for the fact that gcc has a shit-for-brains warning
didn't work well seemed to just make the code worse.

And there was no sane *pattern* to why some cases warned. We have
things like the above in many places. The issue seems to be that
"initialize_var()" needs to be inlined (automatically or explicitly
asked for), and then the error flow in the init function is just
complex enough.

At the point where it doesn't make sense when to initialize things
explicitly, and it changes randomly depending on compiler version and
compiler command line flags, there is *no* sane way to work around it.

We could do whack-a-mole with random code cases, but I really feel
that when the warning is that unreliable and the changes to the source
code to make the broken compiler warning shut up are completely
arbitrary and random, it's worse than useless.

                Linus

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2016-07-29 18:26 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-16 13:20 [RFC] Turn off -Wmaybe-uninitialized completely and move it to W=1 Borislav Petkov
2014-06-16 21:14 ` Sam Ravnborg
2014-06-24 21:38   ` [PATCH] Kbuild: Move -Wmaybe-uninitialized " Borislav Petkov
2014-07-07 10:53     ` Borislav Petkov
2014-07-08  9:25       ` Paul Bolle
2014-07-08 11:37         ` Borislav Petkov
2014-07-10 10:42           ` Borislav Petkov
2014-07-10 11:03             ` Paul Bolle
2016-07-28  4:20       ` Borislav Petkov
2016-07-28  8:29         ` Ingo Molnar
2016-07-28  8:46           ` Borislav Petkov
2016-07-28 16:49             ` Ingo Molnar
2016-07-28 17:04               ` Ingo Molnar
2016-07-28 17:56             ` Markus Trippelsdorf
2016-07-28 19:03           ` Linus Torvalds
2016-07-28 19:08             ` Linus Torvalds
2016-07-28 20:28               ` Ingo Molnar
2016-07-28 21:22             ` Linus Torvalds
2016-07-29 10:08               ` Arnd Bergmann
2016-07-29 10:19                 ` Borislav Petkov
2016-07-29 10:35                   ` Arnd Bergmann
2016-07-29 18:26                   ` Linus Torvalds

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.