| Message ID | 1612950879-49023-1-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/block: nvme: Fix a build error in nvme_process_sq() | expand |
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
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 >
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
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!
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
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!
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
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) {