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

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

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Current QEMU HEAD nvme.c does not compile:

  hw/block/nvme.c: In function ‘nvme_process_sq’:
  hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
         trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
         ^
  hw/block/nvme.c:3150:14: note: ‘result’ was declared here
     uint32_t result;
              ^

Explicitly initialize the result to fix it.

Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 hw/block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ce21b7..c122ac0 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3228,6 +3228,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
         result = ns->features.err_rec;
         goto out;
     case NVME_VOLATILE_WRITE_CACHE:
+        result = 0;
         for (i = 1; i <= n->num_namespaces; i++) {
             ns = nvme_ns(n, i);
             if (!ns) {
-- 
2.7.4

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

[Bin Meng]

On Wed, Feb 10, 2021 at 5:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> From: Bin Meng <bin.meng@windriver.com>
>
> Current QEMU HEAD nvme.c does not compile:
>
>   hw/block/nvme.c: In function ‘nvme_process_sq’:

Not sure why compiler reports this error happens in nvme_process_sq()?

But it should be in nvme_get_feature(). I will update the commit message in v2.

>   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>          ^
>   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
>      uint32_t result;
>               ^
>
> Explicitly initialize the result to fix it.
>
> Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>  hw/block/nvme.c | 1 +
>  1 file changed, 1 insertion(+)
>

Regards,
Bin

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

[Klaus Jensen]

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

On Feb 10 18:15, Bin Meng wrote:
> On Wed, Feb 10, 2021 at 5:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Current QEMU HEAD nvme.c does not compile:
> >
> >   hw/block/nvme.c: In function ‘nvme_process_sq’:
> 
> Not sure why compiler reports this error happens in nvme_process_sq()?
> 

Yeah that is kinda wierd. Also, this went through the full CI suite.
What compiler is this?

> But it should be in nvme_get_feature(). I will update the commit message in v2.
> 
> >   hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> >          ^
> >   hw/block/nvme.c:3150:14: note: ‘result’ was declared here
> >      uint32_t result;
> >               ^
> >
> > Explicitly initialize the result to fix it.
> >
> > Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  hw/block/nvme.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> 
> Regards,
> Bin
> 

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

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

[Bin Meng]

On Wed, Feb 10, 2021 at 6:23 PM Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Feb 10 18:15, Bin Meng wrote:
> > On Wed, Feb 10, 2021 at 5:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > Current QEMU HEAD nvme.c does not compile:
> > >
> > >   hw/block/nvme.c: In function ‘nvme_process_sq’:
> >
> > Not sure why compiler reports this error happens in nvme_process_sq()?
> >
>
> Yeah that is kinda wierd. Also, this went through the full CI suite.
> What compiler is this?
>

Yes it's quite strange.

I am using the default GCC 5.4 on a Ubuntu 16.04 host.

Regards,
Bin

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

[Klaus Jensen]

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

On Feb 10 18:24, Bin Meng wrote:
> On Wed, Feb 10, 2021 at 6:23 PM Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > On Feb 10 18:15, Bin Meng wrote:
> > > On Wed, Feb 10, 2021 at 5:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > Current QEMU HEAD nvme.c does not compile:
> > > >
> > > >   hw/block/nvme.c: In function ‘nvme_process_sq’:
> > >
> > > Not sure why compiler reports this error happens in nvme_process_sq()?
> > >
> >
> > Yeah that is kinda wierd. Also, this went through the full CI suite.
> > What compiler is this?
> >
> 
> Yes it's quite strange.
> 
> 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.

I'll queue up your patch, Thanks!

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

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

[Peter Maydell]

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,
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);".

thanks
-- PMM

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

[Klaus Jensen]

[-- 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 --]

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

[Peter Maydell]

On Wed, 10 Feb 2021 at 11:37, Klaus Jensen <its@irrelevant.dk> wrote:
>
> 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).

Whoops, yes.

> 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.

My new hypothesis is that maybe newer GCCs are more cautious
about when they produce the 'may be used uninitialized' warning,
to avoid having too many false positives.

-- PMM