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

From: Christophe Leroy
Date: Fri Aug 23 2019 - 11:36:13 EST



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