lkml.org 
[lkml]   [2019]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectWARN_ON(1) generates ugly code since commit 6b15f678fb7d
Date
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



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