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 EE7CBA052A; Tue, 26 Jan 2021 12:15:09 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B9C72141437; Tue, 26 Jan 2021 12:15:09 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mails.dpdk.org (Postfix) with ESMTP id 5E209141436 for ; Tue, 26 Jan 2021 12:15:06 +0100 (CET) IronPort-SDR: d6OmGwT/b65m1EE4dtrUIC3HjYNTHoC/taJ13JBH0KTx3K3ucPnst+2iS6seX7WFv9E/Kc41Gu COw4maOtZsPg== X-IronPort-AV: E=McAfee;i="6000,8403,9875"; a="198672208" X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="198672208" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Jan 2021 03:15:05 -0800 IronPort-SDR: LFdxKHcdj3MOS2WpAvBuXN9ABbDQfZ5X42ewmvUVV9XVQkDbTGce0tmdA0tKrUyRNECotaqsts RDfbzeURyLDQ== X-IronPort-AV: E=Sophos;i="5.79,375,1602572400"; d="scan'208";a="361947691" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.19.198]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 26 Jan 2021 03:15:04 -0800 Date: Tue, 26 Jan 2021 11:15:00 +0000 From: Bruce Richardson To: David Marchand Cc: dev , Thomas Monjalon Message-ID: <20210126111500.GA239@bricha3-MOBL.ger.corp.intel.com> References: <20210114110606.21142-1-bruce.richardson@intel.com> <20210125141115.573122-1-bruce.richardson@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 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". 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