[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm
On 27.01.2021 11:13, Oleksandr wrote: > On 26.01.21 02:14, Oleksandr wrote: >> On 26.01.21 01:20, Julien Grall wrote: >>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini >>> <sstabellini@xxxxxxxxxx> wrote: >>>> This seems to be an arm randconfig failure: >>>> >>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953 >>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044 >>> Thanks! The error is: >>> >>> #'target_mem_ref' not supported by expression#'memory.c: In function Btw, I found the first part of this line pretty confusing, to a degree that when seeing it initially I thought this must be some odd tool producing the odd error. But perhaps this is just unfortunate output ordering from different tools running in parallel. >>> 'do_memory_op': >>> memory.c:1210:18: error: may be used uninitialized in this function >>> [-Werror=maybe-uninitialized] >>> 1210 | rc = set_foreign_p2m_entry(currd, d, gfn_list[i], >>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> 1211 | _mfn(mfn_list[i])); >>> | ~~~~~~~~~~~~~~~~~~ >>> >>> I found a few references online of the error message, but it is not >>> clear what it means. From a quick look at Oleksandr's branch, I also >>> can't spot anything unitialized. Any ideas? >> It seems that error happens if *both* CONFIG_GRANT_TABLE and >> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is >> initialized either in acquire_grant_table() or in acquire_ioreq_server(). >> If these options disabled then corresponding helpers are just stubs, >> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc >> complains about it as set_foreign_p2m_entry() is *not* going to be >> called in that case??? > > This weird build error goes away if I simply add: > > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 33296e6..d1bd57b 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1136,7 +1136,7 @@ static int acquire_resource( > * moment since they are small, but if they need to grow in future > * use-cases then per-CPU arrays or heap allocations may be required. > */ > - xen_pfn_t mfn_list[32]; > + xen_pfn_t mfn_list[32] = {0}; > int rc; > > if ( !arch_acquire_resource_check(currd) ) > > > Shall I make the corresponding patch? I'd prefer if we could find another solution, avoiding this pointless writing of 256 bytes of zeros (and really to be on the safe side I think it should rather be ~0 that gets put in there). Could you check whether clearing the array along the lines of this default: memset(mfn_list, ~0, sizeof(mfn_list)); rc = -EOPNOTSUPP; break; helps (avoiding the writes in all normal cases)? Of course this wouldn't be a guarantee that another compiler (version) won't warn yet again. But the only other alternative I can think of without having the writes on the common path would be something along the lines of older Linux'es uninitialized_var(). Maybe someone else has a better idea ... > But it is still unclear to me why the compiler doesn't recognize that > *non-yet-uninitialized* mfn_list[] won't be used if both > CONFIG_GRANT_TABLE and CONFIG_IOREQ_SERVER are not set... The combination of conditions may be too complex for it to figure. I suppose a warning like this can't be had without at least one of false positives or false negatives. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |