[v3] vhost: fix build issues with GCC 12

Message ID 20221005203524.89336-1-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series [v3] vhost: fix build issues with GCC 12 |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/github-robot: build success github build: passed
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS

Commit Message

Maxime Coquelin Oct. 5, 2022, 8:35 p.m. UTC
This patch fixes a compilation issue met with GCC 12 on
LoongArch64:

In function ‘mbuf_to_desc’,
    inlined from ‘vhost_enqueue_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1822:6,
    inlined from ‘virtio_dev_rx_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1836:6,
    inlined from ‘virtio_dev_rx_async_submit_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1895:7:
../../../dpdk/lib/vhost/virtio_net.c:1159:18: error: ‘buf_vec[0].buf_addr’ may be used uninitialized [-Werror=maybe-uninitialized]
 1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
      |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../dpdk/lib/vhost/virtio_net.c: In function ‘virtio_dev_rx_async_submit_packed’:
../../../dpdk/lib/vhost/virtio_net.c:1834:27: note: ‘buf_vec’ declared here
 1834 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
      |                           ^~~~~~~

It happens because the compiler assumes that 'size'
variable in vhost_enqueue_async_packed could wrap to 0 since
'size' is uint32_t and pkt->pkt_len too.

In practice, it would never happen since 'pkt->pkt_len' is
unlikely to be close to UINT32_MAX, but let's just change
'size' to uint64_t to make the compiler happy without
having to add runtime checks.

This patch also fixes similar patterns in three other
places, including one that also produces similar build
issue on ARM64 in vhost_enqueue_single_packed().

Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
- Fix similar issue in 3 other places (David)


 lib/vhost/virtio_net.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

Comments

Stephen Hemminger Oct. 6, 2022, 12:01 a.m. UTC | #1
On Wed,  5 Oct 2022 22:35:24 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> In practice, it would never happen since 'pkt->pkt_len' is
> unlikely to be close to UINT32_MAX, but let's just change
> 'size' to uint64_t to make the compiler happy without
> having to add runtime checks.


Would the standard typedef size_t work since that is what sizeof() returns.
  
Maxime Coquelin Oct. 6, 2022, 7:51 a.m. UTC | #2
Hi Stephen,

On 10/6/22 02:01, Stephen Hemminger wrote:
> On Wed,  5 Oct 2022 22:35:24 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> In practice, it would never happen since 'pkt->pkt_len' is
>> unlikely to be close to UINT32_MAX, but let's just change
>> 'size' to uint64_t to make the compiler happy without
>> having to add runtime checks.
> 
> 
> Would the standard typedef size_t work since that is what sizeof() returns.
> 

I'm not sure it would not create issues on 32 bits architectures build
given size_t is 32bits in this case if I'm not mistaken, as it adds
something to a u32, so it could wrap to 0.
  
Amit Prakash Shukla Oct. 6, 2022, 8:22 a.m. UTC | #3
Hi Maxime,

Compiled this patch with aarch64-linux-gnu-gcc (GCC) 12.2.0 and it compiles fine.

Thanks,
Amit Shukla

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 6, 2022 1:21 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org; cheng1.jiang@intel.com; chenbo.xia@intel.com;
> zhoumin@loongson.cn; david.marchand@redhat.com;
> thomas@monjalon.net; stable@dpdk.org
> Subject: [EXT] Re: [PATCH v3] vhost: fix build issues with GCC 12
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Stephen,
> 
> On 10/6/22 02:01, Stephen Hemminger wrote:
> > On Wed,  5 Oct 2022 22:35:24 +0200
> > Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> >
> >> In practice, it would never happen since 'pkt->pkt_len' is unlikely
> >> to be close to UINT32_MAX, but let's just change 'size' to uint64_t
> >> to make the compiler happy without having to add runtime checks.
> >
> >
> > Would the standard typedef size_t work since that is what sizeof() returns.
> >
> 
> I'm not sure it would not create issues on 32 bits architectures build given
> size_t is 32bits in this case if I'm not mistaken, as it adds something to a u32,
> so it could wrap to 0.
  
Maxime Coquelin Oct. 6, 2022, 9:05 a.m. UTC | #4
On 10/6/22 10:22, Amit Prakash Shukla wrote:
> Hi Maxime,
> 
> Compiled this patch with aarch64-linux-gnu-gcc (GCC) 12.2.0 and it compiles fine.

Thanks Amit!

> Thanks,
> Amit Shukla
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, October 6, 2022 1:21 PM
>> To: Stephen Hemminger <stephen@networkplumber.org>
>> Cc: dev@dpdk.org; cheng1.jiang@intel.com; chenbo.xia@intel.com;
>> zhoumin@loongson.cn; david.marchand@redhat.com;
>> thomas@monjalon.net; stable@dpdk.org
>> Subject: [EXT] Re: [PATCH v3] vhost: fix build issues with GCC 12
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Hi Stephen,
>>
>> On 10/6/22 02:01, Stephen Hemminger wrote:
>>> On Wed,  5 Oct 2022 22:35:24 +0200
>>> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>>>
>>>> In practice, it would never happen since 'pkt->pkt_len' is unlikely
>>>> to be close to UINT32_MAX, but let's just change 'size' to uint64_t
>>>> to make the compiler happy without having to add runtime checks.
>>>
>>>
>>> Would the standard typedef size_t work since that is what sizeof() returns.
>>>
>>
>> I'm not sure it would not create issues on 32 bits architectures build given
>> size_t is 32bits in this case if I'm not mistaken, as it adds something to a u32,
>> so it could wrap to 0.
>
  
Thomas Monjalon Oct. 6, 2022, 10:21 a.m. UTC | #5
05/10/2022 22:35, Maxime Coquelin:
> This patch fixes a compilation issue met with GCC 12 on
> LoongArch64:
> 
> In function ‘mbuf_to_desc’,
>     inlined from ‘vhost_enqueue_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1822:6,
>     inlined from ‘virtio_dev_rx_async_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1836:6,
>     inlined from ‘virtio_dev_rx_async_submit_packed’ at ../../../dpdk/lib/vhost/virtio_net.c:1895:7:
> ../../../dpdk/lib/vhost/virtio_net.c:1159:18: error: ‘buf_vec[0].buf_addr’ may be used uninitialized [-Werror=maybe-uninitialized]
>  1159 |         buf_addr = buf_vec[vec_idx].buf_addr;
>       |         ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../dpdk/lib/vhost/virtio_net.c: In function ‘virtio_dev_rx_async_submit_packed’:
> ../../../dpdk/lib/vhost/virtio_net.c:1834:27: note: ‘buf_vec’ declared here
>  1834 |         struct buf_vector buf_vec[BUF_VECTOR_MAX];
>       |                           ^~~~~~~
> 
> It happens because the compiler assumes that 'size'
> variable in vhost_enqueue_async_packed could wrap to 0 since
> 'size' is uint32_t and pkt->pkt_len too.
> 
> In practice, it would never happen since 'pkt->pkt_len' is
> unlikely to be close to UINT32_MAX, but let's just change
> 'size' to uint64_t to make the compiler happy without
> having to add runtime checks.
> 
> This patch also fixes similar patterns in three other
> places, including one that also produces similar build
> issue on ARM64 in vhost_enqueue_single_packed().
> 
> Fixes: 873e8dad6f49 ("vhost: support packed ring in async datapath")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>
Tested-by: Amit Prakash Shukla <amitprakashs@marvell.com>

Applied, thanks.
  

Patch

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 8f4d0f0502..6b4a062df3 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -787,7 +787,7 @@  fill_vec_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
  */
 static inline int
 reserve_avail_buf_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
-				uint32_t size, struct buf_vector *buf_vec,
+				uint64_t size, struct buf_vector *buf_vec,
 				uint16_t *num_buffers, uint16_t avail_head,
 				uint16_t *nr_vec)
 {
@@ -1277,7 +1277,7 @@  vhost_enqueue_single_packed(struct virtio_net *dev,
 	uint16_t buf_id = 0;
 	uint32_t len = 0;
 	uint16_t desc_count;
-	uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
+	uint64_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uint16_t num_buffers = 0;
 	uint32_t buffer_len[vq->size];
 	uint16_t buffer_buf_id[vq->size];
@@ -1345,7 +1345,7 @@  virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
 
 	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
-		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
+		uint64_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
 		uint16_t nr_vec = 0;
 
 		if (unlikely(reserve_avail_buf_split(dev, vq,
@@ -1689,7 +1689,7 @@  virtio_dev_rx_async_submit_split(struct virtio_net *dev, struct vhost_virtqueue
 	async_iter_reset(async);
 
 	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
-		uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
+		uint64_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen;
 		uint16_t nr_vec = 0;
 
 		if (unlikely(reserve_avail_buf_split(dev, vq, pkt_len, buf_vec,
@@ -1780,7 +1780,7 @@  vhost_enqueue_async_packed(struct virtio_net *dev,
 	uint16_t buf_id = 0;
 	uint32_t len = 0;
 	uint16_t desc_count = 0;
-	uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
+	uint64_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	uint32_t buffer_len[vq->size];
 	uint16_t buffer_buf_id[vq->size];
 	uint16_t buffer_desc_count[vq->size];