On Feb 10 11:01, Peter Maydell wrote: > On Wed, 10 Feb 2021 at 10:31, Klaus Jensen <i...@irrelevant.dk> wrote: > > On Feb 10 18:24, Bin Meng wrote: > > > I am using the default GCC 5.4 on a Ubuntu 16.04 host. > > > > > > > Alright. I'm actually not sure why newer compilers does not report this. > > The warning looks reasonable. > > It's not actually ever possible for nvme_ns() to return > NULL in this loop, because nvme_ns() will only return NULL > if it is passed an nsid value that is 0 or > n->num_namespaces,
NvmeCtrl.namespaces is an array of pointers and some of those will most
likely be NULL (those are unallocated namespaces).
> and the loop conditions mean that we never do that. So
> we can only end up using an uninitialized result if
> n->num_namespaces is zero.
>
> Newer compilers tend to do deeper analysis (eg inlining a
> function like nvme_ns() here and analysing on the basis of
> what that function does), so they can identify that
> the "if (!ns) { continue; }" codepath is never taken.
> I haven't actually done the analysis but I'm guessing that
> newer compilers also manage to figure out somehow that it's not
> possible to get here with n->num_namespaces being zero.
>
> GCC 5.4 is not quite so sophisticated, so it can't tell.
>
> There does seem to be a consistent pattern in the code of
>
> for (i = 1; i <= n->num_namespaces; i++) {
> ns = nvme_ns(n, i);
> if (!ns) {
> continue;
> }
> [stuff]
> }
>
> Might be worth considering replacing the "if (!ns) { continue; }"
> with "assert(ns);".
>
As mentioned above, ns may very well be NULL (an unallocated namespace).
I know that "it's never the compiler". But in this case, wtf? If there
are no allocated namespaces, then we will actually never hit the
statement that initializes result. I just confirmed this with a
configuration without any namespaces.
The patch is good. I wonder why newer GCCs does NOT detect this. Trying
to use `result` as the first statement in the loop also does not cause a
warning. Only using the variable just before the loop triggers a
warning on this.
I'm more than happy to be schooled by compiler people about why the
compiler might be more clever than me!
signature.asc
Description: PGP signature
