Messages in this thread | | | Subject | Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. | From | Christophe Leroy <> | Date | Mon, 19 Aug 2019 17:05:46 +0200 |
| |
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 ?
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); }
00000090 <test_bug1>: 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 00000084 R_PPC_ADDR32 .text+0x00000090
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
| |