ci: Integrate bitcoin-tidy clang-tidy plugin

[fanquake]

Demo of integrating the bitcoin-tidy, clang-tidy plugin written by theuni into our tidy CI job.

The plugin currently has a single check, bitcoin-unterminated-logprintf. This would replace our current Python driven, git-grep-based, .cpp file only, lint-logs linter.

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theuni, MarcoFalke, TheCharlatan
Concept ACK	dergoegge
Approach ACK	stickies-v

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:

• #28087 (ci: Use qemu-user through container engine by MarcoFalke)
• #27976 (ci: Start with clean env by MarcoFalke)
• #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

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]

If this requires clang-15, I'd prefer to wait until that is backported to Ubuntu Jammy instead of hopping the short-term releases.

[fanquake]

If this requires clang-15, I'd prefer to wait until that is backported to Ubuntu Jammy instead of hopping the short-term releases.

This doesn't actually require LLVM 15, it's just that the run-clang-tidy script shipped with LLVM 15 has support for passing -load arguments through to clang-tidy.

I've dropped the Kinetic / LLVM 15 bump, in favour of a different approach; patching -load support into the llvm 14 run-clang-tidy script.

[theuni]

Concept ACK (obviously). Thought dump below.

A few things worth considering when it comes to these plugins:

• They do exactly what you tell them to do - no more and no less.
• Tests are really important
• We may want different checks for certain parts of the code.

Elaborating:

c++ is complicated, so defining compiler rules is also complicated. Taking this code for a trivial example:

struct A{};
void func()
{
    A foo;
    const A bar;
}

In testing, one may find that the following query works to find all declarations of type A:
varDecl(hasType(asString("struct A")))
However, this will find only the non-const-qualified foo and not bar.

Instead, we'd want something more like: varDecl(hasType(cxxRecordDecl(hasName("A"))))

I'm mentioning this to try to demonstrate what I meant by "they do exactly what you tell them to". It's quite common (especially when just getting started) to end up with an overly-broad or unnecessarily specific query. The first query would appear to be fine if a const A is not used anywhere. The best defense against this from what I can tell is adding a test for each failing corner-case.

So I think we'll likely want some pretty thorough documentation that states exactly what the check is intended to do. Additionally, we'll want tests that verify those intentions. In the case of the plugin here, I have added some loose checks here but nothing automated: https://github.com/theuni/bitcoin-tidy/blob/main/test_desig_init.cpp . An example of a corner-case that wasn't caught at first with this check is included here: nested structs with uninitialized members.

tl;dr: before merge, I think we'll want to add a mini test suite to the plugin repo that allows us to verify that checks are working as defined/intended. Additionally, there will need to be some understanding that checks will likely require refinement over time as new corner cases are exposed.

• We may want different checks for certain parts of the code.

This isn't important for now, but I suspect that the infrastructure for this will end up being more complicated eventually. The obvious outlier (imo) is libbitcoinkernel, where we may want to add more strict checks that other parts of the code (bitcoin-qt for ex) aren't required to abide by.

[dergoegge]

Concept ACK

[maflcko]

Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .

Edit: I guess one is for constructors, the other is for aggregates, so they are orthogonal, and go hand-in-hand.

See also #26762 (comment)

[fanquake]

Would be good to explain how this is different from https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-type-member-init.html .

Last I checked, turning that on was essentially unusable, due to false-positives. Will take another look.

[maflcko]

It should only hit on classes that have a SetNull method, no? In those cases the SetNull method can be removed, or the initializers duplicated from the method?

[hebasto]

Last I checked, turning that on was essentially unusable, due to false-positives.

Indeed.

[fanquake]

Dropped the test commit back out.

[theuni]

ACK 1c976c6

[maflcko]

re-ACK 1c976c6 👠

Show signature

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 1c976c691cc4b20f43071aabf36c7afed1571057  👠
AUAtgJBmTgwGLADs8IgqfpOQg2nQfQ2+D/7P8lwnKkrIGJwTNAjXOYnonfN9oAT1L5+CTeUJzKaPpGHVi5t7Cw==

[TheCharlatan]

ACK 1c976c6

[maflcko]

rfm or is anything left to be done here for this test-only pull?

[fanquake]

is anything left to be done here

I missed that Cory had already ACK'd, and was waiting for him to re-ACK. Given that's already happened, I don't think there's anything left here, and docs can be fixed in the followup.

[maflcko]

Maybe also add an example for common systems, Ubuntu/Debian-based ones, and Fedora/CentOS-based ones, what DLLVM_DIR should be for them?

[fanquake]

Done in #28245.

[maflcko]

This is wrong. The argument of WalletLogPrintf is never a string literal (the parameter is).

[maflcko]

Fixed in #28237

[maflcko]

This is wrong, too. ScriptPubKeyMan has a log statement, too.

My recommendation would be to just remove this line completely, unless there is a reason to have it. The other lines should be exact enough to match everything that is needed, without under- or over-matching.

[theuni]

Ugh, I was inclined to argue with you because I think the tests should be as tight as possible. But realistically, it's more likely for a class to be forgotten in the checks (as has happened here) than a false-positive in some future class. So I begrudgingly agree.

[maflcko]

Mind creating a pull with the outstanding feedback? :)

If not, I'll try to do it next week.

[maflcko]

Fixed in #28237

[theuni]

Yep, will do. I was just waiting for the others to settle.