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

[Christophe Leroy <christophe.leroy@c-s.fr>]

On 08/19/2019 03:45 PM, Segher Boessenkool wrote:
> On Mon, Aug 19, 2019 at 05:05:46PM +0200, Christophe Leroy wrote:
>> Le 19/08/2019 à 16:37, Segher Boessenkool a écrit :
>>> On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote:
>>>> Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
>>>>> On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote:
>>>>>> Note that we keep using an assembly text using "twi 31, 0, 0" for
>>>>>> inconditional traps because GCC drops all code after
>>>>>> __builtin_trap() when the condition is always true at build time.
>>>>>
>>>>> As I said, it can also do this for conditional traps, if it can prove
>>>>> the condition is always true.
>>>>
>>>> But we have another branch for 'always true' and 'always false' using
>>>> __builtin_constant_p(), which don't use __builtin_trap(). Is there
>>>> anything wrong with that ?:
>>>
>>> The compiler might not realise it is constant when it evaluates the
>>> __builtin_constant_p, but only realises it later.  As the documentation
>>> for the builtin says:
>>>    A return of 0 does not indicate that the
>>>    value is _not_ a constant, but merely that GCC cannot prove it is a
>>>    constant with the specified value of the '-O' option.
>>
>> So you mean GCC would not be able to prove that
>> __builtin_constant_p(cond) is always true but it would be able to prove
>> that if (cond)  is always true ?
> 
> Not sure what you mean, sorry.
> 
>> And isn't there a away to tell GCC that '__builtin_trap()' is
>> recoverable in our case ?
> 
> No, GCC knows that a trap will never fall through.
> 
>>> I think it may work if you do
>>>
>>> #define BUG_ON(x) do {						\
>>> 	if (__builtin_constant_p(x)) {				\
>>> 		if (x)						\
>>> 			BUG();					\
>>> 	} else {						\
>>> 		BUG_ENTRY("", 0);				\
>>> 		if (x)						\
>>> 			__builtin_trap();			\
>>> 	}							\
>>> } while (0)
>>
>> It doesn't work:
> 
> You need to make a BUG_ENTRY so that it refers to the *following* trap
> instruction, if you go this way.
> 
>>> I don't know how BUG_ENTRY works exactly.
>>
>> It's basic, maybe too basic: it adds an inline asm with a label, and
>> adds a .long in the __bug_table section with the address of that label.
>>
>> When putting it after the __builtin_trap(), I changed it to using the
>> address before the one of the label which is always the twxx instruction
>> as far as I can see.
>>
>> #define BUG_ENTRY(insn, flags, ...)			\
>> 	__asm__ __volatile__(				\
>> 		"1:	" insn "\n"			\
>> 		".section __bug_table,\"aw\"\n"		\
>> 		"2:\t" PPC_LONG "1b, %0\n"		\
>> 		"\t.short %1, %2\n"			\
>> 		".org 2b+%3\n"				\
>> 		".previous\n"				\
>> 		: : "i" (__FILE__), "i" (__LINE__),	\
>> 		  "i" (flags),				\
>> 		  "i" (sizeof(struct bug_entry)),	\
>> 		  ##__VA_ARGS__)
> 
> #define MY_BUG_ENTRY(lab, flags)			\
> 	__asm__ __volatile__(				\
> 		".section __bug_table,\"aw\"\n"		\
> 		"2:\t" PPC_LONG "%4, %0\n"		\
> 		"\t.short %1, %2\n"			\
> 		".org 2b+%3\n"				\
> 		".previous\n"				\
> 		: : "i" (__FILE__), "i" (__LINE__),	\
> 		  "i" (flags),				\
> 		  "i" (sizeof(struct bug_entry)),	\
> 		  "i" (lab))
> 
> called as
> 
> #define BUG_ON(x) do {						\
> 	MY_BUG_ENTRY(&&lab, 0);					\
> 	lab: if (x)						\
> 		__builtin_trap();				\
> } while (0)
> 
> not sure how reliable that works -- *if* it works, I just typed that in
> without testing or anything -- but hopefully you get the idea.
> 

I've not been able to make it work. GCC puts the label (.L2 and .L6) 
outside of the function, so the instruction preceding the label is blr, 
not the trap.

#define _EMIT_BUG_ENTRY				\
	".section __bug_table,\"aw\"\n"		\
	"2:\t" PPC_LONG "%4, %0\n"		\
	"\t.short %1, %2\n"			\
	".org 2b+%3\n"				\
	".previous\n"

#define BUG_ENTRY(flags, label)				\
	__asm__ __volatile__(				\
		_EMIT_BUG_ENTRY				\
		: : "i" (__FILE__), "i" (__LINE__),	\
		  "i" (flags),				\
		  "i" (sizeof(struct bug_entry)),	\
		  "i" (label - 4))

#define __recoverable_trap()	asm volatile ("twi 31, 0, 0;");

#define __WARN_FLAGS(flags) do {				\
	__label__ label;					\
	BUG_ENTRY(BUGFLAG_WARNING | (flags), &&label);		\
	__recoverable_trap();					\
	label: ;						\
} while (0)

#define WARN_ON(x) ({						\
	int __ret_warn_on = !!(x);				\
	if (__builtin_constant_p(__ret_warn_on)) {		\
		if (__ret_warn_on)				\
			__WARN_TAINT(TAINT_WARN);		\
	} else {						\
		__label__ label;				\
		BUG_ENTRY(BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), &&label);	\
		if (__ret_warn_on)				\
			__builtin_trap();			\
		label: ;					\
	}							\
	unlikely(__ret_warn_on);				\
})

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

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

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

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

RELOCATION RECORDS FOR [__bug_table]:
OFFSET   TYPE              VALUE
00000000 R_PPC_ADDR32      .text+0x00000008
00000004 R_PPC_ADDR32      .rodata.str1.4
0000000c R_PPC_ADDR32      .text+0x00000010
00000010 R_PPC_ADDR32      .rodata.str1.4


	.file	"test.c"
	.section	".text"
.Ltext0:
	.align 2
	.globl test_warn1
	.type	test_warn1, @function
test_warn1:
.LFB598:
	.file 1 "arch/powerpc/mm/test.c"
	.loc 1 34 0
.LBB2:
.LBB3:
	.loc 1 35 0
#APP
  # 35 "arch/powerpc/mm/test.c" 1
	.section __bug_table,"aw"
2:	.long .L2-4, .LC0
	.short 35, 2305
.org 2b+12
.previous

  # 0 "" 2
#NO_APP
	or 3,3,4
	twnei 3,0
	blr
.L3:
.L2:
.LBE3:
.LBE2:
.LFE598:
	.size	test_warn1, .-test_warn1
	.align 2
	.globl test_warn2
	.type	test_warn2, @function
test_warn2:
.LFB599:
	.loc 1 39 0
.LBB4:
.LBB5:
	.loc 1 40 0
#APP
  # 40 "arch/powerpc/mm/test.c" 1
	.section __bug_table,"aw"
2:	.long .L6-4, .LC0
	.short 40, 2305
.org 2b+12
.previous

  # 0 "" 2
#NO_APP
	twnei 3,0
	blr
.L7:
.L6:
.LBE5:
.LBE4:
.LFE599:

Any idea ?

Christophe