Messages in this thread | | | From | Christophe Leroy <> | Subject | WARN_ON(1) generates ugly code since commit 6b15f678fb7d | Date | Mon, 19 Aug 2019 10:18:57 +0200 |
| |
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
| |