linuxkpi: Add parentheses to pacify -Wparentheses warnings from GCC.

[bz]

Accept for the mac80211 change; the xarray also seems ok.

[emaste]

mac80211 LGTM
I'd rather __xa_alloc_cyclic be deobfuscated

[• hselasky]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

It should be:
MPASS(mask > ((xa->flags & XA_FLAGS_ALLOC1) ? 1 : 0))

Ditto for next one.

[jhb]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

Eh, why the ternary at all? Just

MPASS(mask > (xa->flags & XA_FLAGS_ALLOC1));

should be sufficient in that case?

[• hselasky]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

#define	XA_FLAGS_ALLOC1 (1U << 2)

@jhb: This is not the same, when you look at the definition.

[wulf]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

MPASS(mask > ((xa->flags & XA_FLAGS_ALLOC1) ? 1 : 0))

This version looks good

[jhb]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

It doesn't matter. KASSERT uses if(), so the expanded expression is:

if (!(mask > (xa->flags & XA_FLAGS_ALLOC1)))
   panic(...)

It doesn't care about exact values of 0 or 1, just 0 and non-zero. More typical FreeBSD style would probably be to use '!= 0' to generate the boolean expression than a ternary.

[• hselasky]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

Remember there is a ">" greater operator there, that compare two values. Of course which values being compared matters!

"mask" is the one value, and the other one is "((xa->flags & XA_FLAGS_ALLOC1) ? 1 : 0)" 1 or 0. After your change the assert compares 0 or 4 with mask!

Is anything unclear?

[jhb]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

Oh, so it's not pacifying the warning, it's that the code is actually broken as-is? ?: has lower priority than >, so today the > is already evaluated first. And in fact, the > is even evaluated before the !=, so today what you have is:

((mask > (xa->flags & XA_FLAGS_ALLOC1)) != 0) ? 1 : 0

[• hselasky]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

Maybe, I'm not sure.

[emaste]

via godbolt.org,

((mask > (xa->flags & XA_FLAGS_ALLOC1)) != 0) ? 1 : 0

mov     rax, QWORD PTR [rbp-16]
mov     eax, DWORD PTR [rax]
shr     eax, 2
and     eax, 1
cmp     DWORD PTR [rbp-4], eax
setg    al
movzx   eax, al

mask > ((xa->flags & XA_FLAGS_ALLOC1) ? 1 : 0))

mov     rax, QWORD PTR [rbp-16]
mov     eax, DWORD PTR [rax]
shr     eax, 2
and     eax, 1
cmp     DWORD PTR [rbp-4], eax
setg    al
movzx   eax, al

mask > ((xa->flags & XA_FLAGS_ALLOC1) ? 1 : 0))

        mov     rax, QWORD PTR [rbp-16]
        mov     eax, DWORD PTR [rax]
        and     eax, 4
        mov     edx, eax
        mov     eax, DWORD PTR [rbp-4]
        cmp     edx, eax
        setb    al
        movzx   eax, al
        test    eax, eax
        je      .L2
        mov     eax, 1
        jmp     .L4
.L2:
        mov     eax, 0
.L4:

return ((mask > (xa->flags & XA_FLAGS_ALLOC1)) != 0) ? 1 : 0

        mov     rax, QWORD PTR [rbp-16]
        mov     eax, DWORD PTR [rax]
        and     eax, 4
        mov     edx, eax
        mov     eax, DWORD PTR [rbp-4]
        cmp     edx, eax
        setb    al
        movzx   eax, al
        test    eax, eax
        je      .L2
        mov     eax, 1
        jmp     .L4
.L2:
        mov     eax, 0
.L4:

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

Yes, this is fixing a bug not pacifying a warning. @jhb's example above compiles to the same as what is currently in the tree.

[wulf]

sys/compat/linuxkpi/common/src/linux_xarray.c

106 ↗

(On Diff #102273)

Oh, so it's not pacifying the warning, it's that the code is actually broken as-is? ?: has lower priority than >, so today the > is already evaluated first. And in fact, the > is even evaluated before the !=, so today what you have is:

((mask > (xa->flags & XA_FLAGS_ALLOC1)) != 0) ? 1 : 0

Yes, the code is actually broken. With all parentheses set in their places It should look like

MPASS(mask > (((xa->flags & XA_FLAGS_ALLOC1) != 0) ? 1 : 0));

Accidentally current broken code does almost the same check the proper code does.

[jhb]

Move xarray changes to a new change.

[jhb]

Moved the linux_xarray.c changes to D34197