Skip to content

ci: switch MSAN to use prebuilt Clang binaries #31850

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

ci: switch MSAN to use prebuilt Clang binaries #31850

wants to merge 1 commit into from

Conversation

fanquake
Copy link
Member

Use the packages from https://apt.llvm.org/.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 12, 2025 •
edited
Loading

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/31850.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31906 (ci: Switch to gcr.io mirror to avoid rate limits by maflcko)
  • #31802 (Add bitcoin-{node,gui} to release binaries for IPC by Sjors)
  • #30975 (ci: build multiprocess on most jobs by Sjors)
  • #28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

@DrahtBot DrahtBot added the Tests label Feb 12, 2025
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Debug: https://github.com/bitcoin/bitcoin/runs/37113437498

Hints

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.

fanquake
fanquake commented Feb 12, 2025
View reviewed changes
maflcko
maflcko approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. The clang compilation was done back when APT_LLVM_V wasn't available. Seems fine to use now and should be trivial to revert if there is an issue.

@willcl-ark
Copy link
Member

Also looks good to me!

A docker build (with --no-cache set, so only using a cached Ubuntu base image) on my 11700k went from:

[+] Building 1521.2s (9/9) FINISHED

to

[+] Building 196.8s (9/9) FINISHED

Should be a nice win on CI runs (and for users) where a rebuild would have previously been triggered (which can be caused by almost anything).

@fanquake
Copy link
Member Author

Swapped to rc2. Needs a fixup for MSAN fuzz.

@willcl-ark
Copy link
Member

For fuzzing end-users, you might consider touching up the wording in:

bitcoin/doc/fuzzing.md

Lines 104 to 112 in 55cf39e

## Using the MemorySanitizer (MSan)
MSan [requires](https://clang.llvm.org/docs/MemorySanitizer.html#handling-external-code)
that all linked code be instrumented. The exact steps to achieve this may vary
but involve compiling `clang` from source, using the built `clang` to compile
an instrumentalized libc++, then using it to build [Bitcoin Core dependencies
from source](../depends/README.md) and finally the Bitcoin Core fuzz binary
itself. One can use the MSan CI job as an example for how to perform these
steps.

...to directly mention using pre-built llvm bins (although it does already say to use the CI job as a reference which is and will remain accurate).

This was referenced Feb 14, 2025
Merged
Merged
Closed
@maflcko
Copy link
Member

maflcko commented Feb 18, 2025

Needs a fixup for MSAN fuzz.

What is the error?

@DrahtBot DrahtBot mentioned this pull request Feb 19, 2025
Merged
@maflcko
Copy link
Member

maflcko commented Feb 20, 2025

Needs a fixup for MSAN fuzz.

What is the error?

Yeah, I don't think this works. This is probably #24289

@fanquake
Copy link
Member Author

Yea, I don't think so. We can circle back around to this soon.

@fanquake fanquake closed this Feb 20, 2025
@maflcko maflcko mentioned this pull request Mar 9, 2025
Open
@maflcko maflcko mentioned this pull request Jul 17, 2025
Merged
@fanquake
Copy link
Member Author

PIcked up in #32999.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fanquake @DrahtBot @willcl-ark @maflcko