Two were found by the compiler when enabling extra warnings. One, the
apparent cause of PR 265749, was found by code inspection.
PR: 265749
Details
Details
- Reviewers
emaste chuck - Group Reviewers
bhyve - Commits
- rGb6ecef28bfd7: bhyve: Address uses of uninitialized variables in pci_nvme.c
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| usr.sbin/bhyve/pci_nvme.c | ||
|---|---|---|
| 2522 | The problem was that bytes was uninitialized in the subsequent call pci_nvme_stats_write_read_update(). Irritatingly, that itself is harmless, since pci_nvme_stats_write_read_update() doesn't use that parameter unless the command is successful, and in this branch it's not. | |
Comment Actions
looks sensible
| usr.sbin/bhyve/pci_nvme.c | ||
|---|---|---|
| 2273 | any idea why we were passing in hard coded 0 before? | |
| usr.sbin/bhyve/pci_nvme.c | ||
|---|---|---|
| 2273 | Oh, this snuck in with the other commit which addresses -Wunused warnings. This hunk has no functional impact, all the callers of pci_nvme_set_completion() are passing 0 for command word 0, this change just passes the callers value instead of hard-coding zero. I think it makes sense to use the caller's value since pci_nvme_set_completion() doesn't know what kind of command it's updating, and the interpretation of CDW0 depends on the command. | |
Comment Actions
Well, all of the warnings have to be fixed first, since we typically build with -Werror. I started working on it. I think we have to set -Wno-unused-parameter for now though, there are too many things to fix otherwise.
Comment Actions
All of these changes are fine, but the fact that they are necessary has me worried. Does what I've coded wander into undefined behavior territory?
| usr.sbin/bhyve/pci_nvme.c | ||
|---|---|---|
| 2273 | pci_nvme_set_completion() only gets called for IO commands (as opposed to Admin commands), and IO commands don't use CDW0. So instead of using cdw0, I hard coded this to 0 | |
Comment Actions
Using variables without initializing them is generally (though not always) undefined behaviour, yes.
Part of the longer-term solution will be to enable stricter compiler warnings in the bhyve build, but for that, existing warnings have to be addressed first. PR 265749 mentions an NVMe compliance test suite, but we don't currently run it in our regression test suite; would it be possible to do so? Finally, we really need some stubs that would let us compile and fuzz device models in userspace.
| usr.sbin/bhyve/pci_nvme.c | ||
|---|---|---|
| 2273 | Ok. I moved this fragment to D36118. Should we simply drop the cdw0 parameter from pci_nvme_set_completion(), then? Alternately it can be tagged with __unused. | |
Comment Actions
In this review, a couple of warnings about uninitialized variables I found by removing the WARNS?=2 line in the Makefile and recompiling pci_nvme.c. D36118 contains fixes for most of the other warnings in that file. Everything else I did so far is not in phabricator yet.
Comment Actions
What if change in Makefile also put to review? I vote to increasing levels of warning in all code.
Comment Actions
Unfortunately, that test suite is not public, but perhaps the FreeBSD Foundation could negotiate something with UNH to be able to run it. Typically, I'm running it prior to commit bhyve NVMe changes.
| usr.sbin/bhyve/pci_nvme.c | ||
|---|---|---|
| 2273 | Yes, I think the cdw0 parameter should go away. | |