WARN_ON(1) generates ugly code since commit 6b15f678fb7d

[Christophe Leroy]

Hi Drew,

I recently noticed gcc suddenly generating ugly code for WARN_ON(1).

It looks like commit 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut 
here" for WARN_ON for __WARN_TAINT architectures") is the culprit.

unsigned long test_mul1(unsigned long a, unsigned long b)
{
ÂÂÂ unsigned long long r = (unsigned long long)a * (unsigned long long)b;

ÂÂÂ if (r > 0xffffffff)
ÂÂÂ ÂÂÂ WARN_ON(1);

ÂÂÂ return r;
}

Before that patch, I was getting the following code:

00000008 <test_mul1>:
 8: 7d 23 20 16  mulhwu r9,r3,r4
ÂÂ c:ÂÂÂ 7c 63 21 d6 ÂÂÂ mullwÂÂ r3,r3,r4
 10: 2f 89 00 00  cmpwi cr7,r9,0
 14: 4d 9e 00 20  beqlr cr7
 18: 0f e0 00 00  twui r0,0
 1c: 4e 80 00 20  blr

Now I get:

0000002c <test_mul1>:
 2c: 7d 23 20 16  mulhwu r9,r3,r4
 30: 94 21 ff f0  stwu r1,-16(r1)
 34: 7c 08 02 a6  mflr r0
 38: 93 e1 00 0c  stw r31,12(r1)
 3c: 90 01 00 14  stw r0,20(r1)
 40: 7f e3 21 d6  mullw r31,r3,r4
 44: 2f 89 00 00  cmpwi cr7,r9,0
 48: 40 9e 00 1c  bne cr7,64 <test_mul1+0x38>
 4c: 80 01 00 14  lwz r0,20(r1)
 50: 7f e3 fb 78  mr r3,r31
 54: 83 e1 00 0c  lwz r31,12(r1)
 58: 7c 08 03 a6  mtlr r0
 5c: 38 21 00 10  addi r1,r1,16
 60: 4e 80 00 20  blr
 64: 3c 60 00 00  lis r3,0
ÂÂÂ ÂÂÂ ÂÂÂ 66: R_PPC_ADDR16_HAÂÂÂ .rodata.str1.4
 68: 38 63 00 00  addi r3,r3,0
ÂÂÂ ÂÂÂ ÂÂÂ 6a: R_PPC_ADDR16_LOÂÂÂ .rodata.str1.4
 6c: 48 00 00 01  bl 6c <test_mul1+0x40>
ÂÂÂ ÂÂÂ ÂÂÂ 6c: R_PPC_REL24ÂÂÂ printk
 70: 0f e0 00 00  twui r0,0
 74: 4b ff ff d8  b 4c <test_mul1+0x20>

As you can see, a call to printk() is added, which means setting up a 
stack frame, saving volatile registers, etc ...
That's all the things we want to avoid when using WARN_ON().

And digging a bit more, I see that you are only adding this 'cut here' 
to calls like WARN_ON(1), ie where the condition is a constant.
For calls where the condition is not a constant, there is no change and 
no 'cut here' line added:

unsigned long test_mul2(unsigned long a, unsigned long b)
{
ÂÂÂ unsigned long long r = (unsigned long long)a * (unsigned long long)b;

ÂÂÂ WARN_ON(r > 0xffffffff);

ÂÂÂ return r;
}

Before and after your patch, the code is clean and no call to add any 
'cut here' line.
00000078 <test_mul2>:
 78: 7d 43 20 16  mulhwu r10,r3,r4
 7c: 7c 63 21 d6  mullw r3,r3,r4
 80: 31 2a ff ff  addic r9,r10,-1
 84: 7d 29 51 10  subfe r9,r9,r10
 88: 0f 09 00 00  twnei r9,0
 8c: 4e 80 00 20  blr


Was it your intention to modify the behaviour and kill the lightweight 
implementations of WARN_ON() ?

Looking into arch/powerpc/include/bug.h, I see that when the condition 
is constant, WARN_ON() uses __WARN(), which itself calls __WARN_FLAGS() 
with relevant flags.

In the old days, __WARN() was implemented in arch/powerpc/include/bug.h
Commit b2be05273a17 ("panic: Allow warnings to set different taint 
flags") replaced __WARN() by __WARN_TAINT() and added a generic 
definition of __WARN()
In the begining I thought the __WARN() call in 
arch/powerpc/include/bug.h was forgotten, but looking into the commit in 
full, it looks like it was intentional to make __WARN() generic and have 
arches use it.

Then commit 19d436268dde ("debug: Add _ONCE() logic to report_bug()") 
replaced __WARN_TAINT() by __WARN_FLAGS().

So by changing the generic __WARN() you are impacting all users include 
those using 'trap' like instruction in order to avoid function calls.

What is to be done for getting back a clean code which doesn't call 
printk() on the hot path ?

Thanks,
Christophe