[Qemu-devel] [PATCH v2 resend] ipmi: Fix macro issues

[Qemu-devel] [PATCH v2 resend] ipmi: Fix macro issues

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Macro parameters should almost always have () around them when used.
llvm reported an error on this.

Remove redundant parenthesis and put parenthesis around the entire
macros with assignments in case they are used in an expression.

Remove some unused macros.

Reported in https://bugs.launchpad.net/bugs/1651167

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/ipmi/isa_ipmi_bt.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index 1c69cb3..2fcc3d2 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -37,40 +37,30 @@
 #define IPMI_BT_HBUSY_BIT          6
 #define IPMI_BT_BBUSY_BIT          7
 
-#define IPMI_BT_CLR_WR_MASK        (1 << IPMI_BT_CLR_WR_BIT)
 #define IPMI_BT_GET_CLR_WR(d)      (((d) >> IPMI_BT_CLR_WR_BIT) & 0x1)
-#define IPMI_BT_SET_CLR_WR(d, v)   (d) = (((d) & ~IPMI_BT_CLR_WR_MASK) | \
-                                       (((v & 1) << IPMI_BT_CLR_WR_BIT)))
 
-#define IPMI_BT_CLR_RD_MASK        (1 << IPMI_BT_CLR_RD_BIT)
 #define IPMI_BT_GET_CLR_RD(d)      (((d) >> IPMI_BT_CLR_RD_BIT) & 0x1)
-#define IPMI_BT_SET_CLR_RD(d, v)   (d) = (((d) & ~IPMI_BT_CLR_RD_MASK) | \
-                                       (((v & 1) << IPMI_BT_CLR_RD_BIT)))
 
-#define IPMI_BT_H2B_ATN_MASK       (1 << IPMI_BT_H2B_ATN_BIT)
 #define IPMI_BT_GET_H2B_ATN(d)     (((d) >> IPMI_BT_H2B_ATN_BIT) & 0x1)
-#define IPMI_BT_SET_H2B_ATN(d, v)  (d) = (((d) & ~IPMI_BT_H2B_ATN_MASK) | \
-                                        (((v & 1) << IPMI_BT_H2B_ATN_BIT)))
 
 #define IPMI_BT_B2H_ATN_MASK       (1 << IPMI_BT_B2H_ATN_BIT)
 #define IPMI_BT_GET_B2H_ATN(d)     (((d) >> IPMI_BT_B2H_ATN_BIT) & 0x1)
-#define IPMI_BT_SET_B2H_ATN(d, v)  (d) = (((d) & ~IPMI_BT_B2H_ATN_MASK) | \
-                                        (((v & 1) << IPMI_BT_B2H_ATN_BIT)))
+#define IPMI_BT_SET_B2H_ATN(d, v)  ((d) = (((d) & ~IPMI_BT_B2H_ATN_MASK) | \
+                                        (((v) & 1) << IPMI_BT_B2H_ATN_BIT)))
 
 #define IPMI_BT_SMS_ATN_MASK       (1 << IPMI_BT_SMS_ATN_BIT)
 #define IPMI_BT_GET_SMS_ATN(d)     (((d) >> IPMI_BT_SMS_ATN_BIT) & 0x1)
-#define IPMI_BT_SET_SMS_ATN(d, v)  (d) = (((d) & ~IPMI_BT_SMS_ATN_MASK) | \
-                                        (((v & 1) << IPMI_BT_SMS_ATN_BIT)))
+#define IPMI_BT_SET_SMS_ATN(d, v)  ((d) = (((d) & ~IPMI_BT_SMS_ATN_MASK) | \
+                                        (((v) & 1) << IPMI_BT_SMS_ATN_BIT)))
 
 #define IPMI_BT_HBUSY_MASK         (1 << IPMI_BT_HBUSY_BIT)
 #define IPMI_BT_GET_HBUSY(d)       (((d) >> IPMI_BT_HBUSY_BIT) & 0x1)
-#define IPMI_BT_SET_HBUSY(d, v)    (d) = (((d) & ~IPMI_BT_HBUSY_MASK) | \
-                                       (((v & 1) << IPMI_BT_HBUSY_BIT)))
+#define IPMI_BT_SET_HBUSY(d, v)    ((d) = (((d) & ~IPMI_BT_HBUSY_MASK) | \
+                                       (((v) & 1) << IPMI_BT_HBUSY_BIT)))
 
 #define IPMI_BT_BBUSY_MASK         (1 << IPMI_BT_BBUSY_BIT)
-#define IPMI_BT_GET_BBUSY(d)       (((d) >> IPMI_BT_BBUSY_BIT) & 0x1)
-#define IPMI_BT_SET_BBUSY(d, v)    (d) = (((d) & ~IPMI_BT_BBUSY_MASK) | \
-                                       (((v & 1) << IPMI_BT_BBUSY_BIT)))
+#define IPMI_BT_SET_BBUSY(d, v)    ((d) = (((d) & ~IPMI_BT_BBUSY_MASK) | \
+                                       (((v) & 1) << IPMI_BT_BBUSY_BIT)))
 
 
 /* Mask register */
@@ -79,13 +69,13 @@
 
 #define IPMI_BT_B2H_IRQ_EN_MASK      (1 << IPMI_BT_B2H_IRQ_EN_BIT)
 #define IPMI_BT_GET_B2H_IRQ_EN(d)    (((d) >> IPMI_BT_B2H_IRQ_EN_BIT) & 0x1)
-#define IPMI_BT_SET_B2H_IRQ_EN(d, v) (d) = (((d) & ~IPMI_BT_B2H_IRQ_EN_MASK) | \
-                                        (((v & 1) << IPMI_BT_B2H_IRQ_EN_BIT)))
+#define IPMI_BT_SET_B2H_IRQ_EN(d, v) ((d) = (((d) & ~IPMI_BT_B2H_IRQ_EN_MASK) |\
+                                        (((v) & 1) << IPMI_BT_B2H_IRQ_EN_BIT)))
 
 #define IPMI_BT_B2H_IRQ_MASK         (1 << IPMI_BT_B2H_IRQ_BIT)
 #define IPMI_BT_GET_B2H_IRQ(d)       (((d) >> IPMI_BT_B2H_IRQ_BIT) & 0x1)
-#define IPMI_BT_SET_B2H_IRQ(d, v)    (d) = (((d) & ~IPMI_BT_B2H_IRQ_MASK) | \
-                                        (((v & 1) << IPMI_BT_B2H_IRQ_BIT)))
+#define IPMI_BT_SET_B2H_IRQ(d, v)    ((d) = (((d) & ~IPMI_BT_B2H_IRQ_MASK) | \
+                                        (((v) & 1) << IPMI_BT_B2H_IRQ_BIT)))
 
 typedef struct IPMIBT {
     IPMIBmc *bmc;
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9? v2 resend] ipmi: Fix macro issues

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

On 03/30/2017 12:28 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Macro parameters should almost always have () around them when used.
> llvm reported an error on this.
> 
> Remove redundant parenthesis and put parenthesis around the entire
> macros with assignments in case they are used in an expression.
> 
> Remove some unused macros.
> 
> Reported in https://bugs.launchpad.net/bugs/1651167
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/ipmi/isa_ipmi_bt.c | 34 ++++++++++++----------------------
>  1 file changed, 12 insertions(+), 22 deletions(-)

Already reviewed by me, so now I'm just adding commentary: Is this still
2.9 material?  It silences a build warning under clang, although I
didn't analyze whether the unpatched code actually caused an observable
behavior bug or just compiler noise.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9? v2 resend] ipmi: Fix macro issues

[Corey Minyard]

On 03/30/2017 12:53 PM, Eric Blake wrote:
> On 03/30/2017 12:28 PM, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> Macro parameters should almost always have () around them when used.
>> llvm reported an error on this.
>>
>> Remove redundant parenthesis and put parenthesis around the entire
>> macros with assignments in case they are used in an expression.
>>
>> Remove some unused macros.
>>
>> Reported in https://bugs.launchpad.net/bugs/1651167
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   hw/ipmi/isa_ipmi_bt.c | 34 ++++++++++++----------------------
>>   1 file changed, 12 insertions(+), 22 deletions(-)
> Already reviewed by me, so now I'm just adding commentary: Is this still
> 2.9 material?  It silences a build warning under clang, although I
> didn't analyze whether the unpatched code actually caused an observable
> behavior bug or just compiler noise.
>
I don't believe the code has a bug, going through all the uses there is
no expression used as a parameter or set used in an expression.

That said, it's also not going to hurt anything and it's nice to silence the
warnings.

-corey

Re: [Qemu-devel] [PATCH for-2.9? v2 resend] ipmi: Fix macro issues

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

On 03/30/2017 02:10 PM, Corey Minyard wrote:

>> Already reviewed by me, so now I'm just adding commentary: Is this still
>> 2.9 material?  It silences a build warning under clang, although I
>> didn't analyze whether the unpatched code actually caused an observable
>> behavior bug or just compiler noise.
>>
> I don't believe the code has a bug, going through all the uses there is
> no expression used as a parameter or set used in an expression.

Actually, let's re-read the clang warning, posted by Ed - we DO have an
expression used as a parameter:

> Found via Clang warning: logical not is only applied to the left hand
> side of this bitwise operator [-Wlogical-not-parentheses]
> 
>                               !IPMI_BT_GET_HBUSY(ib->control_reg));
>                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> expanded from macro 'IPMI_BT_SET_HBUSY'
>                                        (((v & 1) << IPMI_BT_HBUSY_BIT)))
>                                           ^ ~

But we STILL have to audit to see if that expression used as a parameter
causes a bug:

        if (IPMI_BT_GET_HBUSY(val)) {
            /* Toggle */
            IPMI_BT_SET_HBUSY(ib->control_reg,
                              !IPMI_BT_GET_HBUSY(ib->control_reg));
        }

with the expectation that it will toggle the bit.  But does it really?

Unpatched, it expands to this:

(ib->control_reg) = (((ib->control_reg) & ~IPMI_BT_HBUSY_MASK) |
  (((!IPMI_BT_GET_HBUSY(ib->control_reg) & 1) << IPMI_BT_HBUSY_BIT)))

Everything left of the | is okay, but to the right, this further expands to:

(((!(((ib->control_reg) >> IPMI_BT_HBUSY_BIT) & 0x1) & 1) <<
IPMI_BT_HBUSY_BIT))

Since the inner expression of IPMI_BT_GET_HBUSY(ib->control_reg) is
properly parenthesized and always 0 or 1, it boils down to either:

(!0 & 1) << shift // 1 & 1, results in changing the HBUSY from 0 to 1
(!1 & 1) << shift // 0 & 1, results in changing the HBUSY from 1 to 0

so we actually achieve the toggle we wanted.  If I'm reading it
correctly, the clang warning is asking if we instead meant:

(!(0 & 1)) << shift // !0, results in changing the HBUSY from 0 to 1
(!(1 & 1)) << shift // !1, results in changing the HBUSY from 1 to 0

which, perhaps surprisingly, would have the same end results for all our
inputs (but ONLY because the value we are passing to ! is already
limited to the range of 0 and 1).

Post-patch, we are changing to an expansion of:

((ib->control_reg) = (((ib->control_reg) & ~IPMI_BT_HBUSY_MASK) |
  (((!IPMI_BT_GET_HBUSY(ib->control_reg)) & 1) << IPMI_BT_HBUSY_BIT)))

Again, everything to the left of | is okay, to the right further expands to:

(((!(((ib->control_reg) >> IPMI_BT_HBUSY_BIT) & 0x1)) & 1) <<
IPMI_BT_HBUSY_BIT)

which now boils down to either:

(!(0) & 1) << shift
(!(1) & 1) << shift

where we are now being explicit that we want the ! bound only to the
left argument, and not to the overall (a&b) expression.  But we are not
changing semantics, and therefore not fixing an observable bug.

Note, on the other hand, that a call such as
IPMI_BT_SET_HBUSY(ib->control_reg, 2) would result in writing 0 to the
HBUSY bit. In other words, the IPMI_BT_SET_HBUSY() macro is rather weird
in that it sets or clears the HBUSY bit based solely on whether its v
parameter is even or odd, rather than the more usual semantics of
whether the v parameter is 0 or non-zero.  We could change that if we
wanted - by having the macro expand to "(left | (!!(v) << shift))"
instead of our current expansion of "(left | (((v) & 1) << shift))" -
but I still don't think it would change the semantics of any existing
caller.

> 
> That said, it's also not going to hurt anything and it's nice to silence
> the
> warnings.

I don't know if there were other warnings or just the one on the use of
IPMI_BT_SET_BUSY(); it may well be that auditing one of the other
warnings may turn up an actual behavioral change, but at this point, I'm
doubting that we are doing any more than shutting up the compiler, and
working around compiler noise is not the same level of bug fix as an
actual behavior change.  I'm not opposed to this patch going into 2.9,
but I don't see any problem if it slips to 2.10.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9? v2 resend] ipmi: Fix macro issues

[Corey Minyard]

On 03/30/2017 03:00 PM, Eric Blake wrote:
> On 03/30/2017 02:10 PM, Corey Minyard wrote:
>
>>> Already reviewed by me, so now I'm just adding commentary: Is this still
>>> 2.9 material?  It silences a build warning under clang, although I
>>> didn't analyze whether the unpatched code actually caused an observable
>>> behavior bug or just compiler noise.
>>>
>> I don't believe the code has a bug, going through all the uses there is
>> no expression used as a parameter or set used in an expression.
> Actually, let's re-read the clang warning, posted by Ed - we DO have an
> expression used as a parameter:
>
>> Found via Clang warning: logical not is only applied to the left hand
>> side of this bitwise operator [-Wlogical-not-parentheses]
>>
>>                                !IPMI_BT_GET_HBUSY(ib->control_reg));
>>                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> expanded from macro 'IPMI_BT_SET_HBUSY'
>>                                         (((v & 1) << IPMI_BT_HBUSY_BIT)))
>>                                            ^ ~
> But we STILL have to audit to see if that expression used as a parameter
> causes a bug:
>
>          if (IPMI_BT_GET_HBUSY(val)) {
>              /* Toggle */
>              IPMI_BT_SET_HBUSY(ib->control_reg,
>                                !IPMI_BT_GET_HBUSY(ib->control_reg));
>          }
>
> with the expectation that it will toggle the bit.  But does it really?
>
> Unpatched, it expands to this:
>
> (ib->control_reg) = (((ib->control_reg) & ~IPMI_BT_HBUSY_MASK) |
>    (((!IPMI_BT_GET_HBUSY(ib->control_reg) & 1) << IPMI_BT_HBUSY_BIT)))
>
> Everything left of the | is okay, but to the right, this further expands to:
>
> (((!(((ib->control_reg) >> IPMI_BT_HBUSY_BIT) & 0x1) & 1) <<
> IPMI_BT_HBUSY_BIT))
>
> Since the inner expression of IPMI_BT_GET_HBUSY(ib->control_reg) is
> properly parenthesized and always 0 or 1, it boils down to either:
>
> (!0 & 1) << shift // 1 & 1, results in changing the HBUSY from 0 to 1
> (!1 & 1) << shift // 0 & 1, results in changing the HBUSY from 1 to 0
>
> so we actually achieve the toggle we wanted.  If I'm reading it
> correctly, the clang warning is asking if we instead meant:
>
> (!(0 & 1)) << shift // !0, results in changing the HBUSY from 0 to 1
> (!(1 & 1)) << shift // !1, results in changing the HBUSY from 1 to 0
>
> which, perhaps surprisingly, would have the same end results for all our
> inputs (but ONLY because the value we are passing to ! is already
> limited to the range of 0 and 1).
>
> Post-patch, we are changing to an expansion of:
>
> ((ib->control_reg) = (((ib->control_reg) & ~IPMI_BT_HBUSY_MASK) |
>    (((!IPMI_BT_GET_HBUSY(ib->control_reg)) & 1) << IPMI_BT_HBUSY_BIT)))
>
> Again, everything to the left of | is okay, to the right further expands to:
>
> (((!(((ib->control_reg) >> IPMI_BT_HBUSY_BIT) & 0x1)) & 1) <<
> IPMI_BT_HBUSY_BIT)
>
> which now boils down to either:
>
> (!(0) & 1) << shift
> (!(1) & 1) << shift
>
> where we are now being explicit that we want the ! bound only to the
> left argument, and not to the overall (a&b) expression.  But we are not
> changing semantics, and therefore not fixing an observable bug.
>
> Note, on the other hand, that a call such as
> IPMI_BT_SET_HBUSY(ib->control_reg, 2) would result in writing 0 to the
> HBUSY bit. In other words, the IPMI_BT_SET_HBUSY() macro is rather weird
> in that it sets or clears the HBUSY bit based solely on whether its v
> parameter is even or odd, rather than the more usual semantics of
> whether the v parameter is 0 or non-zero.  We could change that if we
> wanted - by having the macro expand to "(left | (!!(v) << shift))"
> instead of our current expansion of "(left | (((v) & 1) << shift))" -
> but I still don't think it would change the semantics of any existing
> caller.

Yeah, that would be better.  Should I do another patch?

-corey

>> That said, it's also not going to hurt anything and it's nice to silence
>> the
>> warnings.
> I don't know if there were other warnings or just the one on the use of
> IPMI_BT_SET_BUSY(); it may well be that auditing one of the other
> warnings may turn up an actual behavioral change, but at this point, I'm
> doubting that we are doing any more than shutting up the compiler, and
> working around compiler noise is not the same level of bug fix as an
> actual behavior change.  I'm not opposed to this patch going into 2.9,
> but I don't see any problem if it slips to 2.10.
>

Re: [Qemu-devel] [PATCH for-2.9? v2 resend] ipmi: Fix macro issues

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On 03/31/2017 08:32 AM, Corey Minyard wrote:

>> Note, on the other hand, that a call such as
>> IPMI_BT_SET_HBUSY(ib->control_reg, 2) would result in writing 0 to the
>> HBUSY bit. In other words, the IPMI_BT_SET_HBUSY() macro is rather weird
>> in that it sets or clears the HBUSY bit based solely on whether its v
>> parameter is even or odd, rather than the more usual semantics of
>> whether the v parameter is 0 or non-zero.  We could change that if we
>> wanted - by having the macro expand to "(left | (!!(v) << shift))"
>> instead of our current expansion of "(left | (((v) & 1) << shift))" -
>> but I still don't think it would change the semantics of any existing
>> caller.
> 
> Yeah, that would be better.  Should I do another patch?

Up to you; I'm fine with either the existing patch as proposed or with
reviewing a v4 (interesting that you resent v2, even though you had also
posted a v3 at one point, where the only difference seems to have been
adding my R-b).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]