Re: [PATCH v13 00/21] remove use of VLAs for Windows

[Andre Muezerie <andremue@linux.microsoft.com>]

On Fri, Jan 10, 2025 at 09:27:01AM +0000, Konstantin Ananyev wrote:
> 
> 
> > On Wed, Jan 08, 2025 at 01:30:13PM +0000, Konstantin Ananyev wrote:
> > >
> > >
> > > > As per guidance technical board meeting 2024/04/17. This series
> > > > removes the use of VLAs from code built for Windows for all 3
> > > > toolchains. If there are additional opportunities to convert VLAs
> > > > to regular C arrays please provide the details for incorporation
> > > > into the series.
> > > >
> > > > MSVC does not support VLAs, replace VLAs with standard C arrays
> > > > or alloca(). alloca() is available for all toolchain/platform
> > > > combinations officially supported by DPDK.
> > > >
> > > > v13:
> > > >   * increase stack allocated buffer size in
> > > >     ipv4_reassembly_interleaved_flows_perf and
> > > >     ipv6_reassembly_interleaved_flows_perf to avoid
> > > >     STATUS_STACK_BUFFER_OVERRUN on Windows using MSVC
> > > >
> > > > v12:
> > > >   * update commit message for patch 06/21 to avoid warning
> > > >
> > > > v11:
> > > >   * add include stdlib.h for alloca() declaration on FreeBSD
> > > >   * zero-initialize array without code loop
> > > >   * increase maximum tuple length
> > > >
> > > > v10:
> > > >  * add ifdef to scope gcc's diagnostic error down to gcc only
> > > >
> > > > v9:
> > > >  * fix sender's email address
> > > >  * fix gcc's diagnostic error string to make clang happy
> > > >
> > > > v8:
> > > >  * rebase
> > > >  * reduce scope for disabling error "-Warray-bounds=" to only
> > > >    the place that needs it
> > > >  * remove parentesis around numbers from defines in test_bitset.c
> > > >
> > > > v7:
> > > >  * remove use of VLA from new file which sneaked in during review
> > > >
> > > > v6:
> > > >  * remove use of VLA from new test code added recently
> > > >  * fix title for patch 08/20
> > > >
> > > > v5:
> > > >  * add patches for net/ice, net/ixgbe and gro
> > > >    from Konstantin Ananyev from
> > > > https://patchwork.dpdk.org/project/dpdk/list/?series=31972&archive=both&state=*
> > > >  * address debug_autotest ASan failure
> > > >  * address array-bound error in bitset_autotest with gcc-13
> > > >
> > > > v4:
> > > >  * rebase and adapt for changes made in main since v3 was sent
> > > >  * use fixed maximum array size instead of VLA when doable
> > > >
> > > > v3:
> > > >  * address checkpatch/check git log warnings (minor typos)
> > > >
> > > > v2:
> > > >  * replace patches for ethdev, hash, rcu and include new
> > > >    patches for eal from Konstantin Ananyev
> > > >    from https://patchwork.dpdk.org/project/dpdk/list/?series=31781
> > > >
> > > > Andre Muezerie (3):
> > > >   test: remove use of VLAs for Windows built code in bitset tests
> > > >   app/testpmd: remove use of VLAs for Windows built code in
> > > >     shared_rxq_fwd
> > > >   hash: remove use of VLAs by using standard arrays
> > > >
> > > > Konstantin Ananyev (10):
> > > >   eal/linux: remove use of VLAs
> > > >   eal/common: remove use of VLAs
> > > >   ethdev: remove use of VLAs for Windows built code
> > > >   hash: remove use of VLAs for Windows built code
> > > >   hash/thash: remove use of VLAs for Windows built
> > > >   rcu: remove use of VLAs for Windows built code
> > > >   gro: fix overwrite unprocessed packets
> > > >   gro: remove use of VLAs
> > > >   net/ixgbe: remove use of VLAs
> > > >   net/ice: remove use of VLAs
> > > >
> > > > Tyler Retzlaff (8):
> > > >   eal: include header required for alloca
> > > >   app/testpmd: remove use of VLAs for Windows built
> > > >   test: remove use of VLAs for Windows built code
> > > >   common/idpf: remove use of VLAs for Windows built code
> > > >   net/i40e: remove use of VLAs for Windows built code
> > > >   common/mlx5: remove use of VLAs for Windows built code
> > > >   net/mlx5: remove use of VLAs for Windows built code
> > > >   build: enable vla warnings on Windows built code
> > > >
> > > > --
> > >
> > > Series-Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> > >
> > > One extra thing I would like to suggest: can we for each lib/driver/app/etc.
> > > we made VLA free add an '-Wvla' option into corresponding meson.build?
> > > To prevent people re-introducing VLA code.
> > >
> > > > 2.47.0.vfs.0.3
> > >
> > 
> > Yes, I'll do so. It will add many small patches to the patchset though. I
> > hope that is fine.
> 
> That's ok with me, hope tree maintainers will not complain too. 
>  Another possible option - combine it with particular lib changes,
> i.e. patch that fixing VLA usage in lib/hash can also enable -Wvla for it,
> and so on.
> 

The previous patchset added -Wvla to the code being built on Windows.

Your suggestion for adding -Wvla on Linux builds goes one step further
and would give an earlier warning to people introducing VLAs in the code,
which is a good thing. It also makes it easier to enable additional code
in the Windows build in the future, as we will not be hitting so many
VLAs anymore.

Patchset v14 containing your suggestions was sent out. Let me know if you
have further comments about it.