Skip to content

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

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
Merged
fanquake merged 1 commit into from
Mar 5, 2024
Merged

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

fanquake 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 •
edited
Loading

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
@hebasto hebasto mentioned this pull request Mar 3, 2024
Closed
hebasto
hebasto approved these changes Mar 3, 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 217c0ce.

However, I'd prefer an alternative approach.

@Sjors
Copy link
Member

Sjors commented Mar 5, 2024 •
edited
Loading

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

Sjors
Sjors reviewed Mar 5, 2024
View reviewed changes
test/sanitizer_suppressions/ubsan
unsigned-integer-overflow:TxConfirmStats::EstimateMedianVal
unsigned-integer-overflow:prevector.h
unsigned-integer-overflow:script/interpreter.cpp
unsigned-integer-overflow:EvalScript
Copy link
Member

@Sjors Sjors Mar 5, 2024

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

@fanquake fanquake Mar 5, 2024

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.

dergoegge
dergoegge approved these changes Mar 5, 2024
View reviewed changes
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
@fanquake fanquake deleted the more_ubsan_symbolizing branch March 5, 2024 16:54
@bitcoin bitcoin locked and limited conversation to collaborators Mar 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@Sjors Sjors Sjors left review comments

@dergoegge dergoegge dergoegge 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.

5 participants

@fanquake @DrahtBot @Sjors @dergoegge @hebasto