| Message ID | 20170411075420.55018-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.04.17 at 09:54, <roger.pau@citrix.com> wrote: > clang gcc-compat warnings can wrongly fire when certain constructions are used, > at least the following flow: > > switch ( ... ) > { > case ...: > while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) > { > ... > > Will cause clang to emit the following warning "'break' is bound to loop, GCC > binds it to switch", which is a false positive, and both gcc and clang bound bind (I think) > the break to the inner switch. In order to workaround this issue, disable the > gcc-compat checks for the usage of the read_atomic macro. Hmm, so far it wasn't clear to me that this is also needing an outer switch() - can you confirm switch inside the while control expression alone does not trigger the warning? > This has been reported upstream as > http://bugs.llvm.org/show_bug.cgi?id=32595. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> The patch itself is fine with me, i.e. Acked-by: Jan Beulich <jbeulich@suse.com> but besides wanting to wait for above confirmation, I'd also like to allow some time for others to voice objections to this new approach. Jan
On Tue, Apr 11, 2017 at 02:35:59AM -0600, Jan Beulich wrote: > >>> On 11.04.17 at 09:54, <roger.pau@citrix.com> wrote: > > clang gcc-compat warnings can wrongly fire when certain constructions are used, > > at least the following flow: > > > > switch ( ... ) > > { > > case ...: > > while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) ^ missing ; > > { > > ... > > > > Will cause clang to emit the following warning "'break' is bound to loop, GCC > > binds it to switch", which is a false positive, and both gcc and clang bound > > bind (I think) Right, I think so. > > > the break to the inner switch. In order to workaround this issue, disable the > > gcc-compat checks for the usage of the read_atomic macro. > > Hmm, so far it wasn't clear to me that this is also needing an outer > switch() - can you confirm switch inside the while control expression > alone does not trigger the warning? Yes, I can confirm that the following: while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x; }) ) { ... Does not trigger the warning. > > This has been reported upstream as > > http://bugs.llvm.org/show_bug.cgi?id=32595. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > The patch itself is fine with me, i.e. > Acked-by: Jan Beulich <jbeulich@suse.com> > > but besides wanting to wait for above confirmation, I'd also like > to allow some time for others to voice objections to this new > approach. Thanks, Roger.
>>> On 11.04.17 at 09:54, <roger.pau@citrix.com> wrote: > clang gcc-compat warnings can wrongly fire when certain constructions are used, > at least the following flow: > > switch ( ... ) > { > case ...: > while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) > { > ... > > Will cause clang to emit the following warning "'break' is bound to loop, GCC > binds it to switch", which is a false positive, and both gcc and clang bound > the break to the inner switch. In order to workaround this issue, disable the > gcc-compat checks for the usage of the read_atomic macro. > > This has been reported upstream as > http://bugs.llvm.org/show_bug.cgi?id=32595. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Julien, Roger forgot to Cc you on this - thoughts wrt 4.9 (without it Xen won't build with clang aiui)? Jan
Hi Jan, On 13/04/17 16:11, Jan Beulich wrote: >>>> On 11.04.17 at 09:54, <roger.pau@citrix.com> wrote: >> clang gcc-compat warnings can wrongly fire when certain constructions are used, >> at least the following flow: >> >> switch ( ... ) >> { >> case ...: >> while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) >> { >> ... >> >> Will cause clang to emit the following warning "'break' is bound to loop, GCC >> binds it to switch", which is a false positive, and both gcc and clang bound >> the break to the inner switch. In order to workaround this issue, disable the >> gcc-compat checks for the usage of the read_atomic macro. >> >> This has been reported upstream as >> http://bugs.llvm.org/show_bug.cgi?id=32595. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Julien, > > Roger forgot to Cc you on this - thoughts wrt 4.9 (without it Xen > won't build with clang aiui)? On the basis that it will break Xen build with clang: Release-acked-by: Julien Grall <julien.grall@arm.com> Cheers,
On Tue, Apr 11, 2017 at 08:54:20AM +0100, Roger Pau Monne wrote: > clang gcc-compat warnings can wrongly fire when certain constructions are used, > at least the following flow: > > switch ( ... ) > { > case ...: > while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) > { > ... > > Will cause clang to emit the following warning "'break' is bound to loop, GCC > binds it to switch", which is a false positive, and both gcc and clang bound > the break to the inner switch. In order to workaround this issue, disable the > gcc-compat checks for the usage of the read_atomic macro. > > This has been reported upstream as http://bugs.llvm.org/show_bug.cgi?id=32595. FWIW, this has now been fixed upstream: https://reviews.llvm.org/rL307051 Roger.
diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h index 2fbe705518..b997a1726b 100644 --- a/xen/include/asm-x86/atomic.h +++ b/xen/include/asm-x86/atomic.h @@ -45,6 +45,7 @@ void __bad_atomic_size(void); #define read_atomic(p) ({ \ unsigned long x_; \ + CLANG_DISABLE_WARN_GCC_COMPAT_START \ switch ( sizeof(*(p)) ) { \ case 1: x_ = read_u8_atomic((uint8_t *)(p)); break; \ case 2: x_ = read_u16_atomic((uint16_t *)(p)); break; \ @@ -52,6 +53,7 @@ void __bad_atomic_size(void); case 8: x_ = read_u64_atomic((uint64_t *)(p)); break; \ default: x_ = 0; __bad_atomic_size(); break; \ } \ + CLANG_DISABLE_WARN_GCC_COMPAT_END \ (typeof(*(p)))x_; \ }) diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 16aeeea7f1..17d1f33a2d 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -100,4 +100,31 @@ # define ASM_FLAG_OUT(yes, no) no #endif +/* + * NB: we need to disable the gcc-compat warnings for clang in some places or + * else it will complain with: "'break' is bound to loop, GCC binds it to + * switch" when a switch is used inside of a while expression inside of a + * switch statement, ie: + * + * switch ( ... ) + * { + * case ...: + * while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) + * { + * ... + * + * This has already been reported upstream: + * http://bugs.llvm.org/show_bug.cgi?id=32595 + */ +#ifdef __clang__ +# define CLANG_DISABLE_WARN_GCC_COMPAT_START \ + _Pragma("clang diagnostic push") \ + _Pragma("clang diagnostic ignored \"-Wgcc-compat\"") +# define CLANG_DISABLE_WARN_GCC_COMPAT_END \ + _Pragma("clang diagnostic pop") +#else +# define CLANG_DISABLE_WARN_GCC_COMPAT_START +# define CLANG_DISABLE_WARN_GCC_COMPAT_END +#endif + #endif /* __LINUX_COMPILER_H */
clang gcc-compat warnings can wrongly fire when certain constructions are used, at least the following flow: switch ( ... ) { case ...: while ( ({ int x; switch ( foo ) { case 1: x = 1; break; } x }) ) { ... Will cause clang to emit the following warning "'break' is bound to loop, GCC binds it to switch", which is a false positive, and both gcc and clang bound the break to the inner switch. In order to workaround this issue, disable the gcc-compat checks for the usage of the read_atomic macro. This has been reported upstream as http://bugs.llvm.org/show_bug.cgi?id=32595. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/include/asm-x86/atomic.h | 2 ++ xen/include/xen/compiler.h | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)