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.

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

test: remove file-wide interpreter.cpp ubsan suppression #29541

Merged
merged 1 commit into from Mar 5, 2024

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Mar 2, 2024

No description provided.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2024

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, Sjors, dergoegge

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:

  • #29543 (refactor: Avoid unsigned integer overflow in script/interpreter.cpp 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.

@DrahtBot DrahtBot changed the title test: remove file-wide interpreter.cpp ubsan suppression test: remove file-wide interpreter.cpp ubsan suppression Mar 2, 2024
@DrahtBot DrahtBot added the Tests label Mar 2, 2024
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 217c0ce.

However, I'd prefer an alternative approach.

@Sjors
Copy link
Member

Sjors commented Mar 5, 2024

Is it possible to mark suppressions inside the source code itself? In that case we could specifically suppress the two affected lines. That has the benefit of not having to touch the code itself (like #29543 does), while avoiding future regressions elsewhere in EvalScript.

utACK 217c0ce

@@ -55,7 +55,7 @@ unsigned-integer-overflow:MurmurHash3
unsigned-integer-overflow:CBlockPolicyEstimator::processBlockTx
unsigned-integer-overflow:TxConfirmStats::EstimateMedianVal
unsigned-integer-overflow:prevector.h
unsigned-integer-overflow:script/interpreter.cpp
unsigned-integer-overflow:EvalScript
Copy link
Member

Choose a reason for hiding this comment

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

I assume stacktop / altstacktop don't work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in this context, because these are runtime suppressions.

@fanquake
Copy link
Member Author

fanquake commented Mar 5, 2024

Is it possible to mark suppressions inside the source code itself?

There are various ways to suppress issues, however, in this PR, I'm not interested in changing the src, just reducing the scope of the currently applied runtime suppression.

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

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

ACK 217c0ce

@fanquake
Copy link
Member Author

fanquake commented Mar 5, 2024

I'm going to merge this to reduce the scope of the suppression, discussion around refactoring / inlining suppressions could can in #29543.

@fanquake fanquake merged commit faff279 into bitcoin:master Mar 5, 2024
16 checks passed
@fanquake fanquake deleted the more_ubsan_symbolizing branch March 5, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants