This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] ConditionBRVisitor: Remove GDM checking
ClosedPublic

Authored by Charusso on Nov 21 2018, 1:51 PM.

Diff Detail

Repository
rC Clang

Event Timeline

Charusso created this revision.Nov 21 2018, 1:51 PM
gerazo added a subscriber: gerazo.Nov 26 2018, 7:20 AM
NoQ added a comment.Nov 30 2018, 4:30 PM

Does this change the observed behavior? If so, a test would be great! If this is a part of D53076, please attach to D53076. If this could only be tested after D53076, please add a test anyway and get these patches committed in the respective order.

I guess it's going to be *less* notes emitted after the patch, because every update in constraint ranges is also a GDM update, so we'll be bailing out more often. But i doubt that there are many situations where something else changes in the GDM and it causes us to emit a note in *this* visitor, so i think the change makes sense.

In D54811#1315431, @NoQ wrote:

Does this change the observed behavior? If so, a test would be great! If this is a part of D53076, please attach to D53076. If this could only be tested after D53076, please add a test anyway and get these patches committed in the respective order.

I am not really into the checker business as I started to work with the 'front-end', so I cannot write a test for now. I have ran CodeChecker with the default 'analyze' command before and after the patch on the following projects: bitcoin, curl, ffmpeg, git, memcached, nginx, openssl, postgresql, tinyxml2, tmux, twin, vim, xerces. After that I made a test that should not pass: the commit hash is different in every file (comparing the files in before/after folders), so I have changed the test with diffing '<(tail -n +8 "$file")' magic. There is no difference at all, so I think it should work fine to continue with other patches. (Z3-compatibility could be problematic as I have no experience with that and I cannot test it currently.)

I guess we used to have something that would result in an event duplication (which was the motivation for this patch), but it's long forgotten.

(Z3-compatibility could be problematic as I have no experience with that and I cannot test it currently.)

That is my no1 concern too. Luckily, we do have lit tests for Z3, if you have the time to configure it (as I sadly don't :/)

Charusso updated this revision to Diff 183713.Jan 26 2019, 12:30 PM
  • Redownloaded and retested the following projects with the same technique as mentioned above with no difference in the plists: bitcoin, curl, ffmpeg, git, memcached, nginx, openssl, postgres, redis, sqlite, tmux, vim, xerces. (This is 8 GB of pure errors.)
  • ninja check-clang passes well.
  • ninja check-clang-analyzer-z3 passes as the trunk version.

Thanks you @Szelethus! I found the lit tests.

The output from the trunk and after the patch running ninja check-clang-analyzer-z3:

Failing Tests (1):
    Clang :: Analysis/plist-macros.cpp

  Expected Passes    : 565
  Expected Failures  : 2
  Unsupported Tests  : 3
  Unexpected Failures: 1

The behaviour is the same before the patch. I have not found a way to eliminate this error as it is a plist-style test and it is impossible to inject #ifndef ANALYZER_CM_Z3 to differentiate the default test.

NoQ added a comment.Jan 28 2019, 5:27 PM

Ok! I'll try to evaluate it and see if i an example of improvement shows up - this would probably be easier than to come up with one manually.

include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
82–85

I think that haveSameConstraints(S1, S2) is a slightly more friendly API.

Charusso updated this revision to Diff 184129.Jan 29 2019, 11:05 AM

Better API.

NoQ accepted this revision.Mar 6 2019, 5:22 PM

This indeed seems to be a no-functional-change patch. For now we don't have checkers that actively add transitions after eagerly-assume operators. evalAssume() doesn't count - as we've recently discussed, it does allow checkers to actively participate in modeling, but it doesn't allow adding transitions :) So we're pretty much stuck with only the uninitialized variable checker that finds binary operators with undefined results, but it doesn't add transitions either; it only generates fatal errors. So for now the invariant "on block edges, GDMs are different iff constraint ranges are different" seems to hold and the original code was working correctly, even if accidentally.

Let's commit! Do you have commit access? You might want to get commit access and commit yourself, 'cause that's a lot of patches!

This revision is now accepted and ready to land.Mar 6 2019, 5:22 PM
Charusso retitled this revision from [analyzer] Remove GDM checking from the ConditionBRVisitor to [analyzer] ConditionBRVisitor: Remove GDM checking.Mar 8 2019, 1:48 PM
In D54811#1420868, @NoQ wrote:

Let's commit! Do you have commit access? You might want to get commit access and commit yourself, 'cause that's a lot of patches!

Thanks you! I will ask for commit access soon.

This revision was automatically updated to reflect the committed changes.