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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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