From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.warmcat.com (mail.warmcat.com [163.172.24.82]) by dpdk.org (Postfix) with ESMTP id AC9F9AABF for ; Thu, 10 May 2018 16:20:28 +0200 (CEST) From: Andy Green To: Luca Boccassi , Bruce Richardson Cc: dev@dpdk.org References: <152591991920.119328.14523975619615362920.stgit@localhost.localdomain> <1525947662.23337.101.camel@debian.org> <86b6bbc1-6ce8-cd51-7f55-6ec967cff647@warmcat.com> <1525955749.23337.103.camel@debian.org> <20180510133650.GB27828@bricha3-MOBL.ger.corp.intel.com> <1525960194.23337.105.camel@debian.org> <1d047816-9737-4f28-f2c8-df2c264c7094@warmcat.com> Message-ID: <01042bb0-faf4-f6b8-37d6-f5d75f5c0336@warmcat.com> Date: Thu, 10 May 2018 22:20:22 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: <1d047816-9737-4f28-f2c8-df2c264c7094@warmcat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 14:20:28 -0000 On 05/10/2018 09:53 PM, Andy Green wrote: > > > On 05/10/2018 09:49 PM, Luca Boccassi wrote: >> On Thu, 2018-05-10 at 14:36 +0100, Bruce Richardson wrote: >>> On Thu, May 10, 2018 at 01:35:49PM +0100, Luca Boccassi wrote: >>>> On Thu, 2018-05-10 at 20:23 +0800, Andy Green wrote: >>>>> >>>>> On 05/10/2018 06:21 PM, Luca Boccassi wrote: >>>>>> On Thu, 2018-05-10 at 10:46 +0800, Andy Green wrote: >>>>>>> The following series gets current master able to build >>>>>>> itself, and allow lagopus to build against it, on Fedora 28 + >>>>>>> x86_64 using gcc 8.0.1. >>>>>>> >>>>>>> The first 17 patches have already been through two spins and >>>>>>> this time are corrected for all the comment (thanks to >>>>>>> everybody who commented) since v2, and have tested-by / >>>>>>> acked-bys applied.  The first workaround patch for the hash >>>>>>> function cast problem is dropped since something has already >>>>>>> been applied in master since yesterday to address it. >>>>>>> >>>>>>> The additional 23 patches are fixes for problems found >>>>>>> actually trying to build lagopus using current master. >>>>>>> These are almost entirely related to signed / unsigned >>>>>>> or truncation without explicit casts inside dpdk >>>>>>> headers. >>>>>>> >>>>>>> --- >>>>>>> >>>>>>> Andy Green (40): >>>>>>>         drivers/bus/pci: fix strncpy dangerous code >>>>>>>         drivers/bus/dpaa: fix inconsistent struct alignment >>>>>>>         drivers/net/axgbe: fix broken eeprom string comp >>>>>>>         drivers/net/nfp/nfpcore: fix strncpy misuse >>>>>>>         drivers/net/nfp/nfpcore: fix off-by-one and no NUL on >>>>>>> strncpy >>>>>>> use >>>>>>>         drivers/net/nfp: don't memcpy out of source range >>>>>>>         drivers/net/nfp: fix buffer overflow in fw_name >>>>>>>         drivers/net/qede: fix strncpy constant and NUL >>>>>>>         drivers/net/qede: fix broken strncpy >>>>>>>         drivers/net/sfc: fix strncpy length >>>>>>>         drivers/net/sfc: fix strncpy size and NUL >>>>>>>         drivers/net/vdev: readlink inputs cannot be aliased >>>>>>>         drivers/net/vdev: fix 3 x strncpy misuse >>>>>>>         app/test-pmd: can't find include >>>>>>>         app/proc-info: fix sprintf overrun bug >>>>>>>         app/test-bbdev: test-bbdev: strcpy ok for allocated >>>>>>> string >>>>>>>         app/test-bbdev: strcpy ok for allocated string >>>>>>>         rte_common.h: cast gcc builtin result to avoid >>>>>>> complaints >>>>>>>         rte_memcpy.h: explicit tmp cast >>>>>>>         lib/librte_eal/common/include/rte_lcore.h: explicit >>>>>>> cast >>>>>>> for >>>>>>> signed change >>>>>>>         /lib/librte_eal/common/include/rte_random.h: stage >>>>>>> cast >>>>>>> from >>>>>>> uint64_t to long >>>>>>>         rte_spinlock.h: stack declarations before code >>>>>>>         rte_ring_generic.h: stack declarations before code >>>>>>>         rte_ring.h: remove signed type flipflopping >>>>>>>         rte_dev.h: stack declaration at top of own basic block >>>>>>>         rte_mbuf.h: avoid truncation warnings from inadvertant >>>>>>> int16_t >>>>>>> to int promotion >>>>>>>         rte_mbuf.h: explicit casts for flipping between >>>>>>> int16_t >>>>>>> and >>>>>>> uint16_t >>>>>>>         rte_mbuf.h: make sure RTE_MIN compares same types >>>>>>>         rte_mbuf.h: explicit cast restricting ptrdiff to >>>>>>> uint16_t >>>>>>>         rte_mbuf.h: explicit cast for size_t to uint32_t >>>>>>>         rte_mbuf.h: explicit casts to uint16_t to avoid >>>>>>> truncation >>>>>>> warnings >>>>>>>         rte_byteorder.h: explicit cast for return promotion >>>>>>>         rte_ether.h: explicit cast avoiding truncation warning >>>>>>>         rte_ether.h: stack vars declared at top of function >>>>>>>         rte_ethdev.h: fix sign and scope of temp var >>>>>>>         rte_ethdev.h: explicit cast for return type >>>>>>>         rte_ethdev.h: explicit cast for truncation >>>>>>>         rte_hash_crc.h: stack vars declared at top of function >>>>>>>         rte_hash_crc.h: explicit casts for truncation >>>>>>>         rte_string_fns.h: explicit cast for int return to >>>>>>> size_t >>>>>> >>>>>> Hi, >>>>>> >>>>>> I've built-tested this series on Debian Stretch (gcc 6.3) and >>>>>> Debian >>>>>> Sid (gcc 8.1). >>>>>> >>>>>> The series builds fine with the default config, but the bnx2x >>>>>> and >>>>>> mlx5 >>>>>> PMDs still have errors with gcc-8: >>>>> >>>>> Yes I just built it with defconfig for x86_64 on Fedora 28 with >>>>> default >>>>> tools and cleared out everything that came up. >>>>> >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c: In function >>>>>> 'bnx2x_alloc_hsi_mem': >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:29: error: '%s' >>>>>> directive >>>>>> writing up to 31 bytes into a region of size between 15 and 25 >>>>>> [- >>>>>> Werror=format-overflow=] >>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, >>>>>> msg, >>>>>>                                ^~ >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7: >>>>>>      if (bnx2x_dma_alloc(sc, sizeof(union >>>>>> bnx2x_host_hc_status_block), >>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> ~~~~ >>>>>> ~~ >>>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) { >>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:176:3: note: 'sprintf' >>>>>> output >>>>>> between 10 and 66 bytes into a destination of size 32 >>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, sc->pcie_device, >>>>>> msg, >>>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> ~~ >>>>>>       rte_get_timer_cycles()); >>>>>>       ~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:29: error: '%s' >>>>>> directive >>>>>> writing up to 31 bytes into a region of size between 23 and 25 >>>>>> [- >>>>>> Werror=format-overflow=] >>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), >>>>>> msg, >>>>>>                                ^~ >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:8874:7: >>>>>>      if (bnx2x_dma_alloc(sc, sizeof(union >>>>>> bnx2x_host_hc_status_block), >>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> ~~~~ >>>>>> ~~ >>>>>>          &fp->sb_dma, buf, RTE_CACHE_LINE_SIZE) != 0) { >>>>>>          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> /tmp/dpdk/drivers/net/bnx2x/bnx2x.c:173:3: note: 'sprintf' >>>>>> output >>>>>> between 10 and 58 bytes into a destination of size 32 >>>>>>      sprintf(mz_name, "bnx2x%d_%s_%" PRIx64, SC_ABS_FUNC(sc), >>>>>> msg, >>>>>>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> ~~ >>>>>>       rte_get_timer_cycles()); >>>>>> >>>>>> >>>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c: In function >>>>>> 'mlx5_pci_probe': >>>>>> /tmp/dpdk/drivers/net/mlx5/mlx5.c:920:13: error: 'vf' may be >>>>>> used >>>>>> uninitialized in this function [-Werror=maybe-uninitialized] >>>>>>      config.vf = vf; >>>>>> >>>>>> Hope this can be useful. >>>>> >>>>> I think gcc 8.0.1 is capable to show that and I am willing to >>>>> look >>>>> at >>>>> them.  But can you help me with exactly what changes you made so >>>>> these >>>>> things built and made trouble, compared to the defconfig I have >>>>> used >>>>> until now? >>>> >>>> If you already have a build directory you are using, the simplest >>>> way >>>> is to edit the .config file in there and change the following from >>>> =n >>>> to =y: >>>> >>>> CONFIG_RTE_LIBRTE_MLX4_PMD >>>> CONFIG_RTE_LIBRTE_MLX5_PMD >>>> CONFIG_RTE_LIBRTE_BNX2X_PMD >>>> >>>> Then rebuild and you should see the errors. >>>> >>> >>> Personally, though I wouldn't view it as necessary to get those extra >>> fixes >>> into this set. The set is big enough as it is, so I'd like to see the >>> existing gcc 8 fixes we have merged to make some progress, rather >>> than >>> constantly spinning ever bigger sets to try and fix them all in one >>> go. >>> >>> My 2c. >>> >>> /Bruce >> >> Yes I agree, they can be done separately, I just reported them because >> I had been asked to give the patchset a spin, and I have those PMDs >> enabled so I noticed those errors. > > If they want to release known-broken stuff, they can fix your broken > stuff separately how they like. > > But your problems are caused by a pile of real breakages (and attempts > to sticking-plaster over them with illegal casts) in this codebase. > > I dunno why you find yourself agreeing that isn't the same priority as > fixing anything else with this stuff. Fixes for your problems are here: https://github.com/lws-team/dpdk/commits/master along with the rest of my stuff. In particular: https://github.com/lws-team/dpdk/commit/6cead9e1947588ddccf0508acd01c17392cd849c https://github.com/lws-team/dpdk/commit/e0063507d824bc153ae5fd3f8fe59ccbdf88ef89 https://github.com/lws-team/dpdk/commit/5daea1b9b24e1151ac8416d7b5c1e7d096389c51 https://github.com/lws-team/dpdk/commit/3e8d82bf693e1469e4143297ea4050023d50fa6b -Andy > -Andy