As per [1] the expansion of the pirq_dpci() macro causes a -Waddress
controlled warning (enabled implicitly in our builds, if not by default)
tying the middle part of the involved conditional expression to the
surrounding boolean context. Work around this by introducing a local
inline function in the affected source file.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967
---
This is intended to replace an earlier patch by Andrew [2], open-coding
and then simplifying the macro in the one problematic place.

Note that, with pirq_dpci() presently used solely in the one file being
changed here, we could in principle also remove the #define and use just
an inline(?) function in this file. But then the macro would need
reinstating as soon as a use elsewhere would become necessary.

As to the inline - I think it's warranted here, but it goes against our
general policy of using inline only in header files. Hence I'd be okay
to drop it to avoid controversy.

[2] https://lists.xen.org/archives/html/xen-devel/2021-10/msg01635.html

--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -25,6 +25,18 @@
 #include <asm/hvm/support.h>
 #include <asm/io_apic.h>
 
+/*
+ * Gcc12 takes issue with pirq_dpci() being used in boolean context (see gcc
+ * bug 102967). While we can't replace the macro definition in the header by an
+ * inline function, we can do so here.
+ */
+static inline struct hvm_pirq_dpci *_pirq_dpci(struct pirq *pirq)
+{
+    return pirq_dpci(pirq);
+}
+#undef pirq_dpci
+#define pirq_dpci(pirq) _pirq_dpci(pirq)
+
 static DEFINE_PER_CPU(struct list_head, dpci_list);
 
 /*

On Fri, May 27, 2022 at 12:37:19PM +0200, Jan Beulich wrote:
> As per [1] the expansion of the pirq_dpci() macro causes a -Waddress
> controlled warning (enabled implicitly in our builds, if not by default)
> tying the middle part of the involved conditional expression to the
> surrounding boolean context. Work around this by introducing a local
> inline function in the affected source file.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967
> ---
> This is intended to replace an earlier patch by Andrew [2], open-coding
> and then simplifying the macro in the one problematic place.
> 
> Note that, with pirq_dpci() presently used solely in the one file being
> changed here, we could in principle also remove the #define and use just
> an inline(?) function in this file. But then the macro would need
> reinstating as soon as a use elsewhere would become necessary.
> 
> As to the inline - I think it's warranted here, but it goes against our
> general policy of using inline only in header files. Hence I'd be okay
> to drop it to avoid controversy.
> 
> [2] https://lists.xen.org/archives/html/xen-devel/2021-10/msg01635.html
> 
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -25,6 +25,18 @@
>  #include <asm/hvm/support.h>
>  #include <asm/io_apic.h>
>  
> +/*
> + * Gcc12 takes issue with pirq_dpci() being used in boolean context (see gcc
> + * bug 102967). While we can't replace the macro definition in the header by an
> + * inline function, we can do so here.
> + */
> +static inline struct hvm_pirq_dpci *_pirq_dpci(struct pirq *pirq)
> +{
> +    return pirq_dpci(pirq);
> +}
> +#undef pirq_dpci
> +#define pirq_dpci(pirq) _pirq_dpci(pirq)

That's fairly ugly.  Seeing as pirq_dpci is only used in hvm.c, would
it make sense to just convert the macro to be a static inline in that
file? (and remove pirq_dpci() from irq.h).

Thanks, Roger.

On 10.06.2022 09:20, Roger Pau Monné wrote:
> On Fri, May 27, 2022 at 12:37:19PM +0200, Jan Beulich wrote:
>> As per [1] the expansion of the pirq_dpci() macro causes a -Waddress
>> controlled warning (enabled implicitly in our builds, if not by default)
>> tying the middle part of the involved conditional expression to the
>> surrounding boolean context. Work around this by introducing a local
>> inline function in the affected source file.
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967
>> ---
>> This is intended to replace an earlier patch by Andrew [2], open-coding
>> and then simplifying the macro in the one problematic place.
>>
>> Note that, with pirq_dpci() presently used solely in the one file being
>> changed here, we could in principle also remove the #define and use just
>> an inline(?) function in this file. But then the macro would need
>> reinstating as soon as a use elsewhere would become necessary.

Did you read this before ...

>> As to the inline - I think it's warranted here, but it goes against our
>> general policy of using inline only in header files. Hence I'd be okay
>> to drop it to avoid controversy.
>>
>> [2] https://lists.xen.org/archives/html/xen-devel/2021-10/msg01635.html
>>
>> --- a/xen/drivers/passthrough/x86/hvm.c
>> +++ b/xen/drivers/passthrough/x86/hvm.c
>> @@ -25,6 +25,18 @@
>>  #include <asm/hvm/support.h>
>>  #include <asm/io_apic.h>
>>  
>> +/*
>> + * Gcc12 takes issue with pirq_dpci() being used in boolean context (see gcc
>> + * bug 102967). While we can't replace the macro definition in the header by an
>> + * inline function, we can do so here.
>> + */
>> +static inline struct hvm_pirq_dpci *_pirq_dpci(struct pirq *pirq)
>> +{
>> +    return pirq_dpci(pirq);
>> +}
>> +#undef pirq_dpci
>> +#define pirq_dpci(pirq) _pirq_dpci(pirq)
> 
> That's fairly ugly.  Seeing as pirq_dpci is only used in hvm.c, would
> it make sense to just convert the macro to be a static inline in that
> file? (and remove pirq_dpci() from irq.h).

... saying so? IOW I'm not entirely opposed, but I'm a little afraid we might
be setting us up for later trouble. 

Jan

On Fri, Jun 10, 2022 at 09:29:44AM +0200, Jan Beulich wrote:
> On 10.06.2022 09:20, Roger Pau Monné wrote:
> > On Fri, May 27, 2022 at 12:37:19PM +0200, Jan Beulich wrote:
> >> As per [1] the expansion of the pirq_dpci() macro causes a -Waddress
> >> controlled warning (enabled implicitly in our builds, if not by default)
> >> tying the middle part of the involved conditional expression to the
> >> surrounding boolean context. Work around this by introducing a local
> >> inline function in the affected source file.
> >>
> >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967
> >> ---
> >> This is intended to replace an earlier patch by Andrew [2], open-coding
> >> and then simplifying the macro in the one problematic place.
> >>
> >> Note that, with pirq_dpci() presently used solely in the one file being
> >> changed here, we could in principle also remove the #define and use just
> >> an inline(?) function in this file. But then the macro would need
> >> reinstating as soon as a use elsewhere would become necessary.
> 
> Did you read this before ...
> 
> >> As to the inline - I think it's warranted here, but it goes against our
> >> general policy of using inline only in header files. Hence I'd be okay
> >> to drop it to avoid controversy.
> >>
> >> [2] https://lists.xen.org/archives/html/xen-devel/2021-10/msg01635.html
> >>
> >> --- a/xen/drivers/passthrough/x86/hvm.c
> >> +++ b/xen/drivers/passthrough/x86/hvm.c
> >> @@ -25,6 +25,18 @@
> >>  #include <asm/hvm/support.h>
> >>  #include <asm/io_apic.h>
> >>  
> >> +/*
> >> + * Gcc12 takes issue with pirq_dpci() being used in boolean context (see gcc
> >> + * bug 102967). While we can't replace the macro definition in the header by an
> >> + * inline function, we can do so here.
> >> + */
> >> +static inline struct hvm_pirq_dpci *_pirq_dpci(struct pirq *pirq)
> >> +{
> >> +    return pirq_dpci(pirq);
> >> +}
> >> +#undef pirq_dpci
> >> +#define pirq_dpci(pirq) _pirq_dpci(pirq)
> > 
> > That's fairly ugly.  Seeing as pirq_dpci is only used in hvm.c, would
> > it make sense to just convert the macro to be a static inline in that
> > file? (and remove pirq_dpci() from irq.h).
> 
> ... saying so? IOW I'm not entirely opposed, but I'm a little afraid we might
> be setting us up for later trouble. 

Sorry, started replying yesterday but had to leave and left the reply
open.  Then when I came back this morning I just read the code and not
the commit message.

Hm, so ideally we would also then move dpci_pirq() to hvm.c in order
to match the move of pirq_dpci(), but that's not possible due to that
helper having other callers outside of hvm.c.

We could always export the function from hvm.c if we gained outside
callers.  In any case, I don't want to block you further on this, so:

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.