On Fri, Feb 21, 2020 at 12:25 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
> On 21/02/20 05:32, Oliver Upton wrote:
> > On Thu, Feb 20, 2020 at 3:23 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> >>
> >> On 18/02/20 20:07, Sean Christopherson wrote:
> >>> On Tue, Feb 18, 2020 at 10:47:56AM -0800, Oliver Upton wrote:
> >>>> Particularly draconian compilers warn of a possible uninitialized use of
> >>>> the nr_pages_avail variable. Silence this warning by initializing it to
> >>>> zero.
> >>> Can you check if the warning still exists with commit 6ad1e29fe0ab ("KVM:
> >>> Clean up __kvm_gfn_to_hva_cache_init() and its callers")? I'm guessing
> >>> (hoping?) the suppression is no longer necessary.
> >>
> >> What if __gfn_to_hva_many and gfn_to_hva_many are marked __always_inline?
> >>
> >> Thanks,
> >>
> >> Paolo
> >>
> >
> > Even with this suggestion the compiler is ill-convinced :/
> >
> > in re to Sean: what do I mean by "draconian compiler"
> >
> > Well, the public answer is that both Barret and I use the same
> > compiler. Nothing particularly interesting about it, but idk what our
> > toolchain folks' stance is on divulging details.
> >
> > I'll instead use Sean's suggested fix (which reads much better) and resend.
>
> Can you instead look into fixing that compiler? After inlining, it is
> trivial to realize that the first two returns imply
> kvm_is_error_hva(ghc->hva). I'm asking this because even for GCC
> -Wuninitialized *used to be* total crap, but these days it's quite
> reliable and even basic data flow should be able to thread through the
> jumps.
Yeah, at this point I completely agree. I'll move onto a mainline
toolchain going forward. This was just a glaring change when I rebased
some work on top of the -Werror change (as it absolutely should).
>
> I'm more than willing to help the compiler with __always_inline hints,
> but an "uninitialized_var()" adds load on the code and I'm not sure it
> makes sense to do that for the sake of some proprietary, or at least
> unnamed, software.
Absolutely. I thought it sensible to send out the fix in case of other
toolchains out in the wild. But if nobody else other than us has
complained it's quite obvious where the problem lies.
Thanks!
