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 10319A052A; Tue, 26 Jan 2021 15:04:42 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B5F5214149C; Tue, 26 Jan 2021 15:04:41 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id 44811141495 for ; Tue, 26 Jan 2021 15:04:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611669879; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hJyrud31Vo8WD+tNZ1Wtt7/tmKkGt5R7wvnGhQfUUYY=; b=eiOWdFkYJm3tVF9kJuoQ5aAdBF8cljUSBx520qKVd3+NoNhslV7uf1+oGGKadNjgmkWZRm QJBQE5V6omFM9e0AE8WPqKIjvsKv03b0J5ts0ClIHjR9/cmRvqoeK33BRxItbc4VDb9OER 9azKvB/QOhcWLGbfsRdNQhln2CEGs3E= Received: from mail-vk1-f200.google.com (mail-vk1-f200.google.com [209.85.221.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-35-Kgdj_j-UNeanlkxPgwwnnQ-1; Tue, 26 Jan 2021 09:04:37 -0500 X-MC-Unique: Kgdj_j-UNeanlkxPgwwnnQ-1 Received: by mail-vk1-f200.google.com with SMTP id j67so2054721vkh.15 for ; Tue, 26 Jan 2021 06:04:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hJyrud31Vo8WD+tNZ1Wtt7/tmKkGt5R7wvnGhQfUUYY=; b=kBahSmDx3TofVjnv1moZzLAa+JQb8iFbYMHOInYdTRwXrgegz8K5P1pHriAXtELTIF X6Qu2bv21OhX/acZ0F36+t5S6tk+r13YD+FgHWMgfbZkLzKWxKMMOtPsEMSr8bk0BdY7 0tTr1YXfis50KX7hgD4VoSPhixN0i98S1Wo21UJs65idhN3E1NHvq12Lls+EuEY7mLGH jPwwl3Z2Iq0yO4hZYgKxpmtGA6rDSpWU4ep39+4e8qRbW1AJ4PQVVOgJETdfNdHzwUpB Nef+CdmsxNTsPI+9mgsFz6AdFKXZ1qK3us235HJIPvAr1wQpECsjLg4SzbxRkzda0HOb 5QZQ== X-Gm-Message-State: AOAM531zctHkIXNlI+Q36Or479NF/I5R0adjefthWiPUnCzZmc1061II UYbeQWwgihNqsD4tZx9f7CngBi5GEcpN9PTk7CYKTve5mGnUvAKPIDV36A62oAGDvmE7IE4NxJI mT87+jy3A+ZCsmJE6U3Y= X-Received: by 2002:ab0:7087:: with SMTP id m7mr4288270ual.53.1611669876563; Tue, 26 Jan 2021 06:04:36 -0800 (PST) X-Google-Smtp-Source: ABdhPJwIPvTqTzjpuzMlaAr6prdFgyjS7KN8FotAzbuit0srhK7IIK4oXoaLpHY3AvsFg9gS1MwcrBpFk0SUjQQGnFk= X-Received: by 2002:ab0:7087:: with SMTP id m7mr4288223ual.53.1611669876156; Tue, 26 Jan 2021 06:04:36 -0800 (PST) MIME-Version: 1.0 References: <20210114110606.21142-1-bruce.richardson@intel.com> <20210125141115.573122-1-bruce.richardson@intel.com> <20210126111500.GA239@bricha3-MOBL.ger.corp.intel.com> In-Reply-To: <20210126111500.GA239@bricha3-MOBL.ger.corp.intel.com> From: David Marchand Date: Tue, 26 Jan 2021 15:04:25 +0100 Message-ID: To: Bruce Richardson Cc: dev , Thomas Monjalon Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 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? -- David Marchand