Unlike BUG_ON(x), WARN_ON(x) uses !!(x) as the trigger
of the t(d/w)nei instruction instead of using directly the
value of x.
This leads to GCC adding unnecessary pair of addic/subfe. This was
revealed after adding a WARN_ON() on top of clear_page() in order
to detect misaligned destination:
@@ -49,6 +51,8 @@ static inline void clear_page(void *addr) { unsigned int i;+ WARN_ON((unsigned long)addr & (L1_CACHE_BYTES - 1));+ for (i = 0; i < PAGE_SIZE / L1_CACHE_BYTES; i++, addr += L1_CACHE_BYTES) dcbz(addr); }
On Sat, Aug 17, 2019 at 09:04:42AM +0000, Christophe Leroy wrote:
Unlike BUG_ON(x), WARN_ON(x) uses !!(x) as the trigger
of the t(d/w)nei instruction instead of using directly the
value of x.
This leads to GCC adding unnecessary pair of addic/subfe.
And it has to, it is passed as an "r" to an asm, GCC has to put the "!!"
value into a register.
By using (x) instead of !!(x) like BUG_ON() does, the additional
instructions go away:
But is it correct? What happens if you pass an int to WARN_ON, on a
64-bit kernel?
(You might want to have 64-bit generate either tw or td. But, with
your __builtin_trap patch, all that will be automatic).
Segher
Le 18/08/2019 à 14:01, Segher Boessenkool a écrit :
On Sat, Aug 17, 2019 at 09:04:42AM +0000, Christophe Leroy wrote:
quoted
Unlike BUG_ON(x), WARN_ON(x) uses !!(x) as the trigger
of the t(d/w)nei instruction instead of using directly the
value of x.
This leads to GCC adding unnecessary pair of addic/subfe.
And it has to, it is passed as an "r" to an asm, GCC has to put the "!!"
value into a register.
quoted
By using (x) instead of !!(x) like BUG_ON() does, the additional
instructions go away:
But is it correct? What happens if you pass an int to WARN_ON, on a
64-bit kernel?
On a 64-bit kernel, an int is still in a 64-bit register, so there would
be no problem with tdnei, would it ? an int 0 is the same as an long 0,
right ?
It is on 32-bit kernel that I see a problem, if one passes a long long
to WARN_ON(), the forced cast to long will just drop the upper size of
it. So as of today, BUG_ON() is buggy for that.
(You might want to have 64-bit generate either tw or td. But, with
your __builtin_trap patch, all that will be automatic).
Yes I'll discard this patch and focus on the __builtin_trap() one which
should solve most issues.
Christophe
On Mon, Aug 19, 2019 at 07:40:42AM +0200, Christophe Leroy wrote:
Le 18/08/2019 à 14:01, Segher Boessenkool a écrit :
quoted
On Sat, Aug 17, 2019 at 09:04:42AM +0000, Christophe Leroy wrote:
quoted
Unlike BUG_ON(x), WARN_ON(x) uses !!(x) as the trigger
of the t(d/w)nei instruction instead of using directly the
value of x.
This leads to GCC adding unnecessary pair of addic/subfe.
And it has to, it is passed as an "r" to an asm, GCC has to put the "!!"
value into a register.
quoted
By using (x) instead of !!(x) like BUG_ON() does, the additional
instructions go away:
But is it correct? What happens if you pass an int to WARN_ON, on a
64-bit kernel?
On a 64-bit kernel, an int is still in a 64-bit register, so there would
be no problem with tdnei, would it ? an int 0 is the same as an long 0,
right ?
The top 32 bits of a 64-bit register holding an int are undefined. Take
as example
int x = 42;
x = ~x;
which may put ffff_ffff_ffff_ffd5 into the reg, not 0000_0000_ffff_ffd5
as you might expect or want. For tw instructions this makes no difference
(they only look at the low 32 bits anyway); for td insns, it does.
It is on 32-bit kernel that I see a problem, if one passes a long long
to WARN_ON(), the forced cast to long will just drop the upper size of
it. So as of today, BUG_ON() is buggy for that.
Sure, it isn't defined what types you can pass to that macro. Another
thing that makes inline functions much saner to use.
quoted
(You might want to have 64-bit generate either tw or td. But, with
your __builtin_trap patch, all that will be automatic).
Yes I'll discard this patch and focus on the __builtin_trap() one which
should solve most issues.
But see my comment there about the compiler knowing all code after an
unconditional trap is dead.
Segher