linux-kernel - Re: [PATCH] FDDI: defxx: simplify if-if to if-else

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6568e274-a013-4ab0-2c6d-228014e605b5@vivo.com>
Date:   Mon, 25 Apr 2022 11:27:46 +0800
From:   Jiabing Wan <wanjiabing@...o.com>
To:     "Maciej W. Rozycki" <macro@...am.me.uk>,
        Andrew Lunn <andrew@...n.ch>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kael_w@...h.net
Subject: Re: [PATCH] FDDI: defxx: simplify if-if to if-else



On 2022/4/25 7:26, Maciej W. Rozycki wrote:
> On Mon, 25 Apr 2022, Andrew Lunn wrote:
>
>>>   NAK.  The first conditional optionally sets `bp->mmio = false', which
>>> changes the value of `dfx_use_mmio' in some configurations:
>>>
>>> #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
>>> #define dfx_use_mmio bp->mmio
>>> #else
>>> #define dfx_use_mmio true
>>> #endif
Yes, it's my fault. I didn't notice "dfx_use_mmio" is a MACRO,
sorry for this wrong patch.
>> It probably won't stop the robots finding this if (x) if (!x), but
>> there is a chance the robot drivers will wonder why it is upper case.
>   Well, blindly relying on automation is bound to cause trouble.  There has
> to be a piece of intelligence signing the results off at the end.
You are right and I'll be more careful to review the result before 
submitting.
>
>   And there's nothing wrong with if (x) if (!x) in the first place; any
> sane compiler will produce reasonable output from it.  Don't fix what
> ain't broke!  And watch out for volatiles!

Yes, there's nothing wrong with if (x) if (!x), but I want to do is
reducing the complexity of the code.

There would be less instructions when using "if and else" rather
than "if (A) and if (!A)" as I tested:

Use if(A) and if(!A):
         ldr     w0, [sp, 28]
         cmp     w0, 0
         beq     .L2
         ldr     w0, [sp, 28]
         add     w0, w0, 1
         str     w0, [sp, 28]
.L2:
         ldr     w0, [sp, 28]   <------ one more ldr instruction
         cmp     w0, 0       <------ one more cmp instruction
         bne     .L3
         ldr     w0, [sp, 28]
         add     w0, w0, 2
         str     w0, [sp, 28]
.L3:
         ldr     w0, [sp, 28]
         mov     w1, w0
         adrp    x0, .LC1
         add     x0, x0, :lo12:.LC1
         bl      printf



Use if(A) and else:
         ldr     w0, [sp, 28]
         cmp     w0, 0
         beq     .L2
         ldr     w0, [sp, 28]
         add     w0, w0, 1
         str     w0, [sp, 28]    <------ reduce two instructions
         b       .L3
.L2:
         ldr     w0, [sp, 28]
         add     w0, w0, 2
         str     w0, [sp, 28]
.L3:
         ldr     w0, [sp, 28]
         mov     w1, w0
         adrp    x0, .LC1
         add     x0, x0, :lo12:.LC1
         bl      printf

I also use "pmccabe" , a tool from gcc, to calculate the complexity of the code.
It shows this patch can reduce the statements in function.

Use if(A) and if(!A):
pmccabe -v test.c Modified McCabe Cyclomatic Complexity
|   Traditional McCabe Cyclomatic Complexity
|       |    # Statements in function
|       |        |   First line of function
|       |        |       |   # lines in function
|       |        |       |       |  filename(definition line number):function
|       |        |       |       |           |
3       3       8       4       17      test.c(4): main

Use if(A) and else:
pmccabe -v test.c

Modified McCabe Cyclomatic Complexity
|   Traditional McCabe Cyclomatic Complexity
|       |    # Statements in function
|       |        |   First line of function
|       |        |       |   # lines in function
|       |        |       |       |  filename(definition line number):function
|       |        |       |       |           |
2       2       7       4       16      test.c(4): main

So I think this type of patchs is meaningful.

Thanks,
Wan Jiabing