Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green <andy@warmcat.com>]

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