Re: [PATCH for-4.16] x86/passthrough: Fix hvm_gsi_eoi() build with GCC 12

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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, and
its use in this case does not do what the source code comment says
either.  A GSI is mapped when a pirq object exists, not a dpci object.


If your answer is "well actually, we didn't mean to say 'if a GSI is
mapped' in the comment, and here's a different predicate which actually
inspects the state of a dpci object for validity", then fine -  that
will shut the compiler up because you're no longer checking for the
NULLness of a pointer to a sub-object of a non-NULL pointer, but that's
a bugfix which needs backporting several releases too.

The current logic is not correct, and does not become correct by trying
pass blame to the compiler.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102967 is the GCC bug, but
the result of it was them persuading me that the diagnostic was
legitimate, even if currently expressed badly.  They've agreed to fix
how it is expressed, but I doubt you'll persuade them that the trigger
for the diagnostic in the first place was wrong.

~Andrew