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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Stefano, Julien,

Stefano Stabellini writes:

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

There is no way to use this variables without initialization. They are
always initialized on the first iteration of the loop, when idx equals
to 0. Newer version of GCC can infer this, but look like this causes
problem for older versions.

>> 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. I'll withdraw the patch and let
> Volodmir fix it properly.
Actually, your patch is fine, taking into account Julien's comment about
xen_pgs and justification in the comments.
So, you can send fixed version or I can do this, if you don't want to.

>
> Volodmir, FYI I reproduced the issue using Ubuntu Trusty gcc
> 4.8.4-2ubuntu1~14.04.3.
Oh, I see. This is the quite old version of GCC. I'm using
aarch64-poky-linux-gcc (Linaro GCC 5.2-2015.11-2) 5.2.1 20151005
which is also old, but at least it tracks variables initialization
better.

--
Best regards,Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel