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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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. In fact, I seem to
recollect that we did that even without collecting all necessary acks.
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.

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