Re: [PATCH][4.15] gnttab: work around "may be used uninitialized" warning

[Julien Grall <julien@xen.org>]

Hi Jan,

On 10/03/2021 16:21, Jan Beulich wrote:
> On 10.03.2021 15:58, Julien Grall wrote:
>> On 10/03/2021 10:13, Jan Beulich wrote:
>>> Sadly I was wrong to suggest dropping vaddrs' initializer during review
>>> of v2 of the patch introducing this code. gcc 4.3 can't cope.
>>
>> What's the error?
> 
> The one quoted in the title.
> 
>> Are you sure this is not going to hiding a potential
>> miscompilation of the function?
> 
> Miscompilation? It may hide us screwing up, but addressing such a
> compiler warning by adding an initializer has been quite common
> in the past.

Well... When a compiler tells me a value may be unitialized, I read it 
as some optimization may decide to use the variable in a way I wasn't 
expected.

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -4026,7 +4026,7 @@ int gnttab_acquire_resource(
>>>        struct grant_table *gt = d->grant_table;
>>>        unsigned int i, final_frame;
>>>        mfn_t tmp;
>>> -    void **vaddrs;
>>> +    void **vaddrs = NULL;
>> I am a bit nervous to inialize vaddrs to NULL for a few reasons:
>>     1) It is not 100% obvious (e.g. it takes more than a second) that
>> vaddrs will always be initialized.
> 
> But convincing ourselves was necessary even more so prior to this
> change. We must not ever rely on the compiler to tell us about
> issues in our code. We can only leverage that in some cases it
> does.

I didn't suggest that we should only rely on the compiler. I pointed out 
that we are telling the compiler to not worry. This may hide a valid 
concern from the compiler.

> From this it (I think obviously) follows that without the
> initializer we're at bigger risk than with it.

I thought deferencing a NULL pointer was still a concern for PV?

For the other setup, I agree that it would only lead to a crash rather 
than dereferencing anything. Yet I am not convinced this is that much 
better...

>>     2) A compiler will not be able to help us if we are adding code
>> without initialized vaddrs.
>>
>> It also feels wrong to me to try to write Xen in a way that will make a
>> 10 years compiler happy...
> 
> As said above - we've worked around limitations quite a few times
> in the past. This is just one more instance.

I find amusing you wrote that when you complained multiple time when 
someone was re-using existing bad pattern. :)

> 
>> If we still want to support them, then maybe a better approach would be
>> to turn off to turn off -Werror for some version of GCC. So we can
>> continue to benefit from the newer compiler diagnostics.
> 
> Avoiding use of -Werror is not an option imo: Once you start seeing
> warnings, you have only two options imo: Either one decides to ignore
> them all (and then one will also ignore new ones introduce by changes
> yet to be submitted), or one would have to memorize, for every build
> one does, which warnings one ought to ignore. The latter doesn't
> scale, while the former is a code quality problem. >
> Suppressing a particular class of warning might be an option, but
> again risks somebody submitting code which elsewhere would trigger
> warnings.

This is pretty much what we are already doing slowly by initializing 
values to shut up older compilers. I agree this is more limited, but we 
also waive off diagnostics from every single compiler in that code 
rather than just one version.

Hence why I suggested dropping -Werror for older compiler. This is not 
ideal but it would give us the ability to keep support for dinausor 
compiler and not hamper our ability to take advantage of newer compiler 
diagnostics.

The ideal solution is to drop support for older compiler (see my other 
thread). But this sounds like a daunting task so far on x86...

Anyway, I will not Nack the patch but will also not Ack it. I will let 
another maintainer ack this patch.

Cheers,

-- 
Julien Grall