* Re: [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation
2017-03-30 23:22 [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation Ed Maste
@ 2017-03-30 16:09 ` Eric Blake
2017-03-30 16:12 ` Corey Minyard
1 sibling, 0 replies; 5+ messages in thread
From: Eric Blake @ 2017-03-30 16:09 UTC (permalink / raw)
To: Ed Maste, qemu-devel; +Cc: qemu-trivial
[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]
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
--
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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation
2017-03-30 23:22 [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation Ed Maste
2017-03-30 16:09 ` Eric Blake
@ 2017-03-30 16:12 ` Corey Minyard
2017-03-30 17:14 ` Ed Maste
1 sibling, 1 reply; 5+ messages in thread
From: Corey Minyard @ 2017-03-30 16:12 UTC (permalink / raw)
To: Ed Maste, qemu-devel; +Cc: qemu-trivial
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;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation
2017-03-30 16:12 ` Corey Minyard
@ 2017-03-30 17:14 ` Ed Maste
2017-03-30 19:45 ` Corey Minyard
0 siblings, 1 reply; 5+ messages in thread
From: Ed Maste @ 2017-03-30 17:14 UTC (permalink / raw)
To: minyard; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation
2017-03-30 17:14 ` Ed Maste
@ 2017-03-30 19:45 ` Corey Minyard
0 siblings, 0 replies; 5+ messages in thread
From: Corey Minyard @ 2017-03-30 19:45 UTC (permalink / raw)
To: Ed Maste, minyard; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation
@ 2017-03-30 23:22 Ed Maste
2017-03-30 16:09 ` Eric Blake
2017-03-30 16:12 ` Corey Minyard
0 siblings, 2 replies; 5+ messages in thread
From: Ed Maste @ 2017-03-30 23:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Ed Maste
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;
--
2.11.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-30 19:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 23:22 [Qemu-devel] [PATCH] ipmi: use parens to avoid unexpected macro var evaluation Ed Maste
2017-03-30 16:09 ` Eric Blake
2017-03-30 16:12 ` Corey Minyard
2017-03-30 17:14 ` Ed Maste
2017-03-30 19:45 ` Corey Minyard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.