Message
Andre Muezerie
Jan. 14, 2025, 2:31 a.m. UTC
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.
v16:
* remove -Wvla from drivers/common/mlx5/meson.build and
drivers/common/qat/meson.build
v15:
* inverted some of the logic added during v14:
add -Wvla to meson build files in app and lib directories, adding
-Wno-vla to the few subdirectories which are not yet VLA free
v14:
* add -Wvla to meson build for directories that are VLA free
under app, lib, drivers. This is to ensure that new VLAs are
not added to these directories in the future.
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 (42):
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
app/pdump: disable warning about use of VLAs
app/proc-info: disable warning about use of VLAs
app/test: disable warning about use of VLAs
app/test-acl: disable warning about use of VLAs
app/test-bbdev: disable warning about use of VLAs
app/test-crypto-perf: disable warning about use of VLAs
app/test-dma-perf: disable warning about use of VLAs
app/test-eventdev: disable warning about use of VLAs
app/flow-perf: add compile warning about use of VLAs
app/test-pmd: check compiler supports flag when adding to set
app/test-sad: disable warning about use of VLAs
drivers/baseband: add compile warning about use of VLAs
drivers/bus: add compile warning about use of VLAs
drivers/common: add compile warning about use of VLAs
drivers/compress: add compile warning about use of VLAs
drivers/gpu: add compile warning about use of VLAs
drivers/mempool: add compile warning about use of VLAs
drivers/ml: add compile warning about use of VLAs
drivers/power: add compile warning about use of VLAs
drivers/raw: add compile warning about use of VLAs
drivers/vdpa: add compile warning about use of VLAs
drivers/regex: add compile warning about use of VLAs
acl: disable warning about use of VLAs
bpf: disable warning about use of VLAs
dispatcher: disable warning about use of VLAs
eventdev: disable warning about use of VLAs
ipsec: disable warning about use of VLAs
member: disable warning about use of VLAs
metrics: disable warning about use of VLAs
pdcp: disable warning about use of VLAs
pdump: disable warning about use of VLAs
pipeline: disable warning about use of VLAs
power: disable warning about use of VLAs
table: disable warning about use of VLAs
vhost: disable warning about use of VLAs
drivers/common: add compile warning about use of VLAs
drivers/net: add compile warning about use of VLAs
app: add compile warning about use of VLAs
lib: add compile warning about use of VLAs
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
app/meson.build | 9 +
app/pdump/meson.build | 8 +
app/proc-info/meson.build | 8 +
app/test-acl/meson.build | 8 +
app/test-bbdev/meson.build | 8 +
app/test-crypto-perf/meson.build | 8 +
app/test-dma-perf/meson.build | 8 +
app/test-eventdev/meson.build | 8 +
app/test-flow-perf/meson.build | 8 +
app/test-pmd/cmdline.c | 2 +-
app/test-pmd/cmdline_flow.c | 15 +-
app/test-pmd/config.c | 16 +-
app/test-pmd/meson.build | 10 +-
app/test-pmd/shared_rxq_fwd.c | 2 +-
app/test-sad/meson.build | 8 +
app/test/meson.build | 16 +-
app/test/test.c | 2 +-
app/test/test_bitset.c | 69 ++++---
app/test/test_cmdline_string.c | 2 +-
app/test/test_cryptodev.c | 34 ++--
app/test/test_cryptodev_blockcipher.c | 4 +-
app/test/test_cryptodev_crosscheck.c | 2 +-
app/test/test_dmadev.c | 9 +-
app/test/test_hash.c | 14 +-
app/test/test_lcore_var_perf.c | 2 +-
app/test/test_mempool.c | 25 +--
app/test/test_reassembly_perf.c | 22 ++-
app/test/test_reorder.c | 48 ++---
app/test/test_service_cores.c | 9 +-
app/test/test_thash.c | 7 +-
config/meson.build | 4 +
drivers/baseband/meson.build | 8 +
drivers/bus/meson.build | 8 +
drivers/common/idpf/idpf_common_rxtx.c | 2 +-
drivers/common/idpf/idpf_common_rxtx_avx512.c | 6 +-
drivers/common/meson.build | 8 +
drivers/common/mlx5/mlx5_common.h | 4 +-
drivers/common/mlx5/mlx5_devx_cmds.c | 7 +-
drivers/common/nfp/meson.build | 8 +
drivers/common/nitrox/meson.build | 8 +
drivers/common/sfc_efx/meson.build | 1 +
drivers/compress/meson.build | 8 +
drivers/gpu/meson.build | 8 +
drivers/mempool/meson.build | 8 +
drivers/ml/meson.build | 8 +
drivers/net/i40e/i40e_testpmd.c | 5 +-
drivers/net/ice/ice_rxtx.c | 18 +-
drivers/net/ice/ice_rxtx.h | 2 +
drivers/net/ixgbe/ixgbe_ethdev.c | 21 +-
drivers/net/ixgbe/ixgbe_rxtx_vec_common.h | 4 +-
drivers/net/meson.build | 8 +
drivers/net/mlx5/mlx5.c | 5 +-
drivers/net/mlx5/mlx5_flow.c | 6 +-
drivers/power/meson.build | 8 +
drivers/raw/meson.build | 8 +
drivers/regex/meson.build | 8 +
drivers/vdpa/meson.build | 8 +
lib/acl/meson.build | 8 +
lib/bpf/meson.build | 8 +
lib/dispatcher/meson.build | 8 +
lib/eal/common/eal_common_proc.c | 5 +-
lib/eal/freebsd/include/rte_os.h | 1 +
lib/eal/linux/eal_interrupts.c | 32 ++-
lib/eal/linux/include/rte_os.h | 1 +
lib/eal/windows/include/rte_os.h | 1 +
lib/ethdev/rte_ethdev.c | 183 +++++++++++-------
lib/eventdev/meson.build | 8 +
lib/gro/rte_gro.c | 42 ++--
lib/hash/rte_cuckoo_hash.c | 4 +-
lib/hash/rte_thash.c | 2 +-
lib/hash/rte_thash.h | 7 +
lib/hash/rte_thash_gf2_poly_math.c | 9 +-
lib/ipsec/meson.build | 8 +
lib/member/meson.build | 8 +
lib/meson.build | 10 +-
lib/metrics/meson.build | 8 +
lib/pdcp/meson.build | 8 +
lib/pdump/meson.build | 8 +
lib/pipeline/meson.build | 8 +
lib/power/meson.build | 9 +
lib/rcu/rte_rcu_qsbr.c | 7 +-
lib/rcu/rte_rcu_qsbr.h | 5 +
lib/table/meson.build | 8 +
lib/vhost/meson.build | 14 +-
84 files changed, 723 insertions(+), 285 deletions(-)
--
Series-acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
2.47.0.vfs.0.3
Comments
On Tue, Jan 14, 2025 at 3:32 AM Andre Muezerie <andremue@linux.microsoft.com> 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. > > v16: > * remove -Wvla from drivers/common/mlx5/meson.build and > drivers/common/qat/meson.build > > v15: > * inverted some of the logic added during v14: > add -Wvla to meson build files in app and lib directories, adding > -Wno-vla to the few subdirectories which are not yet VLA free > > v14: > * add -Wvla to meson build for directories that are VLA free > under app, lib, drivers. This is to ensure that new VLAs are > not added to these directories in the future. Thanks for working on this topic. I see there is some back and forth on the topic of passing -Wvla. It would be less fragile to put a -Wla in a upper level meson.build (like config/meson.build for example), then disable explicitly in the parts that are not ready. Something like: diff --git a/config/meson.build b/config/meson.build index 6aaad6d8a4..be603bd45b 100644 --- a/config/meson.build +++ b/config/meson.build @@ -348,6 +348,17 @@ foreach arg: warning_flags endif endforeach +if cc.has_argument('-Wvla') + add_project_arguments('-Wvla', language: 'c') + if not is_windows + no_vla_cflag = '-Wno-vla' + else + no_vla_cflag = [] + endif +else + no_vla_cflag = [] +endif + # set other values pulled from the build options dpdk_conf.set('RTE_MAX_ETHPORTS', get_option('max_ethports')) dpdk_conf.set('RTE_LIBEAL_USE_HPET', get_option('use_hpet')) This has the benefit of avoiding repeating those if cc.has_argument() loops in all meson.build. Disabling becomes simply a matter of adding cflags += no_vla_cflag. This also enforces -Wvla for code that is built on windows (with mingw build). I had a try, and flagged all remaining components that have VLA in them. You can have a look at: https://github.com/david-marchand/dpdk/commit/vla_v16_dma This helped me catch a new VLA in the recently merged soring test code: https://github.com/david-marchand/dpdk/commit/vla_v16_dma~1 WDYT?
On Thu, Jan 23, 2025 at 12:58:49PM +0100, David Marchand wrote: > On Tue, Jan 14, 2025 at 3:32 AM Andre Muezerie > <andremue@linux.microsoft.com> 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. > > > > v16: > > * remove -Wvla from drivers/common/mlx5/meson.build and > > drivers/common/qat/meson.build > > > > v15: > > * inverted some of the logic added during v14: > > add -Wvla to meson build files in app and lib directories, adding > > -Wno-vla to the few subdirectories which are not yet VLA free > > > > v14: > > * add -Wvla to meson build for directories that are VLA free > > under app, lib, drivers. This is to ensure that new VLAs are > > not added to these directories in the future. > > Thanks for working on this topic. > > I see there is some back and forth on the topic of passing -Wvla. > It would be less fragile to put a -Wla in a upper level meson.build > (like config/meson.build for example), then disable explicitly in the > parts that are not ready. > > Something like: > diff --git a/config/meson.build b/config/meson.build > index 6aaad6d8a4..be603bd45b 100644 > --- a/config/meson.build > +++ b/config/meson.build > @@ -348,6 +348,17 @@ foreach arg: warning_flags > endif > endforeach > > +if cc.has_argument('-Wvla') > + add_project_arguments('-Wvla', language: 'c') > + if not is_windows > + no_vla_cflag = '-Wno-vla' > + else > + no_vla_cflag = [] > + endif > +else > + no_vla_cflag = [] > +endif > + Minor simplification suggestion, put "no_vla_cflag = []" outside the conditionals at the start, as the default value. Save having multiple copies of that assignment, and having to do "else" legs. /Bruce
On Thu, Jan 23, 2025 at 12:43:04PM +0000, Bruce Richardson wrote: > On Thu, Jan 23, 2025 at 12:58:49PM +0100, David Marchand wrote: > > On Tue, Jan 14, 2025 at 3:32 AM Andre Muezerie > > <andremue@linux.microsoft.com> 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. > > > > > > v16: > > > * remove -Wvla from drivers/common/mlx5/meson.build and > > > drivers/common/qat/meson.build > > > > > > v15: > > > * inverted some of the logic added during v14: > > > add -Wvla to meson build files in app and lib directories, adding > > > -Wno-vla to the few subdirectories which are not yet VLA free > > > > > > v14: > > > * add -Wvla to meson build for directories that are VLA free > > > under app, lib, drivers. This is to ensure that new VLAs are > > > not added to these directories in the future. > > > > Thanks for working on this topic. > > > > I see there is some back and forth on the topic of passing -Wvla. > > It would be less fragile to put a -Wla in a upper level meson.build > > (like config/meson.build for example), then disable explicitly in the > > parts that are not ready. > > > > Something like: > > diff --git a/config/meson.build b/config/meson.build > > index 6aaad6d8a4..be603bd45b 100644 > > --- a/config/meson.build > > +++ b/config/meson.build > > @@ -348,6 +348,17 @@ foreach arg: warning_flags > > endif > > endforeach > > > > +if cc.has_argument('-Wvla') > > + add_project_arguments('-Wvla', language: 'c') > > + if not is_windows > > + no_vla_cflag = '-Wno-vla' > > + else > > + no_vla_cflag = [] > > + endif > > +else > > + no_vla_cflag = [] > > +endif > > + > > Minor simplification suggestion, put "no_vla_cflag = []" outside the > conditionals at the start, as the default value. Save having multiple > copies of that assignment, and having to do "else" legs. > > /Bruce These look like great improvements. I especially like the idea of using -Wvla from the very top.
On Thu, Jan 23, 2025 at 5:38 PM Andre Muezerie <andremue@linux.microsoft.com> wrote: > > > I see there is some back and forth on the topic of passing -Wvla. > > > It would be less fragile to put a -Wla in a upper level meson.build > > > (like config/meson.build for example), then disable explicitly in the > > > parts that are not ready. > > > > > > Something like: > > > diff --git a/config/meson.build b/config/meson.build > > > index 6aaad6d8a4..be603bd45b 100644 > > > --- a/config/meson.build > > > +++ b/config/meson.build > > > @@ -348,6 +348,17 @@ foreach arg: warning_flags > > > endif > > > endforeach > > > > > > +if cc.has_argument('-Wvla') > > > + add_project_arguments('-Wvla', language: 'c') > > > + if not is_windows > > > + no_vla_cflag = '-Wno-vla' > > > + else > > > + no_vla_cflag = [] > > > + endif > > > +else > > > + no_vla_cflag = [] > > > +endif > > > + > > > > Minor simplification suggestion, put "no_vla_cflag = []" outside the > > conditionals at the start, as the default value. Save having multiple > > copies of that assignment, and having to do "else" legs. > > These look like great improvements. I especially like the idea of using -Wvla from the very top. Ok, can you work on a new revision and send it for the CI to have a try? Thanks.
On Wed, Jan 29, 2025 at 04:57:16PM +0100, David Marchand wrote: > On Thu, Jan 23, 2025 at 5:38 PM Andre Muezerie > <andremue@linux.microsoft.com> wrote: > > > > I see there is some back and forth on the topic of passing -Wvla. > > > > It would be less fragile to put a -Wla in a upper level meson.build > > > > (like config/meson.build for example), then disable explicitly in the > > > > parts that are not ready. > > > > > > > > Something like: > > > > diff --git a/config/meson.build b/config/meson.build > > > > index 6aaad6d8a4..be603bd45b 100644 > > > > --- a/config/meson.build > > > > +++ b/config/meson.build > > > > @@ -348,6 +348,17 @@ foreach arg: warning_flags > > > > endif > > > > endforeach > > > > > > > > +if cc.has_argument('-Wvla') > > > > + add_project_arguments('-Wvla', language: 'c') > > > > + if not is_windows > > > > + no_vla_cflag = '-Wno-vla' > > > > + else > > > > + no_vla_cflag = [] > > > > + endif > > > > +else > > > > + no_vla_cflag = [] > > > > +endif > > > > + > > > > > > Minor simplification suggestion, put "no_vla_cflag = []" outside the > > > conditionals at the start, as the default value. Save having multiple > > > copies of that assignment, and having to do "else" legs. > > > > These look like great improvements. I especially like the idea of using -Wvla from the very top. > > Ok, can you work on a new revision and send it for the CI to have a try? > Thanks. > > > -- > David Marchand Yes. I sent out v17 which incorporates the suggestions made. -- Andre Muezerie