[RFC PATCH] powerpc: use __builtin_trap() in BUG/WARN macros.

The below exemples of use of WARN_ON() show that the result
is sub-optimal in regard of the capabilities of powerpc.

void test_warn1(unsigned long long a)
{
	WARN_ON(a);
}

void test_warn2(unsigned long a)
{
	WARN_ON(a);
}

void test_warn3(unsigned long a, unsigned long b)
{
	WARN_ON(a < b);
}

void test_warn4(unsigned long a, unsigned long b)
{
	WARN_ON(!a);
}

void test_warn5(unsigned long a, unsigned long b)
{
	WARN_ON(!a && b);
}

00000000 <test_warn1>:
   0:	7c 64 23 78 	or      r4,r3,r4
   4:	31 24 ff ff 	addic   r9,r4,-1
   8:	7c 89 21 10 	subfe   r4,r9,r4
   c:	0f 04 00 00 	twnei   r4,0
  10:	4e 80 00 20 	blr

00000014 <test_warn2>:
  14:	31 23 ff ff 	addic   r9,r3,-1
  18:	7c 69 19 10 	subfe   r3,r9,r3
  1c:	0f 03 00 00 	twnei   r3,0
  20:	4e 80 00 20 	blr

00000024 <test_warn3>:
  24:	7c 84 18 10 	subfc   r4,r4,r3
  28:	7d 29 49 10 	subfe   r9,r9,r9
  2c:	7d 29 00 d0 	neg     r9,r9
  30:	0f 09 00 00 	twnei   r9,0
  34:	4e 80 00 20 	blr

00000038 <test_warn4>:
  38:	7c 63 00 34 	cntlzw  r3,r3
  3c:	54 63 d9 7e 	rlwinm  r3,r3,27,5,31
  40:	0f 03 00 00 	twnei   r3,0
  44:	4e 80 00 20 	blr

00000048 <test_warn5>:
  48:	2f 83 00 00 	cmpwi   cr7,r3,0
  4c:	39 20 00 00 	li      r9,0
  50:	41 9e 00 0c 	beq     cr7,5c <test_warn5+0x14>
  54:	7c 84 00 34 	cntlzw  r4,r4
  58:	54 89 d9 7e 	rlwinm  r9,r4,27,5,31
  5c:	0f 09 00 00 	twnei   r9,0
  60:	4e 80 00 20 	blr

RELOCATION RECORDS FOR [__bug_table]:
OFFSET   TYPE              VALUE
00000000 R_PPC_ADDR32      .text+0x0000000c
0000000c R_PPC_ADDR32      .text+0x0000001c
00000018 R_PPC_ADDR32      .text+0x00000030
00000018 R_PPC_ADDR32      .text+0x00000030
00000024 R_PPC_ADDR32      .text+0x00000040
00000030 R_PPC_ADDR32      .text+0x0000005c

Using __builtin_trap() instead of inline assembly of twnei/tdnei
provides a far better result:

00000000 <test_warn1>:
   0:	7c 64 23 78 	or      r4,r3,r4
   4:	0f 04 00 00 	twnei   r4,0
   8:	4e 80 00 20 	blr

0000000c <test_warn2>:
   c:	0f 03 00 00 	twnei   r3,0
  10:	4e 80 00 20 	blr

00000014 <test_warn3>:
  14:	7c 43 20 08 	twllt   r3,r4
  18:	4e 80 00 20 	blr

0000001c <test_warn4>:
  1c:	0c 83 00 00 	tweqi   r3,0
  20:	4e 80 00 20 	blr

00000024 <test_warn5>:
  24:	2f 83 00 00 	cmpwi   cr7,r3,0
  28:	41 9e 00 08 	beq     cr7,30 <test_warn5+0xc>
  2c:	0c 84 00 00 	tweqi   r4,0
  30:	4e 80 00 20 	blr

RELOCATION RECORDS FOR [__bug_table]:
OFFSET   TYPE              VALUE
00000000 R_PPC_ADDR32      .text+0x00000004
0000000c R_PPC_ADDR32      .text+0x0000000c
00000018 R_PPC_ADDR32      .text+0x00000014
00000024 R_PPC_ADDR32      .text+0x0000001c
00000030 R_PPC_ADDR32      .text+0x0000002c

Signed-off-by: Christophe Leroy <redacted>
---
 arch/powerpc/include/asm/bug.h | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index fed7e6241349..1a37c8d30b78 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h

@@ -44,14 +44,14 @@
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 #define _EMIT_BUG_ENTRY				\
 	".section __bug_table,\"aw\"\n"		\
-	"2:\t" PPC_LONG "1b, %0\n"		\
+	"2:\t" PPC_LONG "1b - 4, %0\n"		\
 	"\t.short %1, %2\n"			\
 	".org 2b+%3\n"				\
 	".previous\n"
 #else
 #define _EMIT_BUG_ENTRY				\
 	".section __bug_table,\"aw\"\n"		\
-	"2:\t" PPC_LONG "1b\n"			\
+	"2:\t" PPC_LONG "1b - 4\n"			\
 	"\t.short %2\n"				\
 	".org 2b+%3\n"				\
 	".previous\n"

@@ -64,8 +64,9 @@
  */
 
 #define BUG() do {						\
+	__builtin_trap();					\
 	__asm__ __volatile__(					\
-		"1:	twi 31,0,0\n"				\
+		"1:\n"						\
 		_EMIT_BUG_ENTRY					\
 		: : "i" (__FILE__), "i" (__LINE__),		\
 		    "i" (0), "i"  (sizeof(struct bug_entry)));	\

@@ -77,18 +78,20 @@
 		if (x)						\
 			BUG();					\
 	} else {						\
+		if (x)						\
+			__builtin_trap();			\
 		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
+		"1:\n"						\
 		_EMIT_BUG_ENTRY					\
 		: : "i" (__FILE__), "i" (__LINE__), "i" (0),	\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" ((__force long)(x)));			\
+		  "i" (sizeof(struct bug_entry)));		\
 	}							\
 } while (0)
 
 #define __WARN_FLAGS(flags) do {				\
+	__builtin_trap();					\
 	__asm__ __volatile__(					\
-		"1:	twi 31,0,0\n"				\
+		"1:\n"						\
 		_EMIT_BUG_ENTRY					\
 		: : "i" (__FILE__), "i" (__LINE__),		\
 		  "i" (BUGFLAG_WARNING|(flags)),		\

@@ -101,13 +104,14 @@
 		if (__ret_warn_on)				\
 			__WARN();				\
 	} else {						\
+		if (__ret_warn_on)				\
+			__builtin_trap();			\
 		__asm__ __volatile__(				\
-		"1:	"PPC_TLNEI"	%4,0\n"			\
+		"1:\n"			\
 		_EMIT_BUG_ENTRY					\
 		: : "i" (__FILE__), "i" (__LINE__),		\
 		  "i" (BUGFLAG_WARNING|BUGFLAG_TAINT(TAINT_WARN)),\
-		  "i" (sizeof(struct bug_entry)),		\
-		  "r" (__ret_warn_on));				\
+		  "i" (sizeof(struct bug_entry)));		\
 	}							\
 	unlikely(__ret_warn_on);				\
 })

-- 
2.17.1

Hi Christophe,

On Sat, Aug 17, 2019 at 06:37:50PM +0000, Christophe Leroy wrote:

 #define BUG() do {						\
+	__builtin_trap();					\

GCC will optimise away all code after this, it knows it is unreachable.
But you want to keep that BUG_ENTRY stuff I think?

The same will happen with a BUG_ON if the compiler can prove your
condition is always true.

 	__asm__ __volatile__(					\
-		"1:	twi 31,0,0\n"				\
+		"1:\n"						\
 		_EMIT_BUG_ENTRY					\
 		: : "i" (__FILE__), "i" (__LINE__),		\
 		    "i" (0), "i"  (sizeof(struct bug_entry)));	\

(GCC wil generate a different trap btw; what is called "trap" as extended
mnemonic, that is, "tw 31,0,0", not the same thing as "twi", if that
matters for the exception handler).


Segher