Skip to content

C++: SimpleRangeAnalysis: Always normalize bounds after a computation#4252

Merged
MathiasVP merged 2 commits intogithub:mainfrom
jbj:normalize-bounds
Sep 14, 2020
Merged

C++: SimpleRangeAnalysis: Always normalize bounds after a computation#4252
MathiasVP merged 2 commits intogithub:mainfrom
jbj:normalize-bounds

Conversation

@jbj
Copy link
Contributor

@jbj jbj commented Sep 11, 2020

Fixes #4243.

jbj added 2 commits September 11, 2020 11:52
This stops some cases of `-0.0` from propagating through the range
analysis, fixing a false positive on arvidn/libtorrent.

There seems to be no need for a corresponding change in the caller of
`getDefLowerBoundsImpl` since that predicate only contains computations
that cannot introduce negative zero.
@jbj jbj requested a review from a team as a code owner September 11, 2020 10:02
@github-actions github-actions bot added the C++ label Sep 11, 2020
@jbj
Copy link
Contributor Author

jbj commented Sep 11, 2020

We might want to run a CPP-Differences job to validate this, but first I'll make sure the tests pass.

@jbj
Copy link
Contributor Author

jbj commented Sep 11, 2020

The checks have passed, except for a check that's broken on all PRs.

I've started https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/1424.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM, will merge when the diff is complete.

@jbj
Copy link
Contributor Author

jbj commented Sep 14, 2020

The CPP-Differences job shows no result changes and no significant performance changes. There is a slight slowdown in SignedOverflowCheck.ql, which uses range analysis, but that same slowdown is present in TaintedPath.ql, which doesn't.

@MathiasVP MathiasVP merged commit 34a57e2 into github:main Sep 14, 2020
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.

LGTM.com - false positive (integer negative 0)

3 participants