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 43CBBA052A; Tue, 26 Jan 2021 15:24:10 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1EB1F140D41; Tue, 26 Jan 2021 15:24:10 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 86C93140CE9 for ; Tue, 26 Jan 2021 15:24:08 +0100 (CET) IronPort-SDR: znWizhw4poY76qWnNuHDUb94SBFjEh6LLA5NTkJU0ZmpHp8BpfcoRL+KO5k1wfxQwO/W8UK9XY QQV5soaAGBZA== X-IronPort-AV: E=McAfee;i="6000,8403,9875"; a="180051686" X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="180051686" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 06:24:07 -0800 IronPort-SDR: Vwl9WwHVzudbKjx/iTixNEspJGgBguMSsflKkWWHqMSC4TeyQdRD4hXRSR1+FUnHBRosIA7le7 7/4B2MvvFnGQ== X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="387841532" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.19.198]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 26 Jan 2021 06:24:06 -0800 Date: Tue, 26 Jan 2021 14:24:02 +0000 From: Bruce Richardson To: David Marchand Cc: dev , Thomas Monjalon Message-ID: <20210126142402.GB239@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 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.