Messages in this thread | | | Subject | Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. | From | Christophe Leroy <> | Date | Fri, 23 Aug 2019 15:35:31 +0000 |
| |
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
| |