lkml.org 
[lkml]   [2019]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
From
Date


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

\
 
 \ /
  Last update: 2019-08-19 17:06    [W:0.740 / U:1.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site