[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.