This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled
ClosedPublic

Authored by baloghadamsoftware on Nov 29 2018, 1:54 AM.

Details

Summary

During the review of D41938 a condition check with an early exit accidentally slipped into a branch, leaving the other branch unprotected. This may result in an assertion later on. This hotfix moves this contition check outside of the branch.

Please review it quickly! It should be released in clang 7.0.1.

Diff Detail

Event Timeline

Szelethus added a comment.EditedNov 29 2018, 4:06 AM

Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce.

Edit: Nevertheless, looks good to me.

Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce.

Edit: Nevertheless, looks good to me.

There could be probably a test provided, but the bug is very obvious if you look at the code. Also if you compare it with the first version of the original patch.

Have you seen a crash resulting from this? Is supplying a test case feasable? I know that some of these errors are extremely hard to reproduce.

Edit: Nevertheless, looks good to me.

There could be probably a test provided, but the bug is very obvious if you look at the code. Also if you compare it with the first version of the original patch.

I'm sold on that. Let's wait for someone else then to formally accept. Please note that AFAIK the merge request deadline for 7.0.1 is tomorrow (nov. 30), so if putting this in is critical, writing a cfe-dev mail with a scary title may be a good idea.

NoQ added a comment.Nov 29 2018, 1:20 PM

Test?

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
485

I guess it's no longer only about subtracting?

NoQ added a comment.Nov 29 2018, 1:22 PM

Whoops, i was reviewing so quickly i didn't notice other comments :D

I still believe that this needs a regression test, for the same reason for which we need tests at all. If the deadline for committing comes before you manage to reduce the test case, please commit, but please add a test case as soon as possible.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 30 2018, 2:40 AM
This revision was automatically updated to reflect the committed changes.

Committed because I was notified that the deadline for 7.0.1 is today.