From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B9890A052A; Tue, 26 Jan 2021 15:39:34 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 931D7140CF9; Tue, 26 Jan 2021 15:39:34 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mails.dpdk.org (Postfix) with ESMTP id C75CD140CE6 for ; Tue, 26 Jan 2021 15:39:32 +0100 (CET) IronPort-SDR: XyHyxg/XUfG8RqTd8iIJm6H6F7K37lnnw3b22wgWnOsnr3N3Ma8wPG7kXDKxG5RAvMjpNJtA56 uStShs8aaCTw== X-IronPort-AV: E=McAfee;i="6000,8403,9875"; a="177339676" X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="177339676" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 06:39:29 -0800 IronPort-SDR: 7DrWXCJo0EeXM1qcUHGOIUOnXycHCDK6riLpj81j1HIgfGl8Q/tClWE+NQNuuYa6Bsq/wWGx3N 7/5uqszerWhw== X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="429730045" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.19.198]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 26 Jan 2021 06:39:28 -0800 Date: Tue, 26 Jan 2021 14:39:25 +0000 From: Bruce Richardson To: David Marchand Cc: dev , Thomas Monjalon , ferruh.yigit@intel.com Message-ID: <20210126143925.GC239@bricha3-MOBL.ger.corp.intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> <20210125141115.573122-1-bruce.richardson@intel.com> <20210126111500.GA239@bricha3-MOBL.ger.corp.intel.com> <20210126142402.GB239@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210126142402.GB239@bricha3-MOBL.ger.corp.intel.com> Subject: Re: [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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > 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 > > > > 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