lint: Check for missing bitcoin-config.h includes #29408

maflcko · 2024-02-08T12:56:47Z

Missing bitcoin-config.h includes are problematic, because the build could silently pass, but produce an unintended result. For example, a slower fallback algorithm could be picked, even though bitcoin-config.h indicates that a faster feature is available and should be used.

As the build succeeds silently, this problem is not possible to detect with iwyu.

Thus, fix this by using a linter based on grepping the source code.

DrahtBot · 2024-02-08T12:56:50Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theuni, TheCharlatan, hebasto
Concept ACK	epiccurious

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29119 (refactor: Use std::span over Span by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2024-02-08T12:58:45Z

The idea to grep was taken from theuni (#26972 (comment))

theuni · 2024-02-08T17:07:33Z

Concept ACK.

The linter output matches my list, with the additions of:

src/crypto/sha256_arm_shani.cpp
src/crypto/sha256_avx2.cpp
src/crypto/sha256_sse41.cpp
src/crypto/sha256_x86_shani.cpp
src/interfaces/wallet.h

The crypto defines actually come from our Makefile. Those have always been wonky, maybe now is a good time to do something about those.

src/interfaces/wallet.h however is a false-positive :(

maflcko · 2024-02-08T17:57:50Z

src/interfaces/wallet.h however is a false-positive :(

Yeah, not sure how to solve this. Maybe rewrite everything as a clang-tidy plugin?

test/lint/test_runner/src/main.rs

epiccurious · 2024-02-09T03:41:54Z

Concept ACK f4db354.

theuni · 2024-02-15T20:18:48Z

What's the status on this? Just waiting on a change for the crypto defines?

It'd be nice to get it in with/soonafter #29404.

maflcko · 2024-02-16T08:09:57Z

Ok, this (The signed, previous to last commit, fa9c71a) is ready for review.

I'll drop the last commit (unsigned merge commit) once and if it is merged.

hebasto

Tested 4856291 on Ubuntu 23.10.

Missing #include <config/bitcoin-config.h> are reported correctly.

What are we supposed to do to automatically avoid redundant / unneeded includes?

maflcko · 2024-02-16T12:48:39Z

What are we supposed to do to automatically avoid redundant / unneeded includes?

Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?

hebasto · 2024-02-16T14:14:40Z

What are we supposed to do to automatically avoid redundant / unneeded includes?

Iwyu is a tool to detect redundant and unneeded includes, so it can be used. However, there may be false warnings in regard to this specific header. Thus, an iwyu pragma can be used to avoid incorrect removal of needed includes. Should I push a commit here?

I assume that both macros HAVE_TIMINGSAFE_BCMP and HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR are commented out in config/bitcoin-config.h in the CI environment.

I'm pretty sure that the same happens to the CHAR_EQUALS_INT8 macro as well. However, IWYU does not suggest to drop #include <config/bitcoin-config.h> from the serialize.h. That seems fragile as IWYU behavior might change in the future..

I suggest to add // IWYU pragma: keep everywhere and put this rule to the Developer Notes.

hebasto

https://api.cirrus-ci.com/v1/task/5330278671974400/logs/ci.log:

crypto/sha256.cpp should remove these lines:
- #include <config/bitcoin-config.h>  // lines 6-6

More // IWYU pragma: keep?

theuni · 2024-02-16T17:35:29Z

It's not clear to me what the benefit of using IWYU is here. The missing defines will (by definition) vary per-platform, so I'm not sure what we're chasing here exactly.

Or is it more-so just to keep IWYU usable without warnings for our c-i configs specifically?

maflcko · 2024-02-17T11:05:38Z

Yeah, good point. So I guess the only way to remove unused includes would be to write another check in the linter. I'll drop the last commit and leave everything else for a follow-up.

DrahtBot · 2024-02-17T12:47:53Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21683771354}

hebasto · 2024-02-19T14:59:06Z

Or is it more-so just to keep IWYU usable without warnings for our c-i configs specifically?

Yes. At some point in the future we might want to treat IWYU warnings as errors.

This is needed for a future change.

This will print a backtrace when an internal code error happened.

maflcko · 2024-02-20T14:02:54Z

Added the check to error on redundant (harmless) includes as well, since it was trivial. Should be trivial to re-ACK.

maflcko · 2024-02-20T14:05:38Z

Rebased, for silent merge conflict in src/bench/wallet_ismine.cpp. I guess this immediately shows that the check is useful :)

…_ismine.cpp It was included indirectly via src/wallet/test/util.h, however it is better to include what you use.

theuni

Weak ACK fa58ae7.

My python-fu is weak, but I think this is a necessary check and it seems to work as advertised.

TheCharlatan

ACK fa58ae7

maflcko · 2024-02-22T11:25:57Z

Added milestone, since this test can catch bugs, and it was reviewed.

hebasto

ACK fa58ae7, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.

…tcoin-config.h includes fa09451 Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke) dddd40b scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke) Pull request description: The `bitcoin-config.h` includes have issues: * The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in #29408 (comment) * Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in #29404 (comment) . ACKs for top commit: achow101: ACK fa09451 TheCharlatan: ACK fa09451 hebasto: re-ACK fa09451, only rebased since my recent [review](#29494 (review)) (`timedata.cpp` removed in #29623). Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52

DrahtBot changed the title ~~lint: Check for missing bitcoin-config.h includes~~ lint: Check for missing bitcoin-config.h includes Feb 8, 2024

DrahtBot added the Tests label Feb 8, 2024

maflcko marked this pull request as draft February 8, 2024 12:57

maflcko mentioned this pull request Feb 8, 2024

doc: List identifiers used from header in #if(def) #26972

Closed

DrahtBot added the CI failed label Feb 8, 2024

maflcko force-pushed the 2402-lint-missing-includes- branch 2 times, most recently from 8c2a68d to f4db354 Compare February 8, 2024 15:22

TheCharlatan reviewed Feb 8, 2024

View reviewed changes

test/lint/test_runner/src/main.rs Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 8, 2024

refactor: Use std::span over Span #29119

Closed

maflcko force-pushed the 2402-lint-missing-includes- branch 2 times, most recently from 51ad354 to ae00c91 Compare February 12, 2024 10:05

maflcko force-pushed the 2402-lint-missing-includes- branch from f45f2bf to 4856291 Compare February 16, 2024 08:07

DrahtBot removed the CI failed label Feb 16, 2024

hebasto reviewed Feb 16, 2024

View reviewed changes

DrahtBot mentioned this pull request Feb 16, 2024

serialization: c++20 endian/byteswap/clz modernization #29263

Merged

maflcko force-pushed the 2402-lint-missing-includes- branch from 9eca5ed to fa9c71a Compare February 17, 2024 11:49

DrahtBot added the CI failed label Feb 17, 2024

maflcko force-pushed the 2402-lint-missing-includes- branch 2 times, most recently from fa55397 to fa97cf6 Compare February 19, 2024 16:35

maflcko mentioned this pull request Feb 20, 2024

refactor: bitcoin-config.h includes cleanup #29404

Merged

maflcko marked this pull request as ready for review February 20, 2024 13:52

MarcoFalke added 2 commits February 20, 2024 15:02

lint: Add get_subtrees() helper

fa10051

This is needed for a future change.

doc: Add missing RUST_BACKTRACE=1

fa770fd

This will print a backtrace when an internal code error happened.

MarcoFalke added 2 commits February 20, 2024 15:03

lint: Make lint error easier to spot in output

fa63b0e

lint: Check for missing or redundant bitcoin-config.h includes

fa31908

maflcko force-pushed the 2402-lint-missing-includes- branch from fa97cf6 to fa31908 Compare February 20, 2024 14:03

refactor: Add missing include for USE_BDB, USE_SQLITE to bench/wallet…

fa58ae7

…_ismine.cpp It was included indirectly via src/wallet/test/util.h, however it is better to include what you use.

maflcko requested a review from hebasto February 20, 2024 16:25

DrahtBot removed the CI failed label Feb 20, 2024

theuni approved these changes Feb 20, 2024

View reviewed changes

TheCharlatan approved these changes Feb 22, 2024

View reviewed changes

maflcko added this to the 27.0 milestone Feb 22, 2024

hebasto approved these changes Feb 22, 2024

View reviewed changes

fanquake merged commit eaede27 into bitcoin:master Feb 26, 2024

maflcko deleted the 2402-lint-missing-includes- branch February 26, 2024 11:34

maflcko mentioned this pull request Feb 27, 2024

build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes #29494

Merged

bitcoin locked and limited conversation to collaborators Feb 25, 2025

lint: Check for missing bitcoin-config.h includes #29408

lint: Check for missing bitcoin-config.h includes #29408

Uh oh!

Conversation

maflcko commented Feb 8, 2024

Uh oh!

DrahtBot commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko commented Feb 8, 2024

Uh oh!

theuni commented Feb 8, 2024

Uh oh!

maflcko commented Feb 8, 2024

Uh oh!

Uh oh!

epiccurious commented Feb 9, 2024

Uh oh!

theuni commented Feb 15, 2024

Uh oh!

maflcko commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 16, 2024

Uh oh!

hebasto commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Feb 16, 2024

Uh oh!

maflcko commented Feb 17, 2024

Uh oh!

DrahtBot commented Feb 17, 2024

Uh oh!

hebasto commented Feb 19, 2024

Uh oh!

maflcko commented Feb 20, 2024

Uh oh!

maflcko commented Feb 20, 2024

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 22, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Feb 8, 2024 •

edited

Loading

maflcko commented Feb 16, 2024 •

edited

Loading

hebasto commented Feb 16, 2024 •

edited

Loading

hebasto left a comment •

edited

Loading