ci, iwyu: Treat warnings as errors for specific directories #31308

hebasto commented at 9:00 PM on November 17, 2024: member

This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the crypto and index directories.

hebasto added the label Refactoring on Nov 17, 2024

hebasto added the label Tests on Nov 17, 2024

DrahtBot commented at 9:00 PM on November 17, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31308.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, maflcko, willcl-ark
Concept ACK	jonatack, l0rinc, sipa

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:

#26966 (index: initial sync speedup, parallelize process by furszy)

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.

hebasto force-pushed on Nov 18, 2024

DrahtBot added the label CI failed on Nov 18, 2024

hebasto force-pushed on Nov 25, 2024

DrahtBot added the label Needs rebase on Dec 5, 2024

hebasto force-pushed on Dec 5, 2024

DrahtBot removed the label Needs rebase on Dec 5, 2024

hebasto marked this as ready for review on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

DrahtBot added the label Needs rebase on Dec 8, 2024

hebasto force-pushed on Dec 12, 2024

hebasto commented at 10:45 AM on December 12, 2024: member

Rebased.

DrahtBot removed the label Needs rebase on Dec 12, 2024

in ci/test/03_test_script.sh:186 in d838f1bac6 outdated

 180 | @@ -180,6 +181,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
 181 |             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
 182 |             -Xiwyu --max_line_length=160 \
 183 |             2>&1 | tee /tmp/iwyu_ci.out
 184 | +
 185 | +  # TODO: Expand the following list to all targets.
 186 | +  TARGETS_WITH_FORCED_IWYU="bitcoin_clientversion bitcoin_crypto"
 187 | +  cd "${BASE_BUILD_DIR}"
 188 | +  bash -c "cmake -S ${BASE_ROOT_DIR} -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=\"include-what-you-use;-Xiwyu;--cxx17ns;-Xiwyu;--mapping_file=${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp;-Xiwyu;--error\""
 189 | +  bash -c "cmake --build . ${MAKEJOBS} --clean-first --target ${TARGETS_WITH_FORCED_IWYU}"

maflcko commented at 10:10 PM on December 19, 2024:

Could make sense to run this before the tee /tmp/iwyu_ci.out command, given that it errors? Or would it be missing (part of) the output? It would be good to see an example CI run. Maybe a -- --keep-going could be used for a slightly better output, if stuff is possibly missing?

hebasto commented at 10:28 AM on January 20, 2025:

Could make sense to run this before the tee /tmp/iwyu_ci.out command, given that it errors?

This approach allows a developer to check other includes, not only the first one that fails.

Maybe a -- --keep-going could be used for a slightly better output, if stuff is possibly missing?

I'm not sure what criteria you are using to define "better" in this case. The current branch adds one more config/build round details to the CI log and the following:

A message like the one posted in the PR description, if there is a warning.
Nothing, if there are no warnings.

maflcko commented at 11:00 AM on January 20, 2025:

I think the message you posted in the PR description only shows a single error in a single dependency. I am wondering what happens if there is more than one error in different targets, possibly ones where cmake will never get to print the second error.

I am asking, because after the cmake migration the behavior of make -k changed. Previously, everything would be attempted to be built. Now, cmake will skip and exit early in some cases.

hebasto commented at 4:37 PM on May 22, 2025:

I think the message you posted in the PR description only shows a single error in a single dependency. I am wondering what happens if there is more than one error in different targets, possibly ones where cmake will never get to print the second error.

Here multiple error are reported.

maflcko commented at 2:58 PM on May 26, 2025:

I can only see 5 reported errors, however, there should be 8 in total, no?

hebasto commented at 2:09 PM on May 27, 2025:

The change touches 8 files. The modification in src/crypto/siphash.h is a no-op from IWYU's perspective.
CMake integrates the IWYU into the compilation step. Therefore, it reports an error for each object file individually. However, the diagnostic output includes both headers and the source file:

[12:32:50.757] Warning: include-what-you-use reported diagnostics:
[12:32:50.757] 
[12:32:50.757] /ci_container_base/src/crypto/poly1305.h should add these lines:
[12:32:50.757] #include <span>      // for span
[12:32:50.757] 
[12:32:50.757] /ci_container_base/src/crypto/poly1305.h should remove these lines:
[12:32:50.757] 
[12:32:50.757] The full include-list for /ci_container_base/src/crypto/poly1305.h:
[12:32:50.757] #include <span.h>    // for UCharCast
[12:32:50.757] #include <stdint.h>  // for uint32_t
[12:32:50.757] #include <cassert>   // for assert
[12:32:50.757] #include <cstddef>   // for byte, size_t
[12:32:50.757] #include <span>      // for span
[12:32:50.757] ---
[12:32:50.757] 
[12:32:50.757] (/ci_container_base/src/crypto/poly1305.cpp has correct #includes/fwd-decls)
[12:32:50.757] 
[12:32:50.757] gmake[3]: *** [src/crypto/CMakeFiles/bitcoin_crypto.dir/build.make:188: src/crypto/CMakeFiles/bitcoin_crypto.dir/poly1305.cpp.o] Error 1

I can see all changes, except the unrelated one, in the IWYU diagnostics. Am I missing anything in particular?

maflcko commented at 12:08 PM on May 28, 2025:

Ok, fair enough. It may be fine, and should be trivial to revert, if it doesn't work as expected.

However, I worry a bit about the output not being copy-paste-able for developers to apply to their source code.

Not sure if this is a valid concern, but an alternative approach would be to just call git diff below on the TARGETS_WITH_FORCED_IWYU folders (not targets). Then exit, when the diff is not empty?

This should also address the issue of having to run twice.

The only downside I see is that the iwyu debug comments (// for UCharCast) have to be looked up by scrolling up, but this should be fine.

hebasto commented at 11:16 AM on May 29, 2025:

Could we defer such amendments to follow-ups until feedback from real-world CI usage has been collected?

hebasto commented at 5:12 PM on May 29, 2025:

Not sure if this is a valid concern, but an alternative approach would be to just call git diff below on the TARGETS_WITH_FORCED_IWYU folders (not targets). Then exit, when the diff is not empty?

Thanks! Reworked.

DrahtBot added the label Needs rebase on Jan 27, 2025

hebasto marked this as a draft on Feb 21, 2025

maflcko commented at 7:08 AM on May 21, 2025: member

concept ack, but yeah, needs rebase

hebasto force-pushed on May 22, 2025

DrahtBot removed the label Needs rebase on May 22, 2025

hebasto force-pushed on May 22, 2025

DrahtBot added the label CI failed on May 22, 2025

DrahtBot commented at 4:35 PM on May 22, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/runs/42720827740</sub> <sub>LLM reason (✨ experimental): The CI failure is due to build errors related to include-what-you-use diagnostics. </sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

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

</details>

hebasto marked this as ready for review on May 22, 2025

hebasto commented at 4:49 PM on May 22, 2025: member

Rebased.

DrahtBot removed the label CI failed on May 22, 2025

in ci/test/03_test_script.sh:185 in 0c3c7d0759 outdated

 177 | @@ -178,6 +178,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
 178 |             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
 179 |             -Xiwyu --max_line_length=160 \
 180 |             2>&1 | tee /tmp/iwyu_ci.out
 181 | +
 182 | +  # TODO: Expand the following list to all targets.
 183 | +  TARGETS_WITH_FORCED_IWYU="bitcoin_clientversion bitcoin_crypto"
 184 | +  cd "${BASE_BUILD_DIR}"
 185 | +  bash -c "cmake -S ${BASE_ROOT_DIR} -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=\"include-what-you-use;-Xiwyu;--cxx17ns;-Xiwyu;--mapping_file=${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp;-Xiwyu;--error\""

maflcko commented at 12:03 PM on May 28, 2025:

nit: no need to disable shellcheck here via bash -c?

hebasto commented at 2:14 PM on May 28, 2025:

Thanks! Done.

in ci/test/03_test_script.sh:186 in 0c3c7d0759 outdated

 177 | @@ -178,6 +178,13 @@ if [ "${RUN_TIDY}" = "true" ]; then
 178 |             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
 179 |             -Xiwyu --max_line_length=160 \
 180 |             2>&1 | tee /tmp/iwyu_ci.out
 181 | +
 182 | +  # TODO: Expand the following list to all targets.
 183 | +  TARGETS_WITH_FORCED_IWYU="bitcoin_clientversion bitcoin_crypto"
 184 | +  cd "${BASE_BUILD_DIR}"
 185 | +  bash -c "cmake -S ${BASE_ROOT_DIR} -DCMAKE_CXX_INCLUDE_WHAT_YOU_USE=\"include-what-you-use;-Xiwyu;--cxx17ns;-Xiwyu;--mapping_file=${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp;-Xiwyu;--error\""
 186 | +  bash -c "cmake --build . ${MAKEJOBS} --clean-first --target ${TARGETS_WITH_FORCED_IWYU}"

maflcko commented at 12:04 PM on May 28, 2025:

nit: Could inline the TARGETS_WITH_FORCED_IWYU to avoid the bash -c disable of shellcheck?

hebasto commented at 2:14 PM on May 28, 2025:

Thanks! Done.

maflcko commented at 12:09 PM on May 28, 2025: member

left some comments

hebasto force-pushed on May 28, 2025

fanquake commented at 11:28 AM on May 29, 2025: member

While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the iwyu_tool.py script or CMake's built-in functionality.

Reading the PR description, it's not really clear why we'd need to retain using both tools? Either the CMake built-in should be better in some way, or at least equivalent in functionality, in which case we should just switch to using it?

in contrib/devtools/iwyu/bitcoin.core.imp:10 in f70ffa4a9c outdated

   6 | +  { include: [ "<tmmintrin.h>", private, "<immintrin.h>", public ] },
   7 | +
   8 | +  # False positive warnings
   9 | +  # See: https://github.com/include-what-you-use/include-what-you-use/issues/1616
  10 | +  { include: [ "<iterator>", public, "<utility>", public ] },
  11 | +  { include: [ "<tuple>", public, "<array>", public ] },

fanquake commented at 11:29 AM on May 29, 2025:

Is there an issue open for this upstream?

fanquake commented at 4:42 PM on June 2, 2025:

?

hebasto commented at 2:01 PM on June 3, 2025:

Is there an issue open for this upstream?

Please see https://github.com/include-what-you-use/include-what-you-use/issues/1763.

hebasto commented at 2:02 PM on June 3, 2025:

The issue could be worked around in different ways. For example, by using an OS image with the default GCC >= 14.

hebasto commented at 4:56 PM on June 3, 2025:

Reworked into a local pragma.

in contrib/devtools/iwyu/bitcoin.core.imp:4 in f70ffa4a9c outdated

   0 | @@ -1,3 +1,11 @@
   1 | -# Nothing for now.
   2 |  [
   3 | +  # Compiler intrinsics
   4 | +  { include: [ "<emmintrin.h>", private, "<immintrin.h>", public ] },

fanquake commented at 11:29 AM on May 29, 2025:

Same here?

fanquake commented at 4:43 PM on June 2, 2025:

?

hebasto commented at 6:16 PM on June 2, 2025:

As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.

hebasto commented at 6:16 PM on June 2, 2025:

As they form part of Intel’s API, these headers are not covered by IWYU’s mappings for the standard libraries, and in my view, they do not fall within the scope of the IWYU repository.

fanquake commented at 6:20 PM on June 2, 2025:

they do not fall within the scope of the IWYU repository.

IWYU has mappings for intrinsics though?

hebasto commented at 3:09 PM on June 3, 2025:

Same here?

Please see https://github.com/include-what-you-use/include-what-you-use/issues/1764.

hebasto force-pushed on May 29, 2025

hebasto renamed this:
~~ci, iwyu: Treat warnings as errors for specific targets~~
ci, iwyu: Treat warnings as errors for specific directories
on May 29, 2025

hebasto commented at 5:11 PM on May 29, 2025: member

While this approach introduces some duplication at present, it will ultimately enable us to consolidate how the IWYU tool is executed, allowing for a choice between using the iwyu_tool.py script or CMake's built-in functionality.

Reading the PR description, it's not really clear why we'd need to retain using both tools? Either the CMake built-in should be better in some way, or at least equivalent in functionality, in which case we should just switch to using it?

I believe that the current approach, using a compilation database, is more suitable for this project than CMake's co-compilation. The only reason for employing the latter in this PR was to use it as a means of gradually fixing includes on a per-target basis.

I've reworked this PR to drop CMake's co-compilation approach in favour of @maflcko's idea.

in ci/test/03_test_script.sh:182 in bd9e37d2ab outdated

 177 | @@ -178,9 +178,8 @@ if [ "${RUN_TIDY}" = "true" ]; then
 178 |             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
 179 |             -Xiwyu --max_line_length=160 \
 180 |             2>&1 | tee /tmp/iwyu_ci.out
 181 | -  cd "${BASE_ROOT_DIR}/src"
 182 |    python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
 183 | -  git --no-pager diff
 184 | +  git --no-pager diff -- ':(exclude)src/leveldb/db/db_impl.cc'

maflcko commented at 2:47 PM on June 2, 2025:

nit in the first commit: Seems odd to exclude this without explanation in the code. Would an alternative be:

diff --git a/ci/test/03_test_script.sh b/ci/test/03_test_script.sh
index 3394c50b8b..db3d3d64de 100755
--- a/ci/test/03_test_script.sh
+++ b/ci/test/03_test_script.sh
@@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
 env
 echo "=== END env ==="
 
-(
+if [ "$RUN_TIDY" != "true" ]; then
+  # The tidy task has no UB detector, so skip the patch there to avoid issues
+  # later on in the task.
   # compact->outputs[i].file_size is uninitialized memory, so reading it is UB.
   # The statistic bytes_written is only used for logging, which is disabled in
   # CI, so as a temporary minimal fix to work around UB and CI failures, leave
@@ -54,7 +56,7 @@ echo "=== END env ==="
    mutex_.Lock();
    stats_[compact->compaction->level() + 1].Add(stats);
 EOF
-)
+fi
 
 if [ "$RUN_FUZZ_TESTS" = "true" ]; then
   export DIR_FUZZ_IN=${DIR_QA_ASSETS}/fuzz_corpora/

?

hebasto commented at 5:03 PM on June 2, 2025:

Thanks! Reworked.

in ci/test/03_test_script.sh:219 in bd9e37d2ab outdated

 177 | @@ -178,9 +178,8 @@ if [ "${RUN_TIDY}" = "true" ]; then
 178 |             -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="${BASE_ROOT_DIR}/contrib/devtools/iwyu/bitcoin.core.imp" \
 179 |             -Xiwyu --max_line_length=160 \
 180 |             2>&1 | tee /tmp/iwyu_ci.out
 181 | -  cd "${BASE_ROOT_DIR}/src"

maflcko commented at 2:47 PM on June 2, 2025:

q in the first commit: This is no longer needed, because cmake uses absolute paths in the compile commands?

hebasto commented at 4:08 PM on June 3, 2025:

I've tested the pre-CMake master branch @ 80f00cafdeef0600fa1a5e906686720786c2336c, and this line wasn't necessary there.

in src/index/coinstatsindex.h:11 in 5cd4bbb67c outdated

   4 | @@ -5,11 +5,20 @@
   5 |  #ifndef BITCOIN_INDEX_COINSTATSINDEX_H
   6 |  #define BITCOIN_INDEX_COINSTATSINDEX_H
   7 |  
   8 | +#include <consensus/amount.h>
   9 |  #include <crypto/muhash.h>
  10 |  #include <index/base.h>
  11 | +#include <primitives/block.h>

maflcko commented at 3:17 PM on June 2, 2025:

5cd4bbb67c3c7e651b3d83c77ffb936f60a2c496: Looks wrong. This should be a fwd dcl

hebasto commented at 5:03 PM on June 2, 2025:

Sure! Fixed.

in src/crypto/hmac_sha256.cpp:8 in eb4be21cc0 outdated

   3 | @@ -4,6 +4,7 @@
   4 |  
   5 |  #include <crypto/hmac_sha256.h>
   6 |  
   7 | +#include <crypto/sha256.h>
   8 |  #include <string.h>

maflcko commented at 3:22 PM on June 2, 2025:

nit in eb4be21cc0bb93b0931a61b2af342946f09a9721: Wrong section and should be cstring to avoid confusing with string.h in our repo?

hebasto commented at 5:04 PM on June 2, 2025:

Thanks! Fixed.

in src/crypto/hmac_sha512.cpp:8 in eb4be21cc0 outdated

   3 | @@ -4,6 +4,7 @@
   4 |  
   5 |  #include <crypto/hmac_sha512.h>
   6 |  
   7 | +#include <crypto/sha512.h>
   8 |  #include <string.h>

maflcko commented at 3:22 PM on June 2, 2025:

same...

hebasto commented at 5:04 PM on June 2, 2025:

Thanks! Fixed.

in ci/test/03_test_script.sh:193 in e9143a68f3 outdated

 194 | +             2>&1 | tee /tmp/iwyu_ci.out
 195 | +    python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
 196 | +  }
 197 | +
 198 | +  run_iwyu "compile_commands_iwyu_errors.json"
 199 | +  git diff --quiet -- ':(exclude)src/leveldb/db/db_impl.cc' || { git --no-pager diff -- ':(exclude)src/leveldb/db/db_impl.cc'; exit 1; }

maflcko commented at 3:28 PM on June 2, 2025:

e9143a68f34f350d7fa4405389222da4dfa9a394: Why not just git diff --no-pager --exit-code?

hebasto commented at 5:04 PM on June 2, 2025:

Thanks! Reworked.

maflcko commented at 3:28 PM on June 2, 2025: member

review ACK e9143a68f34f350d7fa4405389222da4dfa9a394 🎰

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e9143a68f34f350d7fa4405389222da4dfa9a394 🎰
fi/hWQQqCAdnfz0F4yhi8AMYjYOl79DanoxZsC6Am2gCztYYNf/izs+yQjc7oCt14KzsBJkAcSvQN6BVRVniAQ==

</details>

hebasto force-pushed on Jun 2, 2025

in ci/test/03_test_script.sh:37 in ba1d76d7bb outdated

  32 | @@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
  33 |  env
  34 |  echo "=== END env ==="
  35 |  
  36 | -(
  37 | +if [ "$RUN_TIDY" != "true" ]; then
  38 | +  # The tidy task has no UB detector, so skip the patch there to avoid issues

fanquake commented at 4:40 PM on June 2, 2025:

to avoid issues

What issues? Do I understand correctly, that if we land the patch in the subtree, this will just break again?

hebasto commented at 4:41 PM on June 2, 2025:

The following git diff prints this patch even IWYU did not apply any modifications.

fanquake commented at 5:14 PM on June 2, 2025:

Ok, so no an issue if we actually land the patch (and remove the related code in the CI).

maflcko commented at 6:33 AM on June 3, 2025:

Doesn't matter much, but the comment could also say "... so skip the patch because it is not needed and to avoid issues ...".

hebasto commented at 4:55 PM on June 3, 2025:

Doesn't matter much, but the comment could also say "... so skip the patch because it is not needed and to avoid issues ...".

Thanks! Adjusted.

hebasto force-pushed on Jun 2, 2025

hebasto marked this as a draft on Jun 2, 2025

hebasto marked this as ready for review on Jun 2, 2025

hebasto force-pushed on Jun 2, 2025

in ci/test/03_test_script.sh:216 in 92ac4c8353 outdated

 173 | @@ -174,14 +174,27 @@ if [ "${RUN_TIDY}" = "true" ]; then
 174 |      false
 175 |    fi
 176 |  
 177 | +  # TODO: Consider enforcing IWYU across the entire codebase.

jonatack commented at 5:13 PM on June 2, 2025:

Last I looked, enforcing this across the codebase may require large changes at one time in some areas to untangle everything.

jonatack commented at 5:15 PM on June 2, 2025: member

Concept ACK

maflcko commented at 6:31 AM on June 3, 2025: member

only changes are addressed nits

re-review ACK 92ac4c8353c612549afbd68a4db6dfb04cc41dad 🍞

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-review ACK 92ac4c8353c612549afbd68a4db6dfb04cc41dad 🍞
xIdLhdsyYvMngIMaaDU1eiGvAgl1sm5eg7S5tdSf7AK3oTKfKIk7+1i0wvSWq5ufz6uOZG6iKygZDc7JHnCqCg==

</details>

DrahtBot requested review from jonatack on Jun 3, 2025

hebasto force-pushed on Jun 3, 2025

hebasto commented at 4:55 PM on June 3, 2025: member

Rebased.

All feedback from @maflcko and @fanquake has been addressed.

The IWYU-related workarounds are now documented with references to the relevant upstream issues.

in contrib/devtools/iwyu/bitcoin.core.imp:10 in ec42fb489a outdated

   6 | +  { include: [ "<smmintrin.h>", private, "<immintrin.h>", public ] },
   7 | +  { include: [ "<tmmintrin.h>", private, "<immintrin.h>", public ] },
   8 | +
   9 | +  # False positive warnings
  10 | +  # See: https://github.com/include-what-you-use/include-what-you-use/issues/1616
  11 | +  { include: [ "<iterator>", public, "<utility>", public ] },

maflcko commented at 6:47 AM on June 4, 2025:

I don't think this is right. This can lead to missing includes:

bash-5.2# cat map.imp 
[
  { include: [ "<iterator>", public, "<utility>", public ] },
]
bash-5.2# cat repr.cpp 
#include <utility>
#include <iterator>

void A(){
	int a{};
	int b{std::move(a)};
	auto* it{&b};
	std::advance(it,0);
}
bash-5.2# include-what-you-use  -Xiwyu --mapping_file=map.imp repr.cpp 

repr.cpp should add these lines:

repr.cpp should remove these lines:
- #include <iterator>  // lines 2-2

The full include-list for repr.cpp:
#include <utility>  // for move, advance
---
bash-5.2# vim repr.cpp # remove iterator include
bash-5.2# include-what-you-use  -Xiwyu --mapping_file=map.imp repr.cpp 
repr.cpp:8:7: error: no member named 'advance' in namespace 'std'
    8 |         std::advance(it,0);
      |         ~~~~~^

The correct fix is to just add the includes iwyu is asking for

hebasto commented at 2:23 PM on July 11, 2025:

Thanks! Updated.

in src/crypto/hex_base.cpp:12 in ec42fb489a outdated

   4 | @@ -5,8 +5,11 @@
   5 |  #include <crypto/hex_base.h>
   6 |  
   7 |  #include <array>
   8 | +#include <cassert>
   9 |  #include <cstring>
  10 |  #include <string>
  11 | +// A workaround for https://github.com/include-what-you-use/include-what-you-use/issues/1763.
  12 | +// IWYU pragma: no_include <tuple>

maflcko commented at 6:49 AM on June 4, 2025:

I don't think this is right. Can't this lead to missing the tuple include when it is required in the future?

The correct fix is to just add the includes iwyu is asking for

hebasto commented at 2:23 PM on July 11, 2025:

Thanks! Updated.

maflcko commented at 9:45 AM on June 7, 2025: member

lgtm, except for the two missing includes

re-ACK ec42fb489a7eabed4dec155c6655d36114fa67ba 👜

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK ec42fb489a7eabed4dec155c6655d36114fa67ba 👜
FEGC/u3sOGwaQIusYwFrEp5xYc5la53gzpRtrrSaUlHqwWAcpeIzsyPV2yxGl8A7HQezMFSpBdLyeqSk7dACAw==

</details>

achow101 referenced this in commit 5757de4ddd on Jun 11, 2025

DrahtBot added the label Needs rebase on Jun 11, 2025

in src/index/txindex.cpp:26 in ec42fb489a outdated

  22 |  #include <validation.h>
  23 |  
  24 | +#include <cassert>
  25 | +#include <cstdint>
  26 | +#include <exception>
  27 | +#include <fcntl.h>

maflcko commented at 5:08 PM on June 17, 2025:

This will need to go through the wrapper header, no?

src/compat/compat.h:#include <fcntl.h>       // IWYU pragma: export

Even better would be to use the more correct cstdio include.

hebasto commented at 2:22 PM on July 11, 2025:

Sure. Reworked.

hebasto force-pushed on Jul 11, 2025

hebasto commented at 2:22 PM on July 11, 2025: member

Rebased. @maflcko

Thank you for the review! Your feedback has been addressed.

DrahtBot removed the label Needs rebase on Jul 11, 2025

fanquake changes_requested

fanquake commented at 11:57 AM on July 14, 2025: member

This makes the CI dependant on being run on x86_64; the tidy job will no-longer pass when run on aarch64.

maflcko commented at 12:18 PM on July 14, 2025: member

may be good to fix the aarch64 ci. Otherwise:

re-ACK bfbf7de389649f848dffe23be6727df715648b01 🏖

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK bfbf7de389649f848dffe23be6727df715648b01 🏖
n66Unsx6c1Vu6Scpy871MQQIyXO4stQjbv/K4mjljYVqrYRkWt1hzxtnkVXvywtkwJFyeLW3pQCHEsKnvW+NBg==

</details>

fanquake commented at 12:19 PM on July 14, 2025: member

may be good to fix the aarch64 ci.

How would you fix it, given IWYU is giving different recommendations, based on the architecture?

ryanofsky commented at 12:37 PM on July 14, 2025: contributor

How would you fix it, given IWYU is giving different recommendations, based on the architecture?

Easiest way is to use // IWYU pragma: keep when IWYU incorrectly tells you to remove an include that should be kept and // IWYU pragma: no_include when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms. I dealt with this recently in https://github.com/bitcoin-core/libmultiprocess/commit/0d5f1faae5da26205ef9563cb9cc157725ab9479 because there were different recommendations when using libc++ and libstdc++.

maflcko commented at 12:49 PM on July 14, 2025: member

// IWYU pragma: no_include when it tells you to add an include that shouldn't be added. The pragmas will suppress errors on whatever platform is complaining and be ignored on other platforms.

I wouldn't recommend to do this, because this can lead to missing includes, as also mentioned earlier in #31308 (review). The correct fix is to just add the includes iwyu is asking for.

ryanofsky commented at 1:05 PM on July 14, 2025: contributor

The correct fix is to just add the includes iwyu is asking for.

I think this is about cases where IWYU on one platform asks you to add an include, and IWYU on other platform asks you to remove the same include. A reasonable way to resolve these is to decide which IWYU is correct and add pragmas to silence the other IWYU. Other ways to resolve it could be to define mappings, or submit fixes upstream, or choose a single platform to be your canonical IWYU platform and ignore other platforms.

I think pragmas are helpful because they make it easier to enable IWYU more places and allow developers to run it locally, but they do have downsides because they can become incorrect over time and be a maintenance burden. Many approaches seem reasonable and I don't have a recommendation, this was just a suggestion.

maflcko commented at 1:09 PM on July 14, 2025: member

Yes, the keep pragma seems useful, but I personally wouldn't recommend using the no_include pragma in this context

hebasto force-pushed on Jul 17, 2025

hebasto commented at 3:07 PM on July 17, 2025: member

may be good to fix the aarch64 ci.

Fixed.

maflcko commented at 3:29 PM on July 17, 2025: member

review ACK ec37e518682ca6c7272d1eeaea099dc9b3375e41 🌙

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK ec37e518682ca6c7272d1eeaea099dc9b3375e41 🌙
EjPGTyq0n0WQ15FhnkFUC4zbt8c7vpF2xwZ9xoKGZoHNODb4bbMv5amsaljEr1puiAij6EN74zIKaF/6EHHLBA==

</details>

in contrib/devtools/iwyu/bitcoin.core.imp:8 in ec37e51868 outdated

   0 | @@ -1,3 +1,16 @@
   1 | -# Nothing for now.
   2 |  [
   3 | +  # Compiler intrinsics.
   4 | +  # See: https://github.com/include-what-you-use/include-what-you-use/issues/1764.
   5 | +  { "include": [ "<emmintrin.h>", "private", "<immintrin.h>", "public" ] },
   6 | +  { "include": [ "<smmintrin.h>", "private", "<immintrin.h>", "public" ] },
   7 | +  { "include": [ "<tmmintrin.h>", "private", "<immintrin.h>", "public" ] },
   8 | +
   9 | +  # libc symbols.

fanquake commented at 3:30 PM on July 17, 2025:

Has this been upstreamed / why is it needed?

hebasto commented at 3:34 PM on July 17, 2025:

Has this been upstreamed...

Not yet.

why is it needed?

Otherwise, IWYU will suggest platform-specific headers instead of the standard one provided by the C library.

maflcko commented at 8:10 AM on September 1, 2025:

seems fine to upstream this, but this shouldn't be a blocker, i'd say

fanquake commented at 3:56 PM on September 8, 2025:

Not yet.

Is there anything blocking that? It'd be good if we'd atleast report issues upstream, before adding workarounds (with little-to-no documentation).

hebasto commented at 5:16 PM on September 8, 2025:

An upstream issue: https://github.com/include-what-you-use/include-what-you-use/issues/1809.

DrahtBot added the label Needs rebase on Aug 13, 2025

hebasto force-pushed on Aug 14, 2025

hebasto commented at 11:24 AM on August 14, 2025: member

Rebased to resolve a conflict with the merged #33116.

DrahtBot removed the label Needs rebase on Aug 14, 2025

maflcko commented at 8:10 AM on September 1, 2025: member

Only change is rebase.

re-ACK a4fa7b97aa73b177ea416388acfc2aba25c79e19 🚇

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK a4fa7b97aa73b177ea416388acfc2aba25c79e19 🚇
bRpP979EiaRYiW2rw/6yGXWljok64xDpRXXHzbyfOOiBMN+ftjgfNugbR0of17pwNoW+kOx/kZxEgzXhOkWhAQ==

</details>

DrahtBot added the label CI failed on Sep 1, 2025

in ci/test/03_test_script.sh:222 in a4fa7b97aa outdated

 224 | +             2>&1 | tee /tmp/iwyu_ci.out
 225 | +    python3 "/include-what-you-use/fix_includes.py" --nosafe_headers < /tmp/iwyu_ci.out
 226 | +  }
 227 | +
 228 | +  run_iwyu "compile_commands_iwyu_errors.json"
 229 | +  git --no-pager diff --exit-code

maflcko commented at 9:26 AM on September 1, 2025:

Could check the return code and print something to clarify this is an error, similar to the tidy error above:

    echo "^^^ ⚠️ Failure generated from clang-tidy"
    false

hebasto commented at 10:34 AM on September 1, 2025:

Thanks! Done.

hebasto force-pushed on Sep 1, 2025

hebasto commented at 10:34 AM on September 1, 2025: member

Rebased and addressed @maflcko's comment.

maflcko commented at 11:57 AM on September 1, 2025: member

re-ACK 1d49346465de8edec85bb2d498859b4fbc346935 📶

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 1d49346465de8edec85bb2d498859b4fbc346935 📶
2MJL8GNQQjZb4FgXDGl0qh67OHXwPeXDDWmlxONQfHs9hQfrdiETVAw+8nZ0IE+8qrWtyPqrQqZ7OXa2ZYcBCg==

</details>

DrahtBot removed the label CI failed on Sep 1, 2025

in ci/test/03_test_script.sh:47 in 1d49346465 outdated

  32 | @@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
  33 |  env
  34 |  echo "=== END env ==="
  35 |  
  36 | -(
  37 | +if [ "$RUN_TIDY" != "true" ]; then

l0rinc commented at 6:34 PM on September 1, 2025:

could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead? I have opened https://github.com/bitcoin-core/leveldb-subtree/pull/56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)

maflcko commented at 6:31 AM on September 2, 2025:

No objection, but I am not sure we should diverge the subtree for a test-only patch. Also, the patch is pre-existing, so it seems a bit unrelated to this pull.

Ideally, Google would fix the UB in some way or another, and then we can pull whatever their fix was, but that shouldn't be a blocker here.

l0rinc commented at 6:51 AM on September 2, 2025:

The main leveledb repo doesn't even compile for the past 8 months, not sure why we would wait for them. But as I mentioned, I don't mind doing it after the PR, just seems cleaner if we didn't need to change this section again.

in src/index/base.h:37 in 1d49346465 outdated

  38 |      bool synced{false};
  39 |      int best_block_height{0};
  40 |      uint256 best_block_hash;
  41 |  };
  42 | +namespace interfaces {
  43 | +struct BlockRef;

l0rinc commented at 6:40 PM on September 1, 2025:

not sure I understand why this was needed, but I haven't run the scripts, just tried to find out locally

maflcko commented at 6:31 AM on September 2, 2025:

fwd decls are slimmer than a full include, and often sufficient, for example when the header only uses the types as pointers or references. IWYU will suggest them sometimes, when possible.

l0rinc commented at 6:52 AM on September 2, 2025:

I know, that wasn't my question. But I'll figure it out next, feel free to resolve it.

hebasto commented at 8:01 AM on September 2, 2025:

https://github.com/bitcoin/bitcoin/blob/1d49346465de8edec85bb2d498859b4fbc346935/src/index/base.h#L128

l0rinc commented at 8:45 PM on September 1, 2025: contributor

Concept ACK

hebasto commented at 6:03 PM on September 4, 2025: member

@ryanofsky