| Message ID | 52090c8d-fa21-6f53-c33b-776c12338f62@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMU/x86: work around bogus gcc12 warning in hvm_gsi_eoi() | expand |
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.
--- 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); /*
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