Re: [PATCH] hw/block: nvme: Fix a build error in nvme_process_sq()

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On Feb 10 11:01, Peter Maydell wrote:
> On Wed, 10 Feb 2021 at 10:31, Klaus Jensen <its@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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]