Re: [Xen-devel] [PATCH] xen/arm: fix build after 2e35cdf

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.06.19 at 12:00, <julien.grall@xxxxxxx> wrote:
> Hi Stefano,
> 
> On 6/19/19 11:04 PM, Stefano Stabellini wrote:
>> On Wed, 19 Jun 2019, Julien Grall wrote:
>>> On 6/19/19 10:47 PM, Stefano Stabellini wrote:
>>>> On Wed, 19 Jun 2019, Julien Grall wrote:
>>>>> Hi Stefano,
>>>>>
>>>>> Title: You should at least mention this is for op-tee.
>>>>>
>>>>> Also, mostly likely the sha1 is too small and likely to match multiple
>>>>> commit
>>>>> in the future. So you want to specify the title of the commit.
>>>>>
>>>>> On 6/19/19 10:24 PM, Stefano Stabellini wrote:
>>>>>> Optee breaks the build with:
>>>>>>
>>>>>> optee.c: In function ‘translate_noncontig.isra.4’:
>>>>>> optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this
>>>>>> function
>>>>>> [-Werror=maybe-uninitialized]
>>>>>>                 xen_data->next_page_data = page_to_maddr(xen_pgs + 1);
>>>>>>                                          ^
>>>>>> optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this
>>>>>> function [-Werror=maybe-uninitialized]
>>>>>>             page =
>>>>>> get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
>>>>>>                                                                          
>>>>>>  ^
>>>>>> optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this
>>>>>> function
>>>>>> [-Werror=maybe-uninitialized]
>>>>>>                 put_page(guest_pg);
>>>>>>                         ^
>>>>>> cc1: all warnings being treated as errors
>>>>>>
>>>>>> Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set
>>>>>> xen_pgs to NULL for consistency.
>>>>>
>>>>> Without more explanation I think this is an unwise choice. If GCC thinks
>>>>> it is
>>>>> going to be used unitialized, then mostly likely you silent an error that
>>>>> could end up to dereference NULL.
>>>>>
>>>>> Also, setting xen_pgs for consistency will only defeat the compiler.
>>>>> Leading
>>>>> to dereferencing NULL and crash Xen...
>>>>>
>>>>> For xen_pgs, this should definitely not be NULL. For the two others, you
>>>>> need
>>>>> to explain why this is fine (if this is just because the compiler can't
>>>>> find
>>>>> the reason, then you should add a comment in the code to explain it).
>>>>
>>>> I was only trying to unblock the build.
>>>
>>> So? We don't silence a compiler warning just for unblocking the build 
> without
>>> any proper investigation. Didn't you do that before adding the NULL?
>> 
>> No I didn't. But actually, I thought we did unbreak a build as quickly
>> as possible even without a full fix in the past. 
> 
> And who is going to do the follow-up? AFAICT, you will not be the one 
> and therefore that's a call for this to stay as it is in Xen.
> 
>> In fact, I seem to
>> recollect that we did that even without collecting all necessary acks.
> 
> Collecting the necessary acks and not investigating are something 
> totally different. There are a couple of instance where patch went 
> without the necessary acks to unblock build/test (see Jan's series for 
> 4.10 and 4.11).
> 
> However Jan still investigated the problem.
> 
>> Maybe my memory is failing me? But I would have sworn it happened a
>> couple of times in the last 12 months. Or maybe this case is different
>> because it doesn't break the build with the default kconfig? In any
>> case, let's agree on a policy and I am happy to follow it.
> 
> This can't be reached with osstest (as it is protected by EXPERT), but I 
> didn't base my judgment on that.
> 
> I based my judgment on the compiler reporting a potential error and the 
> commit message not explaining why setting to NULL would be ok.
> 
> I am happy to have build fix going without any acks (to certain extend), 
> however we should not lower down the quality of the commit for that.

Since Julien asked for an explicit opinion: I fully agree with him here.
Yes, we have been rushing in build fixes, but only when there was
basically no doubt that everyone who would normally have to ack
such a change wouldn't object. This in particular implies not just
papering over issues.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel