linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast
@ 2020-02-20 17:37 Jesse Brandeburg
  2020-02-20 17:37 ` [PATCH v2 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
  2020-02-20 18:12 ` [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Peter Zijlstra
  0 siblings, 2 replies; 6+ messages in thread
From: Jesse Brandeburg @ 2020-02-20 17:37 UTC (permalink / raw)
  To: tglx, mingo, bp
  Cc: Jesse Brandeburg, x86, linux-kernel, linux, andriy.shevchenko,
	dan.j.williams

Fix many sparse warnings when building with C=1.

When the kernel is compiled with C=1, there are lots of messages like:
  arch/x86/include/asm/bitops.h:77:37: warning: cast truncates bits from constant value (ffffff7f becomes 7f)

CONST_MASK() is using a signed integer "1" to create the mask which
is later cast to (u8) when used. Move the cast to the definition so
and use local variables so sparse and the compiler can see the
correct type.

Verified with a test module (see next patch) and assembly inspection
that the patch doesn't introduce any change in generated code.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
v2: use correct CC: list
---
 arch/x86/include/asm/bitops.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 062cdecb2f24..2922b352b6ed 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -46,15 +46,17 @@
  * a mask operation on a byte.
  */
 #define CONST_MASK_ADDR(nr, addr)	WBYTE_ADDR((void *)(addr) + ((nr)>>3))
-#define CONST_MASK(nr)			(1 << ((nr) & 7))
+#define CONST_MASK(nr)			((u8)1 << ((nr) & 7))
 
 static __always_inline void
 arch_set_bit(long nr, volatile unsigned long *addr)
 {
 	if (__builtin_constant_p(nr)) {
+		u8 cmask = CONST_MASK(nr);
+
 		asm volatile(LOCK_PREFIX "orb %1,%0"
 			: CONST_MASK_ADDR(nr, addr)
-			: "iq" ((u8)CONST_MASK(nr))
+			: "iq" (cmask)
 			: "memory");
 	} else {
 		asm volatile(LOCK_PREFIX __ASM_SIZE(bts) " %1,%0"
@@ -72,9 +74,11 @@ static __always_inline void
 arch_clear_bit(long nr, volatile unsigned long *addr)
 {
 	if (__builtin_constant_p(nr)) {
+		u8 cmaski = ~CONST_MASK(nr);
+
 		asm volatile(LOCK_PREFIX "andb %1,%0"
 			: CONST_MASK_ADDR(nr, addr)
-			: "iq" ((u8)~CONST_MASK(nr)));
+			: "iq" (cmaski));
 	} else {
 		asm volatile(LOCK_PREFIX __ASM_SIZE(btr) " %1,%0"
 			: : RLONG_ADDR(addr), "Ir" (nr) : "memory");
-- 
2.24.1


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

* [PATCH v2 2/2] lib: make a test module with set/clear bit
  2020-02-20 17:37 [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg
@ 2020-02-20 17:37 ` Jesse Brandeburg
  2020-02-20 18:10   ` Peter Zijlstra
  2020-02-20 18:12 ` [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Peter Zijlstra
  1 sibling, 1 reply; 6+ messages in thread
From: Jesse Brandeburg @ 2020-02-20 17:37 UTC (permalink / raw)
  To: tglx, mingo, bp
  Cc: Jesse Brandeburg, x86, linux-kernel, linux, andriy.shevchenko,
	dan.j.williams

Test some bit clears/sets to make sure assembly doesn't change.
Instruct Kbuild to build this file with extra warning level -Wextra,
to catch new issues, and also doesn't hurt to build with C=1.

This was used to test changes to arch/x86/include/asm/bitops.h.

Recommended usage:
make defconfig
scripts/config -m CONFIG_TEST_BITOPS
make modules_prepare
make C=1 W=1 lib/test_bitops.ko
objdump -S -d lib/test_bitops.ko

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
v2: use correct CC: list
---
 lib/Kconfig.debug                  | 13 ++++++++++++
 lib/Makefile                       |  2 ++
 lib/test_bitops.c                  | 34 ++++++++++++++++++++++++++++++
 tools/testing/selftests/lib/config |  1 +
 4 files changed, 50 insertions(+)
 create mode 100644 lib/test_bitops.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 69def4a9df00..61a5d00ea064 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1947,6 +1947,19 @@ config TEST_LKM
 
 	  If unsure, say N.
 
+config TEST_BITOPS
+	tristate "Test module for compilation of clear_bit/set_bit operations"
+	depends on m
+	help
+	  This builds the "test_bitops" module that is much like the
+	  TEST_LKM module except that it does a basic exercise of the
+	  clear_bit and set_bit macros to make sure there are no compiler
+	  warnings from C=1 sparse checker or -Wextra compilations. It has
+	  no dependencies and doesn't run or load unless explicitly requested
+	  by name.  for example: modprobe test_bitops.
+
+	  If unsure, say N.
+
 config TEST_VMALLOC
 	tristate "Test module for stress/performance analysis of vmalloc allocator"
 	default n
diff --git a/lib/Makefile b/lib/Makefile
index 611872c06926..b18db565b355 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -89,6 +89,8 @@ obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
 obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
 obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
 obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
+obj-$(CONFIG_TEST_BITOPS) += test_bitops.o
+CFLAGS_test_bitops.o += -Werror
 
 obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
 
diff --git a/lib/test_bitops.c b/lib/test_bitops.c
new file mode 100644
index 000000000000..077af86cf616
--- /dev/null
+++ b/lib/test_bitops.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+
+/* a tiny module only meant to compile-test set/clear_bit */
+
+static DECLARE_BITMAP(g_bitmap, 80);
+
+static int __init test_bitops_startup(void)
+{
+	pr_warn("Loaded test module\n");
+	set_bit(4, g_bitmap);
+	set_bit(11, g_bitmap);
+	set_bit(40, g_bitmap);
+	return 0;
+}
+
+static void __exit test_bitops_unstartup(void)
+{
+	clear_bit(4, g_bitmap);
+	clear_bit(11, g_bitmap);
+	clear_bit(40, g_bitmap);
+	pr_warn("Unloaded test module\n");
+}
+
+module_init(test_bitops_startup);
+module_exit(test_bitops_unstartup);
+
+MODULE_AUTHOR("Jesse Brandeburg <jesse.brandeburg@intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Bit testing module");
diff --git a/tools/testing/selftests/lib/config b/tools/testing/selftests/lib/config
index 14a77ea4a8da..b80ee3f6e265 100644
--- a/tools/testing/selftests/lib/config
+++ b/tools/testing/selftests/lib/config
@@ -2,3 +2,4 @@ CONFIG_TEST_PRINTF=m
 CONFIG_TEST_BITMAP=m
 CONFIG_PRIME_NUMBERS=m
 CONFIG_TEST_STRSCPY=m
+CONFIG_TEST_BITOPS=m
-- 
2.24.1


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

* Re: [PATCH v2 2/2] lib: make a test module with set/clear bit
  2020-02-20 17:37 ` [PATCH v2 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
@ 2020-02-20 18:10   ` Peter Zijlstra
  2020-02-20 19:03     ` Jesse Brandeburg
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2020-02-20 18:10 UTC (permalink / raw)
  To: Jesse Brandeburg
  Cc: tglx, mingo, bp, x86, linux-kernel, linux, andriy.shevchenko,
	dan.j.williams

On Thu, Feb 20, 2020 at 09:37:22AM -0800, Jesse Brandeburg wrote:
> Test some bit clears/sets to make sure assembly doesn't change.
> Instruct Kbuild to build this file with extra warning level -Wextra,
> to catch new issues, and also doesn't hurt to build with C=1.
> 
> This was used to test changes to arch/x86/include/asm/bitops.h.
> 
> Recommended usage:
> make defconfig
> scripts/config -m CONFIG_TEST_BITOPS
> make modules_prepare
> make C=1 W=1 lib/test_bitops.ko
> objdump -S -d lib/test_bitops.ko

I only applied this second patch:

# sparse --version
0.6.0 (Debian: 0.6.0-3+b1)

# grep TEST_BITOPS defconfig-build/.config
CONFIG_TEST_BITOPS=m

# make C=1 W=1 O=defconfig-build/ lib/test_bitops.ko
make[1]: Entering directory '/usr/src/linux-2.6/defconfig-build'
GEN     Makefile
HOSTCC  scripts/basic/fixdep
HOSTCC  arch/x86/tools/relocs_32.o
HOSTCC  arch/x86/tools/relocs_64.o
HOSTCC  arch/x86/tools/relocs_common.o
HOSTLD  arch/x86/tools/relocs
HOSTCC  scripts/selinux/genheaders/genheaders
HOSTCC  scripts/selinux/mdp/mdp
HOSTCC  scripts/kallsyms
HOSTCC  scripts/sorttable
HOSTCC  scripts/asn1_compiler
HOSTCC  scripts/extract-cert
HOSTCC  scripts/mod/mk_elfconfig
CHECK   ../scripts/mod/empty.c
CC      scripts/mod/empty.o
MKELF   scripts/mod/elfconfig.h
HOSTCC  scripts/mod/modpost.o
CC      scripts/mod/devicetable-offsets.s
HOSTCC  scripts/mod/file2alias.o
HOSTCC  scripts/mod/sumversion.o
HOSTLD  scripts/mod/modpost
CC      kernel/bounds.s
CC      arch/x86/kernel/asm-offsets.s
CALL    ../scripts/checksyscalls.sh
CALL    ../scripts/atomic/check-atomics.sh
DESCEND  objtool
HOSTCC   /usr/src/linux-2.6/defconfig-build/tools/objtool/fixdep.o
HOSTLD   /usr/src/linux-2.6/defconfig-build/tools/objtool/fixdep-in.o
LINK     /usr/src/linux-2.6/defconfig-build/tools/objtool/fixdep
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/exec-cmd.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/help.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/pager.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/parse-options.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/run-command.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/sigchain.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/subcmd-config.o
LD       /usr/src/linux-2.6/defconfig-build/tools/objtool/libsubcmd-in.o
AR       /usr/src/linux-2.6/defconfig-build/tools/objtool/libsubcmd.a
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/arch/x86/decode.o
LD       /usr/src/linux-2.6/defconfig-build/tools/objtool/arch/x86/objtool-in.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/builtin-check.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/builtin-orc.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/check.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/orc_gen.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/orc_dump.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/elf.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/special.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/objtool.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/libstring.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/libctype.o
CC       /usr/src/linux-2.6/defconfig-build/tools/objtool/str_error_r.o
LD       /usr/src/linux-2.6/defconfig-build/tools/objtool/objtool-in.o
LINK     /usr/src/linux-2.6/defconfig-build/tools/objtool/objtool
CHECK   ../lib/test_bitops.c
CC [M]  lib/test_bitops.o
MODPOST 1 modules
CC [M]  lib/test_bitops.mod.o
LD [M]  lib/test_bitops.ko
make[1]: Leaving directory '/usr/src/linux-2.6/defconfig-build'


No warnings for me!

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

* Re: [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast
  2020-02-20 17:37 [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg
  2020-02-20 17:37 ` [PATCH v2 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
@ 2020-02-20 18:12 ` Peter Zijlstra
  2020-02-20 22:32   ` Jesse Brandeburg
  1 sibling, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2020-02-20 18:12 UTC (permalink / raw)
  To: Jesse Brandeburg
  Cc: tglx, mingo, bp, x86, linux-kernel, linux, andriy.shevchenko,
	dan.j.williams

On Thu, Feb 20, 2020 at 09:37:21AM -0800, Jesse Brandeburg wrote:
> Fix many sparse warnings when building with C=1.
> 
> When the kernel is compiled with C=1, there are lots of messages like:
>   arch/x86/include/asm/bitops.h:77:37: warning: cast truncates bits from constant value (ffffff7f becomes 7f)
> 

> @@ -72,9 +74,11 @@ static __always_inline void
>  arch_clear_bit(long nr, volatile unsigned long *addr)
>  {
>  	if (__builtin_constant_p(nr)) {
> +		u8 cmaski = ~CONST_MASK(nr);
> +
>  		asm volatile(LOCK_PREFIX "andb %1,%0"
>  			: CONST_MASK_ADDR(nr, addr)
> -			: "iq" ((u8)~CONST_MASK(nr)));
> +			: "iq" (cmaski));
>  	} else {
>  		asm volatile(LOCK_PREFIX __ASM_SIZE(btr) " %1,%0"
>  			: : RLONG_ADDR(addr), "Ir" (nr) : "memory");

Urgh, that's sad. So why doesn't this still generate a warning, ~ should
promote your u8 to int, and then you down-cast to u8 on assignment
again.

So now you have more lines, more ugly casts and exactly the same
generated code; where the win?

Perhaps you should write it like:

		: "iq" (0xFF ^ CONST_MASK(nr))

hmm?

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

* Re: [PATCH v2 2/2] lib: make a test module with set/clear bit
  2020-02-20 18:10   ` Peter Zijlstra
@ 2020-02-20 19:03     ` Jesse Brandeburg
  0 siblings, 0 replies; 6+ messages in thread
From: Jesse Brandeburg @ 2020-02-20 19:03 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: tglx, mingo, bp, x86, linux-kernel, linux, andriy.shevchenko,
	dan.j.williams

On Thu, 20 Feb 2020 19:10:33 +0100 Peter wrote:
> On Thu, Feb 20, 2020 at 09:37:22AM -0800, Jesse Brandeburg wrote:
> > Test some bit clears/sets to make sure assembly doesn't change.
> > Instruct Kbuild to build this file with extra warning level -Wextra,
> > to catch new issues, and also doesn't hurt to build with C=1.
> > 
> > This was used to test changes to arch/x86/include/asm/bitops.h.
> > 
> > Recommended usage:
> > make defconfig
> > scripts/config -m CONFIG_TEST_BITOPS
> > make modules_prepare
> > make C=1 W=1 lib/test_bitops.ko
> > objdump -S -d lib/test_bitops.ko  
> 
> I only applied this second patch:
> 
> # sparse --version
> 0.6.0 (Debian: 0.6.0-3+b1)

Thanks for the review! The module was created to verify assembly didn't
change before/after, but it's not a reproducer.  Let me fix that (I had
skipped that step as I originally didn't plan to upstream the
test-patch).



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

* Re: [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast
  2020-02-20 18:12 ` [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Peter Zijlstra
@ 2020-02-20 22:32   ` Jesse Brandeburg
  0 siblings, 0 replies; 6+ messages in thread
From: Jesse Brandeburg @ 2020-02-20 22:32 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: tglx, mingo, bp, x86, linux-kernel, linux, andriy.shevchenko,
	dan.j.williams

On Thu, 20 Feb 2020 19:12:36 +0100 Peter wrote:
> On Thu, Feb 20, 2020 at 09:37:21AM -0800, Jesse Brandeburg wrote:
> > @@ -72,9 +74,11 @@ static __always_inline void
> >  arch_clear_bit(long nr, volatile unsigned long *addr)
> >  {
> >  	if (__builtin_constant_p(nr)) {
> > +		u8 cmaski = ~CONST_MASK(nr);
> > +
> >  		asm volatile(LOCK_PREFIX "andb %1,%0"
> >  			: CONST_MASK_ADDR(nr, addr)
> > -			: "iq" ((u8)~CONST_MASK(nr)));
> > +			: "iq" (cmaski));
> 
> Urgh, that's sad. So why doesn't this still generate a warning, ~ should
> promote your u8 to int, and then you down-cast to u8 on assignment
> again.

My suspicion is that using the right size types on the lvalue causes
the compiler (and sparse) to know that the type and number of bits in
the "iq" statement is unambiguous.

> 
> So now you have more lines, more ugly casts and exactly the same
> generated code; where the win?

The win as I see it is that sparse (C=1) doesn't warn, but you're right,
it wasn't my first choice to do it the way I ended up with (see below) 

> Perhaps you should write it like:
> 
> 		: "iq" (0xFF ^ CONST_MASK(nr))
> 
> hmm?

Thanks! That works, for my tests at least. FWIW, at one point during
review I got some feedback from a build bot (zero day tester) that
certain compilers like gcc 7.5.0 interpret ~CONST_MASK(nr) into needing
32 bits. So I solved that issue by using correctly typed (and width)
local variables, but I didn't try the 0xff^ way at that time.

I'll change back to the simpler version of the changes (without locals)
and with your 0xff^ change, we'll see if anything comes up from build
bots.


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

end of thread, other threads:[~2020-02-20 22:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-20 17:37 [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Jesse Brandeburg
2020-02-20 17:37 ` [PATCH v2 2/2] lib: make a test module with set/clear bit Jesse Brandeburg
2020-02-20 18:10   ` Peter Zijlstra
2020-02-20 19:03     ` Jesse Brandeburg
2020-02-20 18:12 ` [PATCH v2 1/2] x86: fix bitops.h warning with a moved cast Peter Zijlstra
2020-02-20 22:32   ` Jesse Brandeburg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).