Messages in this thread | | | Date | Mon, 19 Aug 2019 10:45:31 -0500 | From | Segher Boessenkool <> | Subject | 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 +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.
Segher
| |