From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966462AbcCPIPG (ORCPT ); Wed, 16 Mar 2016 04:15:06 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:35880 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966107AbcCPIOs (ORCPT ); Wed, 16 Mar 2016 04:14:48 -0400 Date: Wed, 16 Mar 2016 09:14:44 +0100 From: Ingo Molnar To: Frederic Weisbecker Cc: Linus Torvalds , Linux Kernel Mailing List , Thomas Gleixner , Peter Zijlstra , Andrew Morton Subject: Re: [PATCH v2] atomic: Fix bugs in 'fetch_or()' and rename it to 'xchg_or()' Message-ID: <20160316081444.GB31133@gmail.com> References: <20160314123200.GA15971@gmail.com> <20160315093245.GA7943@gmail.com> <20160315122145.GA7225@gmail.com> <20160315170835.GA5058@lerouge> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160315170835.GA5058@lerouge> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Frederic Weisbecker wrote: > > diff --git a/include/linux/atomic.h b/include/linux/atomic.h > > index 6c502cb13c95..7bc5297bcca8 100644 > > --- a/include/linux/atomic.h > > +++ b/include/linux/atomic.h > > @@ -549,22 +549,28 @@ static inline int atomic_dec_if_positive(atomic_t *v) > > #endif > > > > /** > > - * fetch_or - perform *ptr |= mask and return old value of *ptr > > - * @ptr: pointer to value > > + * xchg_or - perform *ptr |= mask atomically and return old value of *ptr > > + * @ptr: pointer to value (cmpxchg() compatible integer pointer type) > > * @mask: mask to OR on the value > > * > > - * cmpxchg based fetch_or, macro so it works for different integer types > > + * cmpxchg() based, it's a macro so it works for different integer types. > > */ > > -#ifndef fetch_or > > -#define fetch_or(ptr, mask) \ > > -({ typeof(*(ptr)) __old, __val = *(ptr); \ > > +#ifndef xchg_or > > +# define xchg_or(ptr, mask) \ > > +({ \ > > + typeof(ptr) _ptr = (ptr); \ > > Can we add a comment above to prevent from future mistakes with cmpxchg > variables aliasing? > > I'm suprised that GCC doesn't warn about that actually. Yeah, so in the perf tooling build we do have -Wshadow to catch such mishaps, but not in the main kernel build. ... and yes, if I add it via the patch below the bug gets warned about: include/linux/atomic.h:561:15: note: shadowed declaration is here typeof(ptr) __ptr = (ptr); \ ^ kernel/sched/core.c:332:11: note: in expansion of macro ‘xchg_or’ return !(xchg_or(&ti->flags, _TIF_NEED_RESCHED) & _TIF_POLLING_NRFLA ... but I also get a ton of other warnings, just when building a single kernel/sched/core.o file: ./arch/x86/include/asm/bitops.h:396:28: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow] ./arch/x86/include/asm/bitops.h:396:28: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow] include/linux/jiffies.h:422:60: warning: declaration of ‘jiffies’ shadows a global declaration [-Wshadow] ./arch/x86/include/asm/io_apic.h:187:54: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow] ./arch/x86/include/asm/bitops.h:396:28: warning: declaration of ‘ffs’ shadows a built-in function [-Wshadow] include/linux/jiffies.h:422:60: warning: declaration of ‘jiffies’ shadows a global declaration [-Wshadow] ./arch/x86/include/asm/io_apic.h:187:54: warning: declaration of ‘apic’ shadows a global declaration [-Wshadow] include/linux/kernel.h:750:12: warning: declaration of ‘_min1’ shadows a previous local [-Wshadow] include/linux/kernel.h:750:12: warning: declaration of ‘_min1’ shadows a previous local [-Wshadow] include/linux/kernel.h:751:12: warning: declaration of ‘_min2’ shadows a previous local [-Wshadow] kernel/sched/sched.h:308:43: warning: declaration of ‘down’ shadows a global declaration [-Wshadow] kernel/sched/sched.h:308:60: warning: declaration of ‘up’ shadows a global declaration [-Wshadow] kernel/sched/auto_group.h:44:55: warning: declaration of ‘init_task’ shadows a global declaration [-Wshadow] kernel/sched/core.c:635:20: warning: declaration of ‘down’ shadows a global declaration [-Wshadow] kernel/sched/core.c:635:37: warning: declaration of ‘up’ shadows a global declaration [-Wshadow] and yes, I'd say most of these are signatures of sloppy macros and sloppy variable names - but it would be a ton of work to eliminate these warnings. Thanks, Ingo ==============> Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 7b3ecdcdc6c1..14b0ce82f2b0 100644 --- a/Makefile +++ b/Makefile @@ -776,6 +776,9 @@ KBUILD_CFLAGS += $(call cc-option,-Werror=date-time) # use the deterministic mode of AR if available KBUILD_ARFLAGS := $(call ar-option,D) +# enforce correct pointer usage +KBUILD_CFLAGS += $(call cc-option,-Wshadow) + # check for 'asm goto' ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC)), y) KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO