[dpdk-dev] [PATCH] vhost: fix false-positive warning from clang 5

[dpdk-dev] [PATCH] vhost: fix false-positive warning from clang 5

[Bruce Richardson]

When compiling with clang extra warning flags, such as used by default with
meson, a warning is given in iotlb.c:

../lib/librte_vhost/iotlb.c:318:6: warning: variable 'socket' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

This is a false positive, as the socket value will be initialized by the
call to get_mempolicy in the case where the NUMA build-time flag is set,
and in cases where it is not set, "if (ret)" will always be true as ret is
initialized to -1 and never changed.

However, this is not immediately obvious, and is perhaps a little fragile,
as it will break if other code using ret is subsequently added above the
call to get_mempolicy by someone unaware of this subtle dependency.
Therefore, we can fix the warning and making the code more robust by
explicitly initializing socket to zero, and moving the extra condition
check on ret into the #ifdef alongside the call to get_mempolicy which sets
ret.

Fixes: d012d1f293f4 ("vhost: add IOTLB helper functions")

CC: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_vhost/iotlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
index 066c37a73..6e48eaf0d 100644
--- a/lib/librte_vhost/iotlb.c
+++ b/lib/librte_vhost/iotlb.c
@@ -300,7 +300,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
 {
 	char pool_name[RTE_MEMPOOL_NAMESIZE];
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
-	int ret = -1, socket;
+	int ret = -1, socket = 0;
 
 	if (vq->iotlb_pool) {
 		/*
@@ -314,9 +314,9 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
 
 #ifdef RTE_LIBRTE_VHOST_NUMA
 	ret = get_mempolicy(&socket, NULL, 0, vq, MPOL_F_NODE | MPOL_F_ADDR);
-#endif
 	if (ret)
 		socket = 0;
+#endif
 
 	rte_rwlock_init(&vq->iotlb_lock);
 	rte_rwlock_init(&vq->iotlb_pending_lock);
-- 
2.13.6

Re: [dpdk-dev] [PATCH] vhost: fix false-positive warning from clang 5

[Maxime Coquelin]

On 10/11/2017 11:59 AM, Bruce Richardson wrote:
> When compiling with clang extra warning flags, such as used by default with
> meson, a warning is given in iotlb.c:
> 
> ../lib/librte_vhost/iotlb.c:318:6: warning: variable 'socket' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> 
> This is a false positive, as the socket value will be initialized by the
> call to get_mempolicy in the case where the NUMA build-time flag is set,
> and in cases where it is not set, "if (ret)" will always be true as ret is
> initialized to -1 and never changed.
> 
> However, this is not immediately obvious, and is perhaps a little fragile,
> as it will break if other code using ret is subsequently added above the
> call to get_mempolicy by someone unaware of this subtle dependency.
> Therefore, we can fix the warning and making the code more robust by
> explicitly initializing socket to zero, and moving the extra condition
> check on ret into the #ifdef alongside the call to get_mempolicy which sets
> ret.
> 
> Fixes: d012d1f293f4 ("vhost: add IOTLB helper functions")
> 
> CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   lib/librte_vhost/iotlb.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

I agree this is a bit fragile. Thanks for handling this:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH] vhost: fix false-positive warning from clang5

[Thomas Monjalon]

11/10/2017 12:12, Maxime Coquelin:
> On 10/11/2017 11:59 AM, Bruce Richardson wrote:
> > When compiling with clang extra warning flags, such as used by default with
> > meson, a warning is given in iotlb.c:
> > 
> > ../lib/librte_vhost/iotlb.c:318:6: warning: variable 'socket' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> > 
> > This is a false positive, as the socket value will be initialized by the
> > call to get_mempolicy in the case where the NUMA build-time flag is set,
> > and in cases where it is not set, "if (ret)" will always be true as ret is
> > initialized to -1 and never changed.
> > 
> > However, this is not immediately obvious, and is perhaps a little fragile,
> > as it will break if other code using ret is subsequently added above the
> > call to get_mempolicy by someone unaware of this subtle dependency.
> > Therefore, we can fix the warning and making the code more robust by
> > explicitly initializing socket to zero, and moving the extra condition
> > check on ret into the #ifdef alongside the call to get_mempolicy which sets
> > ret.
> > 
> > Fixes: d012d1f293f4 ("vhost: add IOTLB helper functions")
> > 
> > CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >   lib/librte_vhost/iotlb.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> 
> I agree this is a bit fragile. Thanks for handling this:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

New error with this patch:

lib/librte_vhost/iotlb.c:303:6: error:
unused variable ‘ret’ [-Werror=unused-variable]

Re: [dpdk-dev] [PATCH] vhost: fix false-positive warning from clang5

[Bruce Richardson]

On Wed, Oct 11, 2017 at 12:51:59PM +0200, Thomas Monjalon wrote:
> 11/10/2017 12:12, Maxime Coquelin:
> > On 10/11/2017 11:59 AM, Bruce Richardson wrote:
> > > When compiling with clang extra warning flags, such as used by default with
> > > meson, a warning is given in iotlb.c:
> > > 
> > > ../lib/librte_vhost/iotlb.c:318:6: warning: variable 'socket' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> > > 
> > > This is a false positive, as the socket value will be initialized by the
> > > call to get_mempolicy in the case where the NUMA build-time flag is set,
> > > and in cases where it is not set, "if (ret)" will always be true as ret is
> > > initialized to -1 and never changed.
> > > 
> > > However, this is not immediately obvious, and is perhaps a little fragile,
> > > as it will break if other code using ret is subsequently added above the
> > > call to get_mempolicy by someone unaware of this subtle dependency.
> > > Therefore, we can fix the warning and making the code more robust by
> > > explicitly initializing socket to zero, and moving the extra condition
> > > check on ret into the #ifdef alongside the call to get_mempolicy which sets
> > > ret.
> > > 
> > > Fixes: d012d1f293f4 ("vhost: add IOTLB helper functions")
> > > 
> > > CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >   lib/librte_vhost/iotlb.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > 
> > I agree this is a bit fragile. Thanks for handling this:
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> New error with this patch:
> 
> lib/librte_vhost/iotlb.c:303:6: error:
> unused variable ‘ret’ [-Werror=unused-variable]
> 
See it now, when the VHOST NUMA option - which is different from the EAL
one, I notice - is disabled. I'll do a v2 and remove the variable
completely as it doesn't seem necessary.

/Bruce

[dpdk-dev] [PATCH v2] vhost: fix false-positive warning from clang 5

[Bruce Richardson]

When compiling with clang extra warning flags, such as used by default with
meson, a warning is given in iotlb.c:

../lib/librte_vhost/iotlb.c:318:6: warning: variable 'socket' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

This is a false positive, as the socket value will be initialized by the
call to get_mempolicy in the case where the NUMA build-time flag is set,
and in cases where it is not set, "if (ret)" will always be true as ret is
initialized to -1 and never changed.

However, this is not immediately obvious, and is perhaps a little fragile,
as it will break if other code using ret is subsequently added above the
call to get_mempolicy by someone unaware of this subtle dependency.
Therefore, we can fix the warning and making the code more robust by
explicitly initializing socket to zero, and moving the extra condition
check on the return from get_mempolicy() into the #ifdef

Fixes: d012d1f293f4 ("vhost: add IOTLB helper functions")

CC: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---
V2: remove ret variable completely, to avoid unused var warnings when
compiling with the build-time numa option disabled.
---
 lib/librte_vhost/iotlb.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/librte_vhost/iotlb.c b/lib/librte_vhost/iotlb.c
index 066c37a73..05c278040 100644
--- a/lib/librte_vhost/iotlb.c
+++ b/lib/librte_vhost/iotlb.c
@@ -300,7 +300,7 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
 {
 	char pool_name[RTE_MEMPOOL_NAMESIZE];
 	struct vhost_virtqueue *vq = dev->virtqueue[vq_index];
-	int ret = -1, socket;
+	int socket = 0;
 
 	if (vq->iotlb_pool) {
 		/*
@@ -313,10 +313,9 @@ vhost_user_iotlb_init(struct virtio_net *dev, int vq_index)
 	}
 
 #ifdef RTE_LIBRTE_VHOST_NUMA
-	ret = get_mempolicy(&socket, NULL, 0, vq, MPOL_F_NODE | MPOL_F_ADDR);
-#endif
-	if (ret)
+	if (get_mempolicy(&socket, NULL, 0, vq, MPOL_F_NODE | MPOL_F_ADDR) != 0)
 		socket = 0;
+#endif
 
 	rte_rwlock_init(&vq->iotlb_lock);
 	rte_rwlock_init(&vq->iotlb_pending_lock);
-- 
2.13.6

Re: [dpdk-dev] [PATCH v2] vhost: fix false-positive warning from clang 5

[Thomas Monjalon]

11/10/2017 13:28, Bruce Richardson:
> When compiling with clang extra warning flags, such as used by default with
> meson, a warning is given in iotlb.c:
> 
> ../lib/librte_vhost/iotlb.c:318:6: warning: variable 'socket' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
> 
> This is a false positive, as the socket value will be initialized by the
> call to get_mempolicy in the case where the NUMA build-time flag is set,
> and in cases where it is not set, "if (ret)" will always be true as ret is
> initialized to -1 and never changed.
> 
> However, this is not immediately obvious, and is perhaps a little fragile,
> as it will break if other code using ret is subsequently added above the
> call to get_mempolicy by someone unaware of this subtle dependency.
> Therefore, we can fix the warning and making the code more robust by
> explicitly initializing socket to zero, and moving the extra condition
> check on the return from get_mempolicy() into the #ifdef
> 
> Fixes: d012d1f293f4 ("vhost: add IOTLB helper functions")
> 
> CC: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks