On Thu, Nov 04, 2021 at 01:17:53PM +0100, Jan Beulich wrote:
> On 04.11.2021 11:48, Andrew Cooper wrote:
> > On 04/11/2021 08:07, Jan Beulich wrote:
> >> On 03.11.2021 17:13, Ian Jackson wrote:
> >>> Jan Beulich writes ("Re: [PATCH] x86/passthrough: Fix hvm_gsi_eoi() build
> >>> with GCC 12"):
> >>>> On 27.10.2021 22:07, Andrew Cooper wrote:
> >>>>> if ( !((pirq) ? &(pirq)->arch.hvm.dpci : NULL) )
> >>>> I disagree with the compiler's analysis: While &(pirq)->arch.hvm.dpci
> >>>> indeed can't be NULL, that's not the operand of !. The operand of !
> >>>> can very well be NULL, when pirq is.
> >>>>
> >>>>> which is a hint that the code is should be simplified to just:
> >>>>>
> >>>>> if ( !pirq )
> >>>>>
> >>>>> Do so.
> >>>> And I further agree with Roger's original reply (despite you
> >>>> apparently having managed to convince him): You shouldn't be open-
> >>>> coding pirq_dpci(). Your observation that the construct's result
> >>>> isn't otherwise used in the function is only one half of it. The
> >>>> other half is that hvm_pirq_eoi() gets called from here, and that
> >>>> one does require the result to be non-NULL.
> >>> Can you (collectively) please come to some agreement here ?
> >>> I think this is mostly a question of taste or style.
> >> Personally I don't think open-coding of constructs is merely a style /
> >> taste issue. The supposed construct changing and the open-coded
> >> instance then being forgotten (likely not even noticed) can lead to
> >> actual bugs. I guess the issue should at least be raised as one against
> >> gcc12, such that the compiler folks can provide their view on whether
> >> the warning is actually appropriate in that case (and if so, what their
> >> take is on a general approach towards silencing such warnings when
> >> they're identified as false positives).
> >
> > This isn't opencoding anything.
> >
> > The compiler has pointed out that the logic, as currently expressed, is
> > junk and doesn't do what you think it does.
> >
> > And based on the, IMO dubious, argument that both you and Roger have
> > used to try and defend the current code, I agree with the compiler.
> >
> > pirq_dpci() does not have the property that you seem expect of it,
>
> Which property is that, exactly?
I honestly don't think we expect any property of pirq_dpci, it just
tells whether a pirq has a dpci associated with it or not. As I said
on my previous replies I think GCC is not correct in doing the check
after expanding the macro.
The relation between a pirq and it's dpci is an implementation detail
that could change at any point, and hence the complain by GCC would no
longer be true. That's exactly why we use a macro to get the dpci out
of a pirq, because how that's obtained is opaque to the caller.
So while it's true that a NULL pirq will always result in a NULL dpci,
a non-NULL pirq should not be assumed to result in a non-NULL dpci,
which is inferred by GCC by expanding the macro.
In this specific case I think the change is fine for two reasons:
* The pirq is obtained from the domain struct.
* The domain is known to be HVM because the only caller of
hvm_gsi_eoi is hvm_dpci_eoi which in
turn is only called by the vIO-APIC and the vPIC emulation code.
I dislike of GCC doing those checks to expanded macros. In this case I
think the change is fine due to my reasoning above.
It might be appropriate to switch pirq_dpci to:
#define pirq_dpci(d, pirq) \
((is_hvm_domain(d) && (pirq)) ? &(pirq)->arch.hvm.dpci : NULL)
Or to an inline helper.
Regards, Roger.
