Message ID | 20210115111052.16437-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 D7152A0A02; Fri, 15 Jan 2021 12:11:05 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C0328140F3C; Fri, 15 Jan 2021 12:11:05 +0100 (CET) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 6C0BC140F35 for <dev@dpdk.org>; Fri, 15 Jan 2021 12:11:03 +0100 (CET) IronPort-SDR: //N+QfaQlMQImhJm3Y/redDZleO2RDrNUObjYZoMvfPab+lA77JS+K9E5YheEmRuE9W9iKdN1/ +HNqsaSnzkBA== X-IronPort-AV: E=McAfee;i="6000,8403,9864"; a="242602718" X-IronPort-AV: E=Sophos;i="5.79,349,1602572400"; d="scan'208";a="242602718" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jan 2021 03:11:01 -0800 IronPort-SDR: 1LMZbRsq/MMB41olKkNH693sWr8bddJkXYWLopIFunuJpGIJv5973Z4B3d7wLlgHDPEOp0rJvS QtI/AoVadsGQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.79,349,1602572400"; d="scan'208";a="401252608" Received: from silpixa00399126.ir.intel.com ([10.237.222.4]) by fmsmga002.fm.intel.com with ESMTP; 15 Jan 2021 03:11:00 -0800 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: david.marchand@redhat.com, Bruce Richardson <bruce.richardson@intel.com> Date: Fri, 15 Jan 2021 11:10:32 +0000 Message-Id: <20210115111052.16437-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 v2 00/19] ensure headers have correct 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 |
ensure headers have correct includes
|
|
Message
Bruce Richardson
Jan. 15, 2021, 11:10 a.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. This patchset fixes any missing includes in public headers from the DPDK "lib" folder and then adds a "chkincs" app. to verify this on an ongoing basis. This chkincs app does nothing when run, 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, for example. 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 (19): eal: fix missing header inclusion telemetry: fix missing header include ethdev: fix missing header include net: fix missing header include mbuf: fix missing header include bitratestats: fix missing header include rib: fix missing header includes vhost: fix missing header includes ipsec: fix missing header include fib: fix missing header includes table: fix missing header include pipeline: fix missing header includes metrics: fix variable declaration in header node: fix missing header include app: fix extra include paths for app builds app/chkincs: add chkincs app to verify headers eal: add missing include to mcslock eal/x86: add architecture-specific headers to chkincs test-meson-builds: add includes check to default x86 build MAINTAINERS | 4 +++ app/chkincs/gen_c_file_for_header.py | 12 +++++++++ app/chkincs/main.c | 4 +++ app/chkincs/meson.build | 28 ++++++++++++++++++++ app/meson.build | 2 ++ devtools/test-meson-builds.sh | 2 +- doc/guides/contributing/coding_style.rst | 12 +++++++++ lib/librte_bitratestats/rte_bitrate.h | 1 + lib/librte_eal/include/generic/rte_mcslock.h | 1 + lib/librte_eal/include/meson.build | 2 +- lib/librte_eal/include/rte_reciprocal.h | 1 + lib/librte_eal/include/rte_thread.h | 1 + lib/librte_eal/x86/include/meson.build | 14 ++++++---- lib/librte_ethdev/meson.build | 4 +-- lib/librte_ethdev/rte_eth_ctrl.h | 1 + lib/librte_fib/rte_fib.h | 1 + lib/librte_fib/rte_fib6.h | 1 + lib/librte_hash/meson.build | 4 +-- lib/librte_ipsec/meson.build | 3 ++- lib/librte_ipsec/rte_ipsec_sad.h | 1 + lib/librte_lpm/meson.build | 2 +- lib/librte_mbuf/rte_mbuf_core.h | 1 + lib/librte_mbuf/rte_mbuf_dyn.h | 3 +++ lib/librte_metrics/rte_metrics_telemetry.c | 2 ++ lib/librte_metrics/rte_metrics_telemetry.h | 2 -- lib/librte_net/rte_geneve.h | 1 + lib/librte_node/rte_node_ip4_api.h | 1 + lib/librte_pipeline/rte_swx_ctl.h | 1 + lib/librte_pipeline/rte_swx_pipeline.h | 1 + lib/librte_regexdev/meson.build | 2 +- lib/librte_rib/rte_rib.h | 2 ++ lib/librte_ring/meson.build | 4 ++- lib/librte_stack/meson.build | 4 ++- lib/librte_table/meson.build | 7 +++-- lib/librte_table/rte_lru_x86.h | 1 + lib/librte_telemetry/rte_telemetry.h | 1 + lib/librte_vhost/rte_vdpa.h | 2 ++ lib/librte_vhost/rte_vdpa_dev.h | 1 + lib/librte_vhost/rte_vhost_crypto.h | 7 +++++ lib/meson.build | 3 +++ meson.build | 1 + meson_options.txt | 2 ++ 42 files changed, 128 insertions(+), 22 deletions(-) create mode 100755 app/chkincs/gen_c_file_for_header.py create mode 100644 app/chkincs/main.c create mode 100644 app/chkincs/meson.build -- 2.27.0
Comments
On Fri, Jan 15, 2021 at 12: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. This patchset fixes any missing includes in > public headers from the DPDK "lib" folder and then adds a "chkincs" app. > to verify this on an ongoing basis. > > This chkincs app does nothing when run, 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, > for example. > > 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 (19): > eal: fix missing header inclusion I split this as two patches since the rte_thread.h only applies to 21.02. > telemetry: fix missing header include > ethdev: fix missing header include Patch 3 seems unnecessary, we skip rte_eth_ctrl.h validation later but I took it anyway. > net: fix missing header include > mbuf: fix missing header include Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > bitratestats: fix missing header include > rib: fix missing header includes > vhost: fix missing header includes > ipsec: fix missing header include > fib: fix missing header includes > table: fix missing header include > pipeline: fix missing header includes > metrics: fix variable declaration in header > node: fix missing header include > app: fix extra include paths for app builds Reviewed and applied patches 1-15, thanks Bruce.
On Thu, Jan 21, 2021 at 10:25:07AM +0100, David Marchand wrote: > On Fri, Jan 15, 2021 at 12: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. This patchset fixes any missing includes in > > public headers from the DPDK "lib" folder and then adds a "chkincs" app. > > to verify this on an ongoing basis. > > > > This chkincs app does nothing when run, 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, > > for example. > > > > 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 (19): > > eal: fix missing header inclusion > > I split this as two patches since the rte_thread.h only applies to 21.02. > > > > telemetry: fix missing header include > > ethdev: fix missing header include > > Patch 3 seems unnecessary, we skip rte_eth_ctrl.h validation later but > I took it anyway. > > > net: fix missing header include > > mbuf: fix missing header include > > Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > Good question. The checks I was doing were only for missing headers. Checking for superfluous headers is more complicated and not something I've looked at. I know it was previously suggested to use the "include-what-you-use" tool on DPDK, if anyone wants to attempt that. > > > bitratestats: fix missing header include > > rib: fix missing header includes > > vhost: fix missing header includes > > ipsec: fix missing header include > > fib: fix missing header includes > > table: fix missing header include > > pipeline: fix missing header includes > > metrics: fix variable declaration in header > > node: fix missing header include > > app: fix extra include paths for app builds > > Reviewed and applied patches 1-15, thanks Bruce. > Thanks for the bits of rework, David.
21/01/2021 10:33, Bruce Richardson: > On Thu, Jan 21, 2021 at 10:25:07AM +0100, David Marchand wrote: > > Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > > > Good question. The checks I was doing were only for missing headers. > Checking for superfluous headers is more complicated and not something I've > looked at. I know it was previously suggested to use the > "include-what-you-use" tool on DPDK, if anyone wants to attempt that. I would love having an integration of include-what-you-use in our build system. I don't know whether it's trivial or difficult with meson.
On Thu, Jan 21, 2021 at 10:36:18AM +0100, Thomas Monjalon wrote: > 21/01/2021 10:33, Bruce Richardson: > > On Thu, Jan 21, 2021 at 10:25:07AM +0100, David Marchand wrote: > > > Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > > > > > Good question. The checks I was doing were only for missing headers. > > Checking for superfluous headers is more complicated and not something I've > > looked at. I know it was previously suggested to use the > > "include-what-you-use" tool on DPDK, if anyone wants to attempt that. > > I would love having an integration of include-what-you-use in our build system. > I don't know whether it's trivial or difficult with meson. > +1 to that. I'm not familiar enough with the tool to comment on how hard or difficult it would be. I try it once a few years ago, and I don't think I managed to get it working successfully standalone, let alone as part of a build system. However, I'm sure IWYU has moved on quite a bit since then...
On Thu, Jan 21, 2021 at 10:36:18AM +0100, Thomas Monjalon wrote: > 21/01/2021 10:33, Bruce Richardson: > > On Thu, Jan 21, 2021 at 10:25:07AM +0100, David Marchand wrote: > > > Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > > > > > Good question. The checks I was doing were only for missing headers. > > Checking for superfluous headers is more complicated and not something I've > > looked at. I know it was previously suggested to use the > > "include-what-you-use" tool on DPDK, if anyone wants to attempt that. > > I would love having an integration of include-what-you-use in our build system. > I don't know whether it's trivial or difficult with meson. > Did some initial investigation here, and here are some basic instructions based on how I got it running on my Ubuntu 20.10 system. The positive is that "iwyu_tool" can run on DPDK build dirs without any extra work on our part. 1. Install iwyu from Ubuntu packaging. 2. Install clang-9 package: * This is not a dependency of iwyu, but probably should be * The version of clang installed *must* match that used to build iwyu [I had clang-10 installed which didn't work for this] * If you get errors about missing standard headers e.g. stddef.h, the version mismatch is likely the issue. 3. Configure a clang build of DPDK: * CC=clang meson <args> build-clang 4. "iwyu_tool" supports the build database format used by meson, so just run: "iwyu_tool -p build-clang" Be warned, this runs really slowly and produces lots of output! Regards, /Bruce
21/01/2021 16:15, Bruce Richardson: > On Thu, Jan 21, 2021 at 10:36:18AM +0100, Thomas Monjalon wrote: > > 21/01/2021 10:33, Bruce Richardson: > > > On Thu, Jan 21, 2021 at 10:25:07AM +0100, David Marchand wrote: > > > > Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > > > > > > > Good question. The checks I was doing were only for missing headers. > > > Checking for superfluous headers is more complicated and not something I've > > > looked at. I know it was previously suggested to use the > > > "include-what-you-use" tool on DPDK, if anyone wants to attempt that. > > > > I would love having an integration of include-what-you-use in our build system. > > I don't know whether it's trivial or difficult with meson. > > > > Did some initial investigation here, and here are some basic instructions > based on how I got it running on my Ubuntu 20.10 system. The positive is > that "iwyu_tool" can run on DPDK build dirs without any extra work on our > part. I had looked at it recently but didn't try thinking meson was not compatible. Thank you, it works :) > 1. Install iwyu from Ubuntu packaging. > 2. Install clang-9 package: > * This is not a dependency of iwyu, but probably should be > * The version of clang installed *must* match that used to build iwyu > [I had clang-10 installed which didn't work for this] > * If you get errors about missing standard headers e.g. stddef.h, > the version mismatch is likely the issue. > 3. Configure a clang build of DPDK: > * CC=clang meson <args> build-clang > 4. "iwyu_tool" supports the build database format used by meson, so just > run: "iwyu_tool -p build-clang" > > Be warned, this runs really slowly and produces lots of output! We need to filter only reports of useless includes and do the cleaning... The difficult part is that some includes may be useless on Linux but required on FreeBSD.
On Fri, Jan 22, 2021 at 12:20:55AM +0100, Thomas Monjalon wrote: > 21/01/2021 16:15, Bruce Richardson: > > On Thu, Jan 21, 2021 at 10:36:18AM +0100, Thomas Monjalon wrote: > > > 21/01/2021 10:33, Bruce Richardson: > > > > On Thu, Jan 21, 2021 at 10:25:07AM +0100, David Marchand wrote: > > > > > Is rte_byteorder.h inclusion in rte_mbuf.h still necessary? > > > > > > > > > Good question. The checks I was doing were only for missing headers. > > > > Checking for superfluous headers is more complicated and not something I've > > > > looked at. I know it was previously suggested to use the > > > > "include-what-you-use" tool on DPDK, if anyone wants to attempt that. > > > > > > I would love having an integration of include-what-you-use in our build system. > > > I don't know whether it's trivial or difficult with meson. > > > > > > > Did some initial investigation here, and here are some basic instructions > > based on how I got it running on my Ubuntu 20.10 system. The positive is > > that "iwyu_tool" can run on DPDK build dirs without any extra work on our > > part. > > I had looked at it recently but didn't try thinking meson was not compatible. > Thank you, it works :) > > > 1. Install iwyu from Ubuntu packaging. > > 2. Install clang-9 package: > > * This is not a dependency of iwyu, but probably should be > > * The version of clang installed *must* match that used to build iwyu > > [I had clang-10 installed which didn't work for this] > > * If you get errors about missing standard headers e.g. stddef.h, > > the version mismatch is likely the issue. > > 3. Configure a clang build of DPDK: > > * CC=clang meson <args> build-clang > > 4. "iwyu_tool" supports the build database format used by meson, so just > > run: "iwyu_tool -p build-clang" > > > > Be warned, this runs really slowly and produces lots of output! > > We need to filter only reports of useless includes > and do the cleaning... > > The difficult part is that some includes may be useless on Linux > but required on FreeBSD. > And that some "useless" includes in headers are required by the corresponding C file which then needs the include added there. :-( However, thinking a bit more after sending that email yesterday and looking at the results, I came to the same conclusion as you - we should use it to primarily remove unused headers. That will be slowed down by the need for additional fixup, but it's doable if someone has a bit of time. /Bruce