On Mon, Jan 25, 2021 at 04:51:19PM +0100, David Marchand wrote:
> On Mon, Jan 25, 2021 at 3:11 PM Bruce Richardson
> <bruce.richard...@intel.com> wrote:
> >
> > As a general principle, each header file should include any other
> > headers it needs to provide data type definitions or macros. For
> > example, any header using the uintX_t types in structures or function
> > prototypes should include "stdint.h" to provide those type definitions.
> >
> > In practice, while many, but not all, headers in DPDK did include all
> > necessary headers, it was never actually checked that each header could
> > be included in a C file and compiled without having any compiler errors
> > about missing definitions. The script "check-includes.sh" could be used
> > for this job, but it was not called out in the documentation, so many
> > contributors may not have been aware of it's existance. It also was
> > difficult to run from a source-code directory, as the script did not
> > automatically allow finding of headers from one DPDK library directory
> > to another [this was probably based on running it on a build created by
> > the "make" build system, where all headers were in a single directory].
> > To attempt to have a build-system integrated replacement, this patchset
> > adds a "chkincs" app in the buildtools directory to verify this on an
> > ongoing basis.
> >
> > This chkincs app does nothing when run, and is not installed as part of
> > a DPDK "ninja install", it's for build-time checking only. Its source
> > code consists of one C file per public DPDK header, where that C file
> > contains nothing except an include for that header. Therefore, if any
> > header is added to the lib folder which fails to compile when included
> > alone, the build of chkincs will fail with a suitable error message.
> > Since this compile checking is not needed on most builds of DPDK, the
> > building of chkincs is disabled by default, but can be enabled by the
> > "test_includes" meson option. To catch errors with patch submissions,
> > the final patch of this series enables it for a single build in
> > test-meson-builds script.
> >
> > Future work could involve doing similar checks on headers for C++
> > compatibility, which was something done by the check-includes.sh script
> > but which is missing here..
> >
> > V3:
> > * Shrunk patchset as most header fixes already applied
> > * Moved chkincs from "apps" to the "buildtools" directory, which is a
> > better location for something not for installation for end-user use.
> > * Added patch to drop check-includes script.
> >
> > V2:
> > * Add maintainers file entry for new app
> > * Drop patch for c11 ring header
> > * Use build variable "headers_no_chkincs" for tracking exceptions
> >
> > Bruce Richardson (4):
> > eal: add missing include to mcslock
> > build: separate out headers for include checking
> > buildtools/chkincs: add app to verify header includes
> > devtools: remove check-includes script
> >
> > MAINTAINERS | 5 +-
> > buildtools/chkincs/gen_c_file_for_header.py | 12 +
> > buildtools/chkincs/main.c | 4 +
> > buildtools/chkincs/meson.build | 40 +++
> > devtools/check-includes.sh | 259 -------------------
> > devtools/test-meson-builds.sh | 2 +-
> > doc/guides/contributing/coding_style.rst | 12 +
> > lib/librte_eal/include/generic/rte_mcslock.h | 1 +
> > lib/librte_eal/include/meson.build | 2 +-
> > lib/librte_eal/x86/include/meson.build | 14 +-
> > lib/librte_ethdev/meson.build | 4 +-
> > lib/librte_hash/meson.build | 4 +-
> > lib/librte_ipsec/meson.build | 3 +-
> > lib/librte_lpm/meson.build | 2 +-
> > lib/librte_regexdev/meson.build | 2 +-
> > lib/librte_ring/meson.build | 4 +-
> > lib/librte_stack/meson.build | 4 +-
> > lib/librte_table/meson.build | 7 +-
> > lib/meson.build | 3 +
> > meson.build | 6 +
> > meson_options.txt | 2 +
> > 21 files changed, 112 insertions(+), 280 deletions(-)
> > create mode 100755 buildtools/chkincs/gen_c_file_for_header.py
> > create mode 100644 buildtools/chkincs/main.c
> > create mode 100644 buildtools/chkincs/meson.build
> > delete mode 100755 devtools/check-includes.sh
>
> - clang is not happy when enabling the check:
> $ meson configure $HOME/builds/build-clang-static -Dcheck_includes=true
> $ devtools/test-meson-builds.sh
> ...
> [362/464] Compiling C object
> buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_vdev.c.o
> FAILED: buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_vdev.c.o
> clang -Ibuildtools/chkincs/chkincs.p -Ibuildtools/chkincs
> -I../../dpdk/buildtools/chkincs -Idrivers/bus/pci
> -I../../dpdk/drivers/bus/pci -Idrivers/bus/vdev
> -I../../dpdk/drivers/bus/vdev -I. -I../../dpdk -Iconfig
> -I../../dpdk/config -Ilib/librte_eal/include
> -I../../dpdk/lib/librte_eal/include -Ilib/librte_eal/linux/include
> -I../../dpdk/lib/librte_eal/linux/include -Ilib/librte_eal/x86/include
> -I../../dpdk/lib/librte_eal/x86/include -Ilib/librte_kvargs
> -I../../dpdk/lib/librte_kvargs -Ilib/librte_metrics
> -I../../dpdk/lib/librte_metrics -Ilib/librte_telemetry
> -I../../dpdk/lib/librte_telemetry -Ilib/librte_eal/common
> -I../../dpdk/lib/librte_eal/common -Ilib/librte_eal
> -I../../dpdk/lib/librte_eal -Ilib/librte_ring
> -I../../dpdk/lib/librte_ring -Ilib/librte_rcu
> -I../../dpdk/lib/librte_rcu -Ilib/librte_mempool
> -I../../dpdk/lib/librte_mempool -Ilib/librte_mbuf
> -I../../dpdk/lib/librte_mbuf -Ilib/librte_net
> -I../../dpdk/lib/librte_net -Ilib/librte_meter
> -I../../dpdk/lib/librte_meter -Ilib/librte_ethdev
> -I../../dpdk/lib/librte_ethdev -Ilib/librte_pci
> -I../../dpdk/lib/librte_pci -Ilib/librte_cmdline
> -I../../dpdk/lib/librte_cmdline -Ilib/librte_hash
> -I../../dpdk/lib/librte_hash -Ilib/librte_timer
> -I../../dpdk/lib/librte_timer -Ilib/librte_acl
> -I../../dpdk/lib/librte_acl -Ilib/librte_bbdev
> -I../../dpdk/lib/librte_bbdev -Ilib/librte_bitratestats
> -I../../dpdk/lib/librte_bitratestats -Ilib/librte_cfgfile
> -I../../dpdk/lib/librte_cfgfile -Ilib/librte_compressdev
> -I../../dpdk/lib/librte_compressdev -Ilib/librte_cryptodev
> -I../../dpdk/lib/librte_cryptodev -Ilib/librte_distributor
> -I../../dpdk/lib/librte_distributor -Ilib/librte_efd
> -I../../dpdk/lib/librte_efd -Ilib/librte_eventdev
> -I../../dpdk/lib/librte_eventdev -Ilib/librte_gro
> -I../../dpdk/lib/librte_gro -Ilib/librte_gso
> -I../../dpdk/lib/librte_gso -Ilib/librte_ip_frag
> -I../../dpdk/lib/librte_ip_frag -Ilib/librte_jobstats
> -I../../dpdk/lib/librte_jobstats -Ilib/librte_kni
> -I../../dpdk/lib/librte_kni -Ilib/librte_latencystats
> -I../../dpdk/lib/librte_latencystats -Ilib/librte_lpm
> -I../../dpdk/lib/librte_lpm -Ilib/librte_member
> -I../../dpdk/lib/librte_member -Ilib/librte_power
> -I../../dpdk/lib/librte_power -Ilib/librte_pdump
> -I../../dpdk/lib/librte_pdump -Ilib/librte_rawdev
> -I../../dpdk/lib/librte_rawdev -Ilib/librte_regexdev
> -I../../dpdk/lib/librte_regexdev -Ilib/librte_rib
> -I../../dpdk/lib/librte_rib -Ilib/librte_reorder
> -I../../dpdk/lib/librte_reorder -Ilib/librte_sched
> -I../../dpdk/lib/librte_sched -Ilib/librte_security
> -I../../dpdk/lib/librte_security -Ilib/librte_stack
> -I../../dpdk/lib/librte_stack -Ilib/librte_vhost
> -I../../dpdk/lib/librte_vhost -Ilib/librte_ipsec
> -I../../dpdk/lib/librte_ipsec -Ilib/librte_fib
> -I../../dpdk/lib/librte_fib -Ilib/librte_port
> -I../../dpdk/lib/librte_port -Ilib/librte_table
> -I../../dpdk/lib/librte_table -Ilib/librte_pipeline
> -I../../dpdk/lib/librte_pipeline -Ilib/librte_flow_classify
> -I../../dpdk/lib/librte_flow_classify -Ilib/librte_bpf
> -I../../dpdk/lib/librte_bpf -Ilib/librte_graph
> -I../../dpdk/lib/librte_graph -Ilib/librte_node
> -I../../dpdk/lib/librte_node
> -I/home/dmarchan/intel-ipsec-mb/install/include -Xclang
> -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch
> -Werror -O2 -g -include rte_config.h -Wextra -Wcast-qual -Wdeprecated
> -Wformat -Wformat-nonliteral -Wformat-security -Wmissing-declarations
> -Wmissing-prototypes -Wnested-externs -Wold-style-definition
> -Wpointer-arith -Wsign-compare -Wstrict-prototypes -Wundef
> -Wwrite-strings -Wno-address-of-packed-member
> -Wno-missing-field-initializers -D_GNU_SOURCE -march=native
> -Wno-unused-function -DALLOW_EXPERIMENTAL_API -MD -MQ
> buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_vdev.c.o -MF
> buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_vdev.c.o.d -o
> buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_vdev.c.o -c
> buildtools/chkincs/chkincs.p/rte_ethdev_vdev.c
> In file included from buildtools/chkincs/chkincs.p/rte_ethdev_vdev.c:1:
> In file included from
> /home/dmarchan/dpdk/lib/librte_ethdev/rte_ethdev_vdev.h:12:
> ../../dpdk/lib/librte_ethdev/rte_ethdev_driver.h:964:1: error: unknown
> attribute 'error' ignored [-Werror,-Wunknown-attributes]
> __rte_internal
> ^
> ../../dpdk/lib/librte_eal/include/rte_compat.h:25:16: note: expanded
> from macro '__rte_internal'
> __attribute__((error("Symbol is not public ABI"), \
> ^
>
>
> - Other issues with ARM builds (arch-specific headers probably the reason):
> $ meson configure $HOME/builds/build-arm64-bluefield -Dcheck_includes=true
> $ devtools/test-meson-builds.sh
> ...
> In file included from buildtools/chkincs/chkincs.p/rte_rib6.c:1:
> /home/dmarchan/dpdk/lib/librte_rib/rte_rib6.h: In function ‘get_msk_part’:
> /home/dmarchan/dpdk/lib/librte_rib/rte_rib6.h:112:10: error: implicit
> declaration of function ‘RTE_MIN’; did you mean ‘INT8_MIN’?
> [-Werror=implicit-function-declaration]
> depth = RTE_MIN(depth, 128);
> ^~~~~~~
> INT8_MIN
> /home/dmarchan/dpdk/lib/librte_rib/rte_rib6.h:112:10: error: nested
> extern declaration of ‘RTE_MIN’ [-Werror=nested-externs]
> /home/dmarchan/dpdk/lib/librte_rib/rte_rib6.h:113:9: error: implicit
> declaration of function ‘RTE_MAX’; did you mean ‘INT8_MAX’?
> [-Werror=implicit-function-declaration]
> part = RTE_MAX((int16_t)depth - (byte * 8), 0);
> ^~~~~~~
> INT8_MAX
> /home/dmarchan/dpdk/lib/librte_rib/rte_rib6.h:113:9: error: nested
> extern declaration of ‘RTE_MAX’ [-Werror=nested-externs]
> cc1: all warnings being treated as errors
>
>
> - This check should be enabled for x86 and aarch cross build in GHA.
>
Sure, will look into all of these.
/Bruce
