Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
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 +, 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(&, 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"
Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
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 +, 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(&, 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.
Segher
Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
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 +, 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 ?
And isn't there a away to tell GCC that '__builtin_trap()' is
recoverable in our case ?
(and there should be many more and more serious warnings here).
#define BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
} else {\
if (x) \
__builtin_trap(); \
BUG_ENTRY("", 0); \
} \
} while (0)
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:
void test_bug1(unsigned long long a)
{
BUG_ON(a);
}
0090 :
90: 7c 63 23 78 or r3,r3,r4
94: 0f 03 00 00 twnei r3,0
98: 4e 80 00 20 blr
RELOCATION RECORDS FOR [__bug_table]:
OFFSET TYPE VALUE
0084 R_PPC_ADDR32 .text+0x0090
As you see, the relocation in __bug_table points to the 'or' and not to
the 'twnei'.
or even just
#define BUG_ON(x) do { \
BUG_ENTRY("", 0); \
if (x) \
__builtin_trap(); \
} \
} while (0)
if BUG_ENTRY can work for the trap insn *after* it.
Can you put the bug table asm *before* the __builtin_trap maybe? That
should make it all work fine... If you somehow can tell what machine
instruction is that trap, anyway.
And how can I tell that ?
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__)
Christophe
Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
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 +, 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.
(and there should be many more and more serious warnings here).
> #define BUG_ON(x) do {\
> if (__builtin_constant_p(x)) { \
> if (x) \
> BUG(); \
> } else {\
> if (x) \
> __builtin_trap(); \
> BUG_ENTRY("", 0); \
> } \
> } while (0)
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)
or even just
#define BUG_ON(x) do { \
BUG_ENTRY("", 0); \
if (x) \
__builtin_trap(); \
} \
} while (0)
if BUG_ENTRY can work for the trap insn *after* it.
> >Can you put the bug table asm *before* the __builtin_trap maybe? That
> >should make it all work fine... If you somehow can tell what machine
> >instruction is that trap, anyway.
>
> And how can I tell that ?
I don't know how BUG_ENTRY works exactly.
Segher
Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
Le 19/08/2019 à 15:23, Segher Boessenkool a écrit :
On Mon, Aug 19, 2019 at 01:06:31PM +, 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 ?:
#define BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
} else {\
if (x) \
__builtin_trap(); \
BUG_ENTRY("", 0); \
} \
} 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 {\
if (__ret_warn_on) \
__builtin_trap(); \
BUG_ENTRY("", BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN)); \
} \
unlikely(__ret_warn_on);\
})
Can you put the bug table asm *before* the __builtin_trap maybe? That
should make it all work fine... If you somehow can tell what machine
instruction is that trap, anyway.
And how can I tell that ?
When I put it *after*, it always points to the trap instruction. When I
put it *before* it usually points on the first instruction used to
prepare the registers for the trap condition.
Christophe
Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
On Mon, Aug 19, 2019 at 01:06:31PM +, 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. Can you put the bug table asm *before* the __builtin_trap maybe? That should make it all work fine... If you somehow can tell what machine instruction is that trap, anyway. Segher
[PATCH 3/3] 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);
}
:
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
0014 :
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
0024 :
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
0038 :
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
0048 :
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
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
R_PPC_ADDR32 .text+0x000c
000c R_PPC_ADDR32 .text+0x001c
0018 R_PPC_ADDR32 .text+0x0030
0018 R_PPC_ADDR32 .text+0x0030
0024 R_PPC_ADDR32 .text+0x0040
0030 R_PPC_ADDR32 .text+0x005c
Using __builtin_trap() instead of inline assembly of twnei/tdnei
provides a far better result:
:
0: 7c 64 23 78 or r4,r3,r4
4: 0f 04 00 00 twnei r4,0
8: 4e 80 00 20 blr
000c :
c: 0f 03 00 00 twnei r3,0
10: 4e 80 00 20 blr
0014 :
14: 7c 43 20 08 twllt r3,r4
18: 4e 80 00 20 blr
001c :
1c: 0c 83 00 00 tweqi r3,0
20: 4e 80 00 20 blr
0024 :
24: 2f 83 00 00 cmpwi cr7,r3,0
28: 41 9e 00 08 beq cr7,30
2c: 0c 84 00 00 tweqi r4,0
30: 4e 80 00 20 blr
RELOCATION RECORDS FOR [__bug_table]:
OFFSET TYPE VALUE
R_PPC_ADDR32 .text+0x0004
000c R_PPC_ADDR32 .text+0x000c
0018 R_PPC_ADDR32 .text+0x0014
0024 R_PPC_ADDR32 .text+0x001c
0030 R_PPC_ADDR32 .text+0x002c
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.
In addition, this patch also fixes bugs in the BUG_ON(x) macro
which unlike WARN_ON(x) uses (x) directly as the condition by
forcing it to long instead of using !!(x). This leads to
upper part of an unsigned long long being ignored on PPC32 and
may produce bugs on PPC64 if (x) is a smaller type like an int.
Signed-off-by: Christophe Leroy
---
arch/powerpc/include/asm/bug.h | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index dbf7da90f507..a229130ffcf9 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"
@@ -59,7 +59,8 @@
#define BUG_ENTRY(insn, flags, ...)\
__asm__ __volatile__( \
- "1: " insn "\n" \
+ insn "\n" \
+ "1:\n" \
_EMIT_BUG_ENTRY \
: : "i" (__FILE__), "i" (__LINE__), \
"i" (flags), \
@@ -82,7 +83,9 @@
if (x) \
BUG(); \
} else {\
- BUG_ENTRY(PPC_TLNEI " %4, 0",
