Skip to content

C++: Fix SignedOverflowCheck.ql performance #2404

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

Merged
merged 5 commits into from
Nov 25, 2019

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Nov 21, 2019

See commit messages.

jbj added 2 commits November 21, 2019 10:40
The `SignedOverflowCheck.ql` query was very slow on certain snapshots
(jluttine/suitesparse and Chromium) due to bad magic in
`MacroInvocation::getAnAffectedElement_dispred#fb`. This commit doesn't
fix the bad magic but changes the exclusion mechanism to use a predicate
where we can better control the magic and optimization.

The query should also give more good results due to this new exclusion
mechanism, which is the same one used in its sibling,
`PointerOverflow.ql`.
This fixes the performance of `SignedOverflowCheck.ql` on
jluttine/suitesparse.
@jbj jbj added the C++ label Nov 21, 2019
@jbj jbj added this to the 1.23 milestone Nov 21, 2019
@jbj jbj requested a review from a team as a code owner November 21, 2019 10:03
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, and produces the promised speedup.

I'm seeing new results affected by macros on FFmpeg/FFmpeg and libretro/mame2003-plus-libretro, however. Compare https://lgtm.com/query/2133993824516378217/ (before) with https://lgtm.com/query/1000398469093303802/ (after).

jbj added 2 commits November 21, 2019 14:02
This fixes a failing qltest and makes the exclusion similar to what's in
`PointerOverflow.ql`. It's possible we should exclude based on both `+`
and `<`, but we can revisit that if false positives show up.
@jbj
Copy link
Contributor Author

jbj commented Nov 22, 2019

https://lgtm.com/query/8781856674325036692/ shows that the FFmpeg FPs go away after
bd4fa10, but the MAME FP remains. I'll see if I can fix that easily.

@jbj
Copy link
Contributor Author

jbj commented Nov 22, 2019

I've just started https://lgtm.com/query/9140037359670573854/ to test matching end locations instead of start locations.

This should fix a FP in libretro/mame2003-plus-libretro.
@jbj
Copy link
Contributor Author

jbj commented Nov 22, 2019

I've pushed an attempted fix, but I haven't verified it yet. I'll follow up later.

@jbj
Copy link
Contributor Author

jbj commented Nov 22, 2019

The new results LGTM: the FPs in both FFmpeg and MAME are gone. I hope you also find the QL code to be less strange.

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.

Looks good and is behaving.

@geoffw0 geoffw0 merged commit 1d26d4c into github:rc/1.23 Nov 25, 2019
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.

2 participants