Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422830 > unrolled thread

Re: linux-next: build failure after merge of the akpm-current tree

Started byPaul Bolle <pebolle@tiscali.nl>
First post2016-06-15 11:40 +0200
Last post2016-06-15 23:20 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: linux-next: build failure after merge of the akpm-current tree Paul Bolle <pebolle@tiscali.nl> - 2016-06-15 11:40 +0200
    Re: linux-next: build failure after merge of the akpm-current tree Josh Poimboeuf <jpoimboe@redhat.com> - 2016-06-15 16:10 +0200
      Re: linux-next: build failure after merge of the akpm-current tree Paul Bolle <pebolle@tiscali.nl> - 2016-06-15 23:20 +0200

#1422830 — Re: linux-next: build failure after merge of the akpm-current tree

FromPaul Bolle <pebolle@tiscali.nl>
Date2016-06-15 11:40 +0200
SubjectRe: linux-next: build failure after merge of the akpm-current tree
Message-ID<rKfsK-Lj-19@gated-at.bofh.it>
On do, 2016-05-05 at 22:44 -0700, Andrew Morton wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Subject: byteswap: try to avoid __builtin_constant_p gcc bug
> 
> This is another attempt to avoid a regression in wwn_to_u64() after that
> started using get_unaligned_be64(), which in turn ran into a bug on
> gcc-4.9 through 6.1.
> 
> The regression got introduced due to the combination of two separate
> workarounds (e3bde9568d99 ("include/linux/unaligned: force inlining of
> byteswap operations") and ef3fb2422ffe ("scsi: fc: use get/put_unaligned64
> for wwn access")) that each try to sidestep distinct problems with gcc
> behavior (code growth and increased stack usage).  Unfortunately after
> both have been applied, a more serious gcc bug has been uncovered, leading
> to incorrect object code that discards part of a function and causes
> undefined behavior.
> 
> As part of this problem is how __builtin_constant_p gets evaluated on an
> argument passed by reference into an inline function, this avoids the use
> of __builtin_constant_p() for all architectures that set
> CONFIG_ARCH_USE_BUILTIN_BSWAP.  Most architectures do not set
> ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably do not suffer
> from the problem in the qla2xxx driver, but they might still run into it
> elsewhere.
> 
> Both of the original workarounds were only merged in the 4.6 kernel, and
> the bug that is fixed by this patch should only appear if both are there,
> so we probably don't need to backport the fix.  On the other hand, it
> works by simplifying the code path and should not have any negative
> effects.
> 
> [arnd@arndb.de: fix older gcc warnings]
>   (http://lkml.kernel.org/r/12243652.bxSxEgjgfk@wuerfel)
> Link: https://lkml.org/lkml/headers/2016/4/12/1103
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70232
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> Fixes: e3bde9568d99 ("include/linux/unaligned: force inlining of byteswap operations")
> Fixes: ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> Link: http://lkml.kernel.org/r/1780465.XdtPJpi8Tt@wuerfel
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Tested-by: Josh Poimboeuf <jpoimboe@redhat.com> # on gcc-5.3
> Tested-by: Quinn Tran <quinn.tran@qlogic.com>
> Cc: Martin Jambor <mjambor@suse.cz>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
> Cc: Jan Hubicka <hubicka@ucw.cz>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

This became commit 7322dd755e7d ("byteswap: try to avoid
__builtin_constant_p gcc bug"). That commit was included in v4.6-rc7.
Ever since that rc I see this warning when building for x86_64:
    net/netfilter/ipvs/ip_vs_sync.c: In function ‘ip_vs_proc_sync_conn’:
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.init_seq’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      struct ip_vs_sync_conn_options opt;
                                     ^
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.previous_delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).init_seq’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).previous_delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]

(It doesn't show up when building for 32 bits x86. Perhaps there's an
int/long mismatch somewhere in the related code.)

Anyone else seeing this? 

It looks like a false positive. I can make it disappear by making sure
ip_vs_proc_seqopt() isn't inlined. But that's not something I dare to
put into a patch for a false positive. Anyone sitting on a better fix?


Paul Bolle

[toc] | [next] | [standalone]

#1423051

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-06-15 16:10 +0200
Message-ID<rKjG1-3xJ-3@gated-at.bofh.it>
In reply to#1422830
On Wed, Jun 15, 2016 at 11:33:32AM +0200, Paul Bolle wrote:
> On do, 2016-05-05 at 22:44 -0700, Andrew Morton wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > Subject: byteswap: try to avoid __builtin_constant_p gcc bug
> > 
> > This is another attempt to avoid a regression in wwn_to_u64() after that
> > started using get_unaligned_be64(), which in turn ran into a bug on
> > gcc-4.9 through 6.1.
> > 
> > The regression got introduced due to the combination of two separate
> > workarounds (e3bde9568d99 ("include/linux/unaligned: force inlining of
> > byteswap operations") and ef3fb2422ffe ("scsi: fc: use get/put_unaligned64
> > for wwn access")) that each try to sidestep distinct problems with gcc
> > behavior (code growth and increased stack usage).  Unfortunately after
> > both have been applied, a more serious gcc bug has been uncovered, leading
> > to incorrect object code that discards part of a function and causes
> > undefined behavior.
> > 
> > As part of this problem is how __builtin_constant_p gets evaluated on an
> > argument passed by reference into an inline function, this avoids the use
> > of __builtin_constant_p() for all architectures that set
> > CONFIG_ARCH_USE_BUILTIN_BSWAP.  Most architectures do not set
> > ARCH_SUPPORTS_OPTIMIZED_INLINING, which means they probably do not suffer
> > from the problem in the qla2xxx driver, but they might still run into it
> > elsewhere.
> > 
> > Both of the original workarounds were only merged in the 4.6 kernel, and
> > the bug that is fixed by this patch should only appear if both are there,
> > so we probably don't need to backport the fix.  On the other hand, it
> > works by simplifying the code path and should not have any negative
> > effects.
> > 
> > [arnd@arndb.de: fix older gcc warnings]
> >   (http://lkml.kernel.org/r/12243652.bxSxEgjgfk@wuerfel)
> > Link: https://lkml.org/lkml/headers/2016/4/12/1103
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66122
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70232
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70646
> > Fixes: e3bde9568d99 ("include/linux/unaligned: force inlining of byteswap operations")
> > Fixes: ef3fb2422ffe ("scsi: fc: use get/put_unaligned64 for wwn access")
> > Link: http://lkml.kernel.org/r/1780465.XdtPJpi8Tt@wuerfel
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Reviewed-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Tested-by: Josh Poimboeuf <jpoimboe@redhat.com> # on gcc-5.3
> > Tested-by: Quinn Tran <quinn.tran@qlogic.com>
> > Cc: Martin Jambor <mjambor@suse.cz>
> > Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> > Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
> > Cc: Denys Vlasenko <dvlasenk@redhat.com>
> > Cc: Thomas Graf <tgraf@suug.ch>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>
> > Cc: Jan Hubicka <hubicka@ucw.cz>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> This became commit 7322dd755e7d ("byteswap: try to avoid
> __builtin_constant_p gcc bug"). That commit was included in v4.6-rc7.
> Ever since that rc I see this warning when building for x86_64:
>     net/netfilter/ipvs/ip_vs_sync.c: In function ‘ip_vs_proc_sync_conn’:
>     net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.init_seq’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>       struct ip_vs_sync_conn_options opt;
>                                      ^
>     net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘opt.previous_delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).init_seq’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     net/netfilter/ipvs/ip_vs_sync.c:1069:33: warning: ‘*((void *)&opt+12).previous_delta’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> (It doesn't show up when building for 32 bits x86. Perhaps there's an
> int/long mismatch somewhere in the related code.)
> 
> Anyone else seeing this? 
> 
> It looks like a false positive. I can make it disappear by making sure
> ip_vs_proc_seqopt() isn't inlined. But that's not something I dare to
> put into a patch for a false positive. Anyone sitting on a better fix?

Hi Paul,

I agree it looks like a false positive, though the code is a bit
convoluted, so I'm not surprised that gcc might get confused.  How about
initializing opt to 0?

diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 803001a..f45496c 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1066,7 +1066,7 @@ static int ip_vs_proc_str(__u8 *p, unsigned int plen, unsigned int *data_len,
  */
 static inline int ip_vs_proc_sync_conn(struct netns_ipvs *ipvs, __u8 *p, __u8 *msg_end)
 {
-	struct ip_vs_sync_conn_options opt;
+	struct ip_vs_sync_conn_options opt = {0};
 	union  ip_vs_sync_conn *s;
 	struct ip_vs_protocol *pp;
 	struct ip_vs_conn_param param;

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1423456

FromPaul Bolle <pebolle@tiscali.nl>
Date2016-06-15 23:20 +0200
Message-ID<rKqoa-7Kt-23@gated-at.bofh.it>
In reply to#1423051
Hi Josh,

On wo, 2016-06-15 at 09:03 -0500, Josh Poimboeuf wrote:
> I agree it looks like a false positive, though the code is a bit
> convoluted, so I'm not surprised that gcc might get confused.

Agree.

> How about initializing opt to 0?

I'm unconvinced. Especially since this only shows up in 64 bits builds.

> --- a/net/netfilter/ipvs/ip_vs_sync.c
> +++ b/net/netfilter/ipvs/ip_vs_sync.c

>  static inline int ip_vs_proc_sync_conn(struct netns_ipvs *ipvs, __u8

> -	struct ip_vs_sync_conn_options opt;
> +	struct ip_vs_sync_conn_options opt = {0};

You forgot to append
                                                  /* I give up */

here. More common would be to use
                                                  /* silence GCC */
Thanks,


Paul Bolle

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web