| Message ID | 20210125141115.573122-1-bruce.richardson@intel.com (mailing list archive) |
|---|---|
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 05650A052A; Mon, 25 Jan 2021 15:11:57 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 92F6D140F48; Mon, 25 Jan 2021 15:11:48 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id ADAB9140F40 for <dev@dpdk.org>; Mon, 25 Jan 2021 15:11:46 +0100 (CET) IronPort-SDR: 1JNGqsR1icUT2Qv25YitpkzicWD5JKPgIRUotQuruf7Oy/N2ORJGd72+yqxl1gHeGCrNGWJ8RP YX9mGz8ytWzA== X-IronPort-AV: E=McAfee;i="6000,8403,9874"; a="198511275" X-IronPort-AV: E=Sophos;i="5.79,373,1602572400"; d="scan'208";a="198511275" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jan 2021 06:11:24 -0800 IronPort-SDR: RBGTOMZZpHnkxcOxKcOdbXBPBioFeu64b19poytUYfCK/4Qc9tbFfXi8vThwTMPbO8PjBgvFvY AEFPTvRQuGWQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.79,373,1602572400"; d="scan'208";a="504149137" Received: from silpixa00399126.ir.intel.com ([10.237.222.4]) by orsmga004.jf.intel.com with ESMTP; 25 Jan 2021 06:11:22 -0800 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: david.marchand@redhat.com, thomas@monjalon.net, Bruce Richardson <bruce.richardson@intel.com> Date: Mon, 25 Jan 2021 14:11:11 +0000 Message-Id: <20210125141115.573122-1-bruce.richardson@intel.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20210114110606.21142-1-bruce.richardson@intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [PATCH v3 0/4] add checking of header includes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
| Series | add checking of header includes | |
Message
Bruce Richardson
Jan. 25, 2021, 2:11 p.m. UTC
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 -- 2.27.0
Comments
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.
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.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. > Sure, will look into all of these. /Bruce
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.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"), \ > ^ > This looks to be a real issue with our header file - clang does not have an "error" attribute. The closest equivalent I can see is "diagnose_if". Therefore, I'd suggest we need to change compat.h to be something like: #if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */ #define __rte_internal \ __attribute__((error("Symbol is not public ABI"), \ section(".text.internal"))) #elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */ #define __rte_internal \ __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ section(".text.internal"))) #else #define __rte_internal \ __attribute__((section(".text.internal"))) #endif Any thoughts or suggestions for better alternatives here? /Bruce
On Tue, Jan 26, 2021 at 12:15 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > 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.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"), \ > > ^ > > > > This looks to be a real issue with our header file - clang does not have an > "error" attribute. The closest equivalent I can see is "diagnose_if". Indeed, it does trigger a build error, so it works as expected ;-). On the header check itself, even if we find a way to properly tag those symbols with the macro in rte_compat.h, the next issue is that clang complains about such marked symbols without the ALLOW_INTERNAL_API build flag. FAILED: buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.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_pci.c.o -MF buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.c.o.d -o buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.c.o -c buildtools/chkincs/chkincs.p/rte_ethdev_pci.c In file included from buildtools/chkincs/chkincs.p/rte_ethdev_pci.c:1: /home/dmarchan/dpdk/lib/librte_ethdev/rte_ethdev_pci.h:86:13: error: Symbol is not public ABI eth_dev = rte_eth_dev_allocate(name); ^ ../../dpdk/lib/librte_ethdev/rte_ethdev_driver.h:1003:1: note: from 'diagnose_if' attribute on 'rte_eth_dev_allocate': __rte_internal ^~~~~~~~~~~~~~ ../../dpdk/lib/librte_eal/include/rte_compat.h:30:16: note: expanded from macro '__rte_internal' __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ ^ ~ [...] gcc seems more lenient about this. > Therefore, I'd suggest we need to change compat.h to be something like: > > #if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */ > > #define __rte_internal \ > __attribute__((error("Symbol is not public ABI"), \ > section(".text.internal"))) > > #elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */ > > #define __rte_internal \ > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > section(".text.internal"))) > > #else > > #define __rte_internal \ > __attribute__((section(".text.internal"))) > > #endif > > Any thoughts or suggestions for better alternatives here? I'd rather leave a build error on an unknown attribute than silence this check (which happens in your snippet, where it falls back to the #else part). Did you consider the deprecated() like for the experimental tag?
On Tue, Jan 26, 2021 at 03:04:25PM +0100, David Marchand wrote: > On Tue, Jan 26, 2021 at 12:15 PM Bruce Richardson > <bruce.richardson@intel.com> wrote: > > > > 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.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"), \ > > > ^ > > > > > > > This looks to be a real issue with our header file - clang does not have an > > "error" attribute. The closest equivalent I can see is "diagnose_if". > > Indeed, it does trigger a build error, so it works as expected ;-). > > > On the header check itself, even if we find a way to properly tag > those symbols with the macro in rte_compat.h, the next issue is that > clang complains about such marked symbols without the > ALLOW_INTERNAL_API build flag. > > FAILED: buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.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_pci.c.o -MF > buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.c.o.d -o > buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.c.o -c > buildtools/chkincs/chkincs.p/rte_ethdev_pci.c > In file included from buildtools/chkincs/chkincs.p/rte_ethdev_pci.c:1: > /home/dmarchan/dpdk/lib/librte_ethdev/rte_ethdev_pci.h:86:13: error: > Symbol is not public ABI > eth_dev = rte_eth_dev_allocate(name); > ^ > ../../dpdk/lib/librte_ethdev/rte_ethdev_driver.h:1003:1: note: from > 'diagnose_if' attribute on 'rte_eth_dev_allocate': > __rte_internal > ^~~~~~~~~~~~~~ > ../../dpdk/lib/librte_eal/include/rte_compat.h:30:16: note: expanded > from macro '__rte_internal' > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > ^ ~ > [...] > > > gcc seems more lenient about this. > > > > > Therefore, I'd suggest we need to change compat.h to be something like: > > > > #if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */ > > > > #define __rte_internal \ > > __attribute__((error("Symbol is not public ABI"), \ > > section(".text.internal"))) > > > > #elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */ > > > > #define __rte_internal \ > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > > section(".text.internal"))) > > > > #else > > > > #define __rte_internal \ > > __attribute__((section(".text.internal"))) > > > > #endif > > > > Any thoughts or suggestions for better alternatives here? > > I'd rather leave a build error on an unknown attribute than silence > this check (which happens in your snippet, where it falls back to the > #else part). > > Did you consider the deprecated() like for the experimental tag? > I've added the ALLOW_INTERNAL_API define to the build of these headers in v4.
On Tue, Jan 26, 2021 at 02:24:02PM +0000, Bruce Richardson wrote: > On Tue, Jan 26, 2021 at 03:04:25PM +0100, David Marchand wrote: > > On Tue, Jan 26, 2021 at 12:15 PM Bruce Richardson > > <bruce.richardson@intel.com> wrote: > > > > > > 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.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"), \ > > > > ^ > > > > > > > > > > This looks to be a real issue with our header file - clang does not have an > > > "error" attribute. The closest equivalent I can see is "diagnose_if". > > > > Indeed, it does trigger a build error, so it works as expected ;-). > > > > > > On the header check itself, even if we find a way to properly tag > > those symbols with the macro in rte_compat.h, the next issue is that > > clang complains about such marked symbols without the > > ALLOW_INTERNAL_API build flag. > > > > FAILED: buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.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_pci.c.o -MF > > buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.c.o.d -o > > buildtools/chkincs/chkincs.p/meson-generated_rte_ethdev_pci.c.o -c > > buildtools/chkincs/chkincs.p/rte_ethdev_pci.c > > In file included from buildtools/chkincs/chkincs.p/rte_ethdev_pci.c:1: > > /home/dmarchan/dpdk/lib/librte_ethdev/rte_ethdev_pci.h:86:13: error: > > Symbol is not public ABI > > eth_dev = rte_eth_dev_allocate(name); > > ^ > > ../../dpdk/lib/librte_ethdev/rte_ethdev_driver.h:1003:1: note: from > > 'diagnose_if' attribute on 'rte_eth_dev_allocate': > > __rte_internal > > ^~~~~~~~~~~~~~ > > ../../dpdk/lib/librte_eal/include/rte_compat.h:30:16: note: expanded > > from macro '__rte_internal' > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > > ^ ~ > > [...] > > > > > > gcc seems more lenient about this. > > > > > > > > > Therefore, I'd suggest we need to change compat.h to be something like: > > > > > > #if !defined ALLOW_INTERNAL_API && __has_attribute(error) /* For GCC */ > > > > > > #define __rte_internal \ > > > __attribute__((error("Symbol is not public ABI"), \ > > > section(".text.internal"))) > > > > > > #elif !defined ALLOW_INTERNAL_API && __has_attribute(diagnose_if) /* For clang */ > > > > > > #define __rte_internal \ > > > __attribute__((diagnose_if(1, "Symbol is not public ABI", "error"), \ > > > section(".text.internal"))) > > > > > > #else > > > > > > #define __rte_internal \ > > > __attribute__((section(".text.internal"))) > > > > > > #endif > > > > > > Any thoughts or suggestions for better alternatives here? > > > > I'd rather leave a build error on an unknown attribute than silence > > this check (which happens in your snippet, where it falls back to the > > #else part). > > > > Did you consider the deprecated() like for the experimental tag? > > > > I've added the ALLOW_INTERNAL_API define to the build of these headers in > v4. +Ferruh, Thomas Removing the ALLOW_INTERNAL_API is probably a good idea, but it does indeed throw up the errors with clang - but not gcc, which is strange. The offending headers seem to be (initially): * rte_ethdev_vdev.h * rte_ethdev_pci.h Are these public header files, or should they skip header checking - and installation - as internal-only? /Bruce
26/01/2021 15:39, Bruce Richardson: > Removing the ALLOW_INTERNAL_API is probably a good idea, but it does indeed > throw up the errors with clang - but not gcc, which is strange. The > offending headers seem to be (initially): > > * rte_ethdev_vdev.h > * rte_ethdev_pci.h > > Are these public header files, or should they skip header checking - and > installation - as internal-only? They are helpers for the drivers, so should be internal.
On Tue, Jan 26, 2021 at 04:31:36PM +0100, Thomas Monjalon wrote: > 26/01/2021 15:39, Bruce Richardson: > > Removing the ALLOW_INTERNAL_API is probably a good idea, but it does indeed > > throw up the errors with clang - but not gcc, which is strange. The > > offending headers seem to be (initially): > > > > * rte_ethdev_vdev.h > > * rte_ethdev_pci.h > > > > Are these public header files, or should they skip header checking - and > > installation - as internal-only? > > They are helpers for the drivers, so should be internal. > Just to confirm, by that you mean that they should not be installed for end-application use? In which case, I believe that "rte_ethdev_driver", "rte_ethdev_vdev" and "rte_ethdev_pci" should be removed from the "headers" list in "librte_ethdev/meson.build". Current file list there is: headers = files('rte_ethdev.h', 'rte_ethdev_driver.h', 'rte_ethdev_pci.h', 'rte_ethdev_trace.h', 'rte_ethdev_trace_fp.h', 'rte_ethdev_vdev.h', 'rte_dev_info.h', 'rte_flow.h', 'rte_flow_driver.h', 'rte_mtr.h', 'rte_mtr_driver.h', 'rte_tm.h', 'rte_tm_driver.h') headers_no_chkincs += files('rte_eth_ctrl.h', 'rte_ethdev_core.h') [Note that the "headers_no_chkincs" files still get installed, they just skip header checking as indirect includes.] Anything else that should be removed. Also, tangential to this, it would probably be good if we could come up with a different naming scheme for internal-only headers, to make the difference between what is to be installed or not, a lot clearer! /Bruce
26/01/2021 16:42, Bruce Richardson: > On Tue, Jan 26, 2021 at 04:31:36PM +0100, Thomas Monjalon wrote: > > 26/01/2021 15:39, Bruce Richardson: > > > Removing the ALLOW_INTERNAL_API is probably a good idea, but it does indeed > > > throw up the errors with clang - but not gcc, which is strange. The > > > offending headers seem to be (initially): > > > > > > * rte_ethdev_vdev.h > > > * rte_ethdev_pci.h > > > > > > Are these public header files, or should they skip header checking - and > > > installation - as internal-only? > > > > They are helpers for the drivers, so should be internal. > > > Just to confirm, by that you mean that they should not be installed for > end-application use? I think yes they are not needed to be installed. > In which case, I believe that "rte_ethdev_driver", "rte_ethdev_vdev" and > "rte_ethdev_pci" should be removed from the "headers" list in > "librte_ethdev/meson.build". Current file list there is: > > headers = files('rte_ethdev.h', > 'rte_ethdev_driver.h', > 'rte_ethdev_pci.h', > 'rte_ethdev_trace.h', > 'rte_ethdev_trace_fp.h', > 'rte_ethdev_vdev.h', > 'rte_dev_info.h', > 'rte_flow.h', > 'rte_flow_driver.h', > 'rte_mtr.h', > 'rte_mtr_driver.h', > 'rte_tm.h', > 'rte_tm_driver.h') > headers_no_chkincs += files('rte_eth_ctrl.h', > 'rte_ethdev_core.h') > > [Note that the "headers_no_chkincs" files still get installed, they just > skip header checking as indirect includes.] > Anything else that should be removed. > > Also, tangential to this, it would probably be good if we could come up > with a different naming scheme for internal-only headers, to make the > difference between what is to be installed or not, a lot clearer! The difference is supposed to be the rte_ prefix in the file name. Doxygen will parse only these files: FILE_PATTERNS = rte_*.h cmdline.h I agree we should fix the internal filenames and not install them.