Skip to content
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

ci: remove --with-asm=no (secp256k1) from MSAN jobs #29742

Merged
merged 2 commits into from Apr 7, 2024
Merged

ci: remove --with-asm=no (secp256k1) from MSAN jobs #29742

merged 2 commits into from Apr 7, 2024

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Mar 26, 2024 •
edited

Bumps LLVM to 18.1.3:

Drops --with-asm=no (only being passed to secp256k1) from the MSAN CI. New MSAN annotations were pulled in as part of #29803.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 26, 2024 •
edited

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 hebasto, maflcko
Concept ACK theuni, real-or-random

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:

  • #29790 ([DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP by hebasto)

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.

@fanquake fanquake mentioned this pull request Mar 26, 2024
Merged
theuni
theuni reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

Nice. Obvious concept ACK. Can discuss the secp subtree bump separately.

Why is the llvm 18.1.2 bump bundled here, though?

@fanquake
Copy link
Member Author

Why is the llvm 18.1.2 bump bundled here, though?

Mostly just because I was going to include another MSAN patch, to fix the instrumentation on libuwind (llvm/llvm-project#84348) but haven't quite got that working yet.

Note that depending on timings, if llvm/llvm-project#86201 goes into the next 18.x release, I'll want to bump/pull that in too, because it'll remove the need for a workaround when running the CI locally on newer kernels (same issue as this one bitcoin-core/secp256k1#1506). Happy to split up the changes in any case, just had a MSAN bundle to test here.

@DrahtBot
Copy link
Contributor

🚧 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/23120272058

@maflcko
Copy link
Member

maflcko commented Mar 27, 2024

Working ok locally.

Seems to be failing in CI. I guess due to build-architecture differences, or is there a bug in the CI system?

@fanquake fanquake mentioned this pull request Mar 27, 2024
Closed
@fanquake
Copy link
Member Author

Seems to be failing in CI. I guess due to build-architecture differences, or is there a bug in the CI system?

I think so:
aarch64
native_msan: pass
native_fuzz_with_msan: pass

x86_64
native_msan: fail: (reported here: bitcoin-core/secp256k1#1511). pass: with the secp256k1 tests disabled.
native_fuzz_with_msan: pass

@fanquake fanquake mentioned this pull request Mar 27, 2024
Merged
@DrahtBot DrahtBot mentioned this pull request Apr 2, 2024
Draft
@theuni
Copy link
Member

theuni commented Apr 3, 2024

Ready for bump now that bitcoin-core/secp256k1#1512 is merged.

@DrahtBot DrahtBot mentioned this pull request Apr 4, 2024
Merged
@fanquake
Copy link
Member Author

fanquake commented Apr 4, 2024

Ready for bump now that bitcoin-core/secp256k1#1512 is merged.

Converted to a proper subtree bump.
Rewrote the PR description.

@fanquake fanquake mentioned this pull request Apr 4, 2024
Merged
real-or-random
real-or-random reviewed Apr 4, 2024
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment •
edited

Choose a reason for hiding this comment

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

Concept ACK on removing --with-asm=no

This was referenced Apr 4, 2024
Draft
Draft
Draft
fanquake added a commit that referenced this pull request Apr 6, 2024
@fanquake
Merge #29803: Update libsecp256k1 subtree to latest master
b5d2118 
53eec53 Squashed 'src/secp256k1/' changes from efe85c70a2..d8311688bd (fanquake)

Pull request description:

  Updates the libsecp256k1 subtree to bitcoin-core/secp256k1@d831168.

  Part of #29742. See that PR for more details, the particularly relevant changes are:
  * bitcoin-core/secp256k1#1496
  * bitcoin-core/secp256k1#1512

ACKs for top commit:
  theuni:
    ACK 4654cc3
  jonasnick:
    utACK 4654cc3

Tree-SHA512: 84e711e9245ced6cc679e082f597d096361d8824c6ff7de2d4d7f59adb3316464b3643ffa588a899345cb88532672a66968b6c66c51b1924adf4441f54427277
fanquake added a commit to fanquake/oss-fuzz that referenced this pull request Apr 6, 2024
@fanquake
bitcoin-core: remove --with-asm=no from MSAN job
24f4bff 
Can be dropped after bitcoin/bitcoin#29803.
Equivalent in our CI is bitcoin/bitcoin#29742.
@fanquake fanquake changed the title [WIP] ci: test secp256k1 MSAN asm annotations ci: remove --with-asm=no (secp256k1) from MSAN jobs Apr 6, 2024
@fanquake fanquake marked this pull request as ready for review April 6, 2024 08:44
@fanquake fanquake mentioned this pull request Apr 6, 2024
Merged
@fanquake
Copy link
Member Author

fanquake commented Apr 6, 2024

Opened google/oss-fuzz#11785 for oss-fuzz.

Also cc @jonasnick.

hebasto
hebasto approved these changes Apr 6, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 61641e2.

@fanquake
Copy link
Member Author

fanquake commented Apr 6, 2024

Have run this successfully on all 4 MSAN jobs locally.

@maflcko
Copy link
Member

maflcko commented Apr 7, 2024

lgtm ACK 61641e2

@fanquake fanquake merged commit bb1383e into bitcoin:master Apr 7, 2024
16 checks passed
@fanquake fanquake deleted the secp256k1_msan_fixups branch April 7, 2024 09:50
jonathanmetzman pushed a commit to google/oss-fuzz that referenced this pull request Apr 8, 2024
@fanquake
bitcoin-core: remove --with-asm=no from MSAN job (#11785)
e8affd7 
Can be dropped after bitcoin/bitcoin#29803.
Equivalent in our CI is bitcoin/bitcoin#29742.
Pttn added a commit to RiecoinTeam/Riecoin that referenced this pull request Apr 13, 2024
@Pttn
Merge bitcoin#29742
9fa7d11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko left review comments

@hebasto hebasto hebasto approved these changes

@real-or-random real-or-random Awaiting requested review from real-or-random

@theuni theuni Awaiting requested review from theuni

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fanquake @DrahtBot @maflcko @theuni @real-or-random @hebasto