From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 46C36A00C5; Thu, 25 Aug 2022 14:36:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CF8784280B; Thu, 25 Aug 2022 14:36:47 +0200 (CEST) Received: from loongson.cn (mail.loongson.cn [114.242.206.163]) by mails.dpdk.org (Postfix) with ESMTP id A47CA40156 for ; Thu, 25 Aug 2022 14:36:45 +0200 (CEST) Received: from localhost.localdomain (unknown [10.20.42.60]) by localhost.localdomain (Coremail) with SMTP id AQAAf8CxYOLXbAdjfMUJAA--.44008S3; Thu, 25 Aug 2022 20:36:40 +0800 (CST) Subject: Re: [PATCH v1] vhost: fix build To: David Marchand Cc: "Xia, Chenbo" , dev , maobibo@loongson.cn, Maxime Coquelin References: <20220822074233.209414-1-zhoumin@loongson.cn> From: zhoumin Message-ID: Date: Thu, 25 Aug 2022 20:36:39 +0800 User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID: AQAAf8CxYOLXbAdjfMUJAA--.44008S3 X-Coremail-Antispam: 1UD129KBjvJXoW3Gr1rCr47Kw18JFWDCrWxZwb_yoWxuFW5pF 4fWw17AryUJr4Sga18W3W8A34Fyw18G347Jr4aqF40k3yUtrnFvr10grWj9r1UJrWFy3WF qF1jy3WDZF1UZFJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvjb7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Gr0_Xr1l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Cr0_Gr1UM28EF7xvwVC2z280aVAFwI0_Cr1j6rxdM28EF7xvwV C2z280aVCY1x0267AKxVW0oVCq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC 0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr 1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxk0xIA0c2IEe2xFo4CEbIxvr21l c2xSY4AK6svPMxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I 8CrVAFwI0_Jr0_Jr4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVWUAVWU twCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x 0267AKxVWUJVW8JwCI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAIcVC2z280aVAFwI0_ Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVWUJVW8JbIYCTnIWIevJa73UjIFyTuYvjxUgg _TUUUUU X-CM-SenderInfo: 52kr3ztlq6z05rqj20fqof0/ X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi David, Thanks for your kind reply. On Wed, Aug 24, 2022 at 22:09, David Marchand wrote: > On Mon, Aug 22, 2022 at 9:42 AM Min Zhou wrote: >> This patch fixes the following build failure seen on CentOS 8 >> with gcc 12.1 because of uninitialized struct variable: >> >> [..] >> ../lib/vhost/virtio_net.c:1159:18: warning: 'buf_vec[0].buf_addr' may be used uninitialized [-Wmaybe-uninitialized] >> 1159 | buf_addr = buf_vec[vec_idx].buf_addr; >> | ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~ >> [..] > I don't like setting the whole variable to 0 just to silence a > warning, like pushing something under the rug. > This is all the more suspicious as there is other code in this file > that does almost the same. I'm sorry. I did see the same code at eight places in this file. They are all similar to the following:     struct buf_vector buf_vec[BUF_VECTOR_MAX];     [...]     fill_vec_buf_{split, packed}(.., buf_vec, ..)     [...]     mbuf_to_desc(.., buf_vec, ..) or desc_to_mbuf(.., buf_vec, ..) At these eight places, the array variable `buf_vec` is not explicitly initialized. Actually, all these `buf_vec` arraies are initialized by sub-function calls under various conditions. When I saw the GCC warning at line 1838, I decided to initialize the `buf_vec` array by calling memset() at all eight places. But when I just did that for line 1838, the GCC warning was disappeared. I'm not familiar with vhost, so this made me puzzled and I tried to minimize the amount of code modification. I wondered if the uninitialized case at line 1838 is such evident to GCC that it can be easily found and the other seven uninitialized cases are hidden by more complicated sub-function call routines which made GCC more cautious to warn. I am not sure either, but that's what I am guessing. Now I think I'm wrong. I sorted out the sub-function calls related to the `buf_vec` array for all these eight places as follows. **Assignment** means assigning values to member of the `buf_vec` array. **Using** means using values from the `buf_vec` array. 1. virtio_dev_rx_split // line 1330    |__ reserve_avail_buf_split    |    |__ fill_vec_buf_split    |        |__ map_one_desc // assign values to buf_vec[vec_id]    |__ mbuf_to_desc // use values from buf_vec[vec_id] 2. virtio_dev_rx_single_packed // line 1498    |__ vhost_enqueue_single_packed         |__ fill_vec_buf_packed         |    |__ fill_vec_buf_packed_indirect         |    |    |__ map_one_desc // assignment         |    |__ map_one_desc // assignment         |__ mbuf_to_desc // using 3. virtio_dev_rx_async_submit_split // line 1670    |__ reserve_avail_buf_split    |    |__ fill_vec_buf_split    |        |__ map_one_desc // assignment    |__ mbuf_to_desc // using 4. virtio_dev_rx_async_packed // line 1834    |__ vhost_enqueue_async_pakced         |__ fill_vec_buf_packed         |    |__ fill_vec_buf_packed_indirect         |    |    |__ map_one_desc // assignment         |    |__ map_one_desc // assignment         |__ mbuf_to_desc // **using, but gcc emits warning** 5. virtio_dev_tx_split // line 2878    |__ fill_vec_buf_split    |    |__ map_one_desc // assignment    |__ desc_to_mbuf // using 6. vhost_dequeue_single_packed // line 3105    |__ fill_vec_buf_packed    |    |__ fill_vec_buf_packed_indirect    |    |    |__ map_one_desc // assignment    |    |__ map_one_desc // assignment    |__ desc_to_mbuf // using 7. virtio_dev_tx_async_split // line 3397    |__ fill_vec_buf_split    |    |__ map_one_desc // assignment    |__ desc_to_mbuf // using 8. virtio_dev_tx_async_single_packed // line 3571    |__ fill_vec_buf_packed    |    |__ fill_vec_buf_packed_indirect    |    |    |__ map_one_desc // assignment    |    |__ map_one_desc // assignment    |__ desc_to_mbuf // using However, I cannot guess the warning rules of GCC from the above calling relationships. These eight groups of calling relationships seem to follow the same pattern and are not significantly different. It's difficult for me to find the real reasons behind the GCC warning. > > I had seen a similar warning during 22.07 when cross compiling but did > not investigate much. > The patch that I had written at the time was: > > diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c > index 35fa4670fd..9446e33aa7 100644 > --- a/lib/vhost/virtio_net.c > +++ b/lib/vhost/virtio_net.c > @@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct > vhost_virtqueue *vq, > struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL; > struct vhost_async *async = vq->async; > > - if (unlikely(m == NULL)) > + if (unlikely(m == NULL || nr_vec == 0)) > return -1; > > buf_addr = buf_vec[vec_idx].buf_addr; > > > Could you see if this fixes your issue? > > If it is the case, it may be worth better understanding what bothers > the compiler in the current code. > > I have verified that the solution you proposed here is effective. It can eliminate the GCC warning. But I don't know what this change means for the compiler. From the programmer's point of view, we can know the binding relationship between the `nr_vec` variable and the `buf_vec` array. we can use "nr_vec == 0" to determine the validity of the `buf_vec[0]`. But, I'm not sure if the compiler knows about it. I cannot explain from the GCC's point of view why this modification can eliminate the warning. However, in terms of correctness, this modification is very reasonable because the `buf_vec[0]` would be invalid if the `nr_vec` variable equals to zero. In this case, the function should return. In addition, we can see that the `buf_vec` array are also used in function desc_to_mbuf() from the above calling relationships. Do you think it is necessary to add a similar check to this function? like this: diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c index 9446e33aa7..99233f1759 100644 --- a/lib/vhost/virtio_net.c +++ b/lib/vhost/virtio_net.c @@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,         struct vhost_async *async = vq->async;         struct async_inflight_info *pkts_info; +       if (unlikely(nr_vec == 0)) +               return -1; +         buf_addr = buf_vec[vec_idx].buf_addr;         buf_iova = buf_vec[vec_idx].buf_iova;         buf_len = buf_vec[vec_idx].buf_len; I expect this compilation warning can be resolved. Because LoongArch cross-compile tools must be based on GCC 12.1 and this compilation warning will cause LoongArch CI job to fail. Thinks, Min Zhou `