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 054781BB20 for ; Thu, 10 May 2018 15:59:17 +0200 (CEST) To: Bruce Richardson , Luca Boccassi 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> From: Andy Green Message-ID: <8ef223c7-d8d1-e804-2270-1c743c10fcf5@warmcat.com> Date: Thu, 10 May 2018 21:59:12 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: <20180510133650.GB27828@bricha3-MOBL.ger.corp.intel.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 13:59:18 -0000 On 05/10/2018 09:36 PM, 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. Sorry, but your codebase just keeps coming up with new things to fix. Unfortunately I have yet to see gcc8 complain about something that was not a real problem in the code. If I were maintaining this, what I would do is look through the freebies I am getting from that awesome guy who is donating his time fixing my code, and anything that I could understand was useful, I would apply, let him rebase out the stuff that is in, reissue, until everything that is going in, is in. You don't have to apply the whole series, that is just how I am posting them because that is how they are in my tree. Likewise, if I post 2 x 20 patches, instead of 1 x 40, it makes no difference what you choose to cherrypick. The problems Luca pointed our are caused by problems in your code, not gcc8, and not me fixing more things in your code for free. I get it you want to make a release but am I the only person throwing gcc8 quality-related patches at you? Then maybe you should take a pause and absorb the quality improvements instead of releasing known-broken code (let us pretend there are not 166 coverity breakages). > My 2c. ...and I would get rid of completely pointless "quality theater" roadblocks like this git subject grep stuff. -Andy > /Bruce >