Re: [dpdk-dev] [PATCH v3 0/4] add checking of header includes

[David Marchand <david.marchand@redhat.com>]

On Mon, Jan 25, 2021 at 3:11 PM Bruce Richardson
<bruce.richardson@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.


-- 
David Marchand