diff mbox

ipmi: use parens to avoid unexpected macro var evaluation

Message ID 20170330232236.8272-1-emaste@freebsd.org (mailing list archive)
State New, archived
Headers show

Commit Message

Ed Maste March 30, 2017, 11:22 p.m. UTC 
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)))
                                          ^ ~

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
 hw/ipmi/isa_ipmi_bt.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Eric Blake March 30, 2017, 4:09 p.m. UTC | #1 
On 03/30/2017 06:22 PM, Ed Maste wrote:
> 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));
>                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

You just re-found bug 1651167 reported in December:
https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg02704.html

> 
> expanded from macro 'IPMI_BT_SET_HBUSY'
>                                        (((v & 1) << IPMI_BT_HBUSY_BIT)))
>                                           ^ ~
> 
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
>  hw/ipmi/isa_ipmi_bt.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index 1c69cb33f8..c1dea503a2 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -40,37 +40,37 @@
>  #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)))
> +                                       ((((v) & 1) << IPMI_BT_CLR_WR_BIT)))

and still overparenthesized.  My proposal back then was:
https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg03095.html

> Better would be:
> 
> ((d) = (((d) & ~IPMI_BD_CLR_WR_MASK) | \
>         (((v) & 1) << IPMI_BT_CLR_WR_BIT)))

So why didn't we ever take v3 of the patch back then?
https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg03196.html
Corey Minyard March 30, 2017, 4:12 p.m. UTC | #2 
This isn't quite right, a lot of these need parenthesis around the whole
thing, and some of the macros are unused and need to be removed.
I had submitted something for this a while ago, but it hadn't been
taken.  I will re-submit.

-corey

On 03/30/2017 06:22 PM, Ed Maste wrote:
> 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)))
>                                            ^ ~
>
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
>   hw/ipmi/isa_ipmi_bt.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index 1c69cb33f8..c1dea503a2 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -40,37 +40,37 @@
>   #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)))
> +                                       ((((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)))
> +                                       ((((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)))
> +                                        ((((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)))
> +                                        ((((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)))
> +                                        ((((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)))
> +                                       ((((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)))
> +                                       ((((v) & 1) << IPMI_BT_BBUSY_BIT)))
>   
>   
>   /* Mask register */
> @@ -80,12 +80,12 @@
>   #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)))
> +                                        ((((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)))
> +                                        ((((v) & 1) << IPMI_BT_B2H_IRQ_BIT)))
>   
>   typedef struct IPMIBT {
>       IPMIBmc *bmc;
Ed Maste March 30, 2017, 5:14 p.m. UTC | #3 
On 30 March 2017 at 12:12, Corey Minyard <tcminyard@gmail.com> wrote:
> This isn't quite right, a lot of these need parenthesis around the whole
> thing, and some of the macros are unused and need to be removed.
> I had submitted something for this a while ago, but it hadn't been
> taken.  I will re-submit.

Ok, thanks and apologies for not noticing existing patch and discussion.

On the topic of building with recent Clang there are also a large
number of warnings of the form "taking the address of a packed member
'magic' of class or structure 'QCowHeader' may result in an unaligned
pointer value [-Waddress-of-packed-member]". Unfortunately this
warning seems to be overly aggressive and prone to false positives.
Corey Minyard March 30, 2017, 7:45 p.m. UTC | #4 
On 03/30/2017 12:14 PM, Ed Maste wrote:
> On 30 March 2017 at 12:12, Corey Minyard <tcminyard@gmail.com> wrote:
>> This isn't quite right, a lot of these need parenthesis around the whole
>> thing, and some of the macros are unused and need to be removed.
>> I had submitted something for this a while ago, but it hadn't been
>> taken.  I will re-submit.
> Ok, thanks and apologies for not noticing existing patch and discussion.

Not a problem, I had forgotten about this and it spurred me to action.

> On the topic of building with recent Clang there are also a large
> number of warnings of the form "taking the address of a packed member
> 'magic' of class or structure 'QCowHeader' may result in an unaligned
> pointer value [-Waddress-of-packed-member]". Unfortunately this
> warning seems to be overly aggressive and prone to false positives.

You should probably bring this up in a new thread.

-corey
diff mbox

Patch

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index 1c69cb33f8..c1dea503a2 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -40,37 +40,37 @@ 
 #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)))
+                                       ((((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)))
+                                       ((((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)))
+                                        ((((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)))
+                                        ((((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)))
+                                        ((((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)))
+                                       ((((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)))
+                                       ((((v) & 1) << IPMI_BT_BBUSY_BIT)))
 
 
 /* Mask register */
@@ -80,12 +80,12 @@ 
 #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)))
+                                        ((((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)))
+                                        ((((v) & 1) << IPMI_BT_B2H_IRQ_BIT)))
 
 typedef struct IPMIBT {
     IPMIBmc *bmc;