Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 29, 2021

The ubsan suppressions for test files have several issues:

  • They bloat the suppressions file, distracting from real bugs
  • They are file-wide, thus suppressing any other (newly introduced) issues in the same file
  • Some of them are causing compile issues with -Wimplicit-int-conversion

Fix all issues by making the integer truncations or sign changes explicit.

This is a refactor that shouldn't change the test binary unless compiled with sanitizers.

@maflcko maflcko added the Tests label Nov 29, 2021
@maflcko maflcko force-pushed the 2111-testInt branch 2 times, most recently from fa9627f to fa9bab0 Compare January 2, 2022 15:35
@maflcko
Copy link
Member Author

maflcko commented Jan 2, 2022

Rebased for fresh CI

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 3, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23962 (Use int64_t type for transaction sizes consistently 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.

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Code review ACK fa9bab0

@maflcko
Copy link
Member Author

maflcko commented Jan 20, 2022

Apologies, but I decided to force push to replace unsigned(~0x00800000) with ~0x00800000U to avoid a c-style cast where it is not needed. no other changes other than rebase

@jonatack
Copy link
Member

ACK faedb11 per git range-diff 1824644a fa9bab0 faedb11 and clang 13 debug build sanity check

@maflcko maflcko merged commit b60c477 into bitcoin:master Jan 20, 2022
@maflcko maflcko deleted the 2111-testInt branch January 20, 2022 17:13
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 21, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jan 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants