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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> 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.

As per my earlier reply on the thread, I view the check as a guard
for the subsequent call to hvm_pirq_eoi(), where _this_ pointer
(and not pirq) is assumed to be non-NULL (while pirq gets explicitly
checked).

> 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.

I have yet to understand in which way you deem the current logic to not
be correct. I'm sorry for being dense.

> 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.

Well, thanks for the pointer in any event. I've commented there as well.

Jan