Removed the GDM checking what could prevent reports made by this visitor. Now we rely on constraint changes instead.
- rGcf0b4e32eb8a: [analyzer] ConditionBRVisitor: Remove GDM checking
rL356322: [analyzer] ConditionBRVisitor: Remove GDM checking
rC356322: [analyzer] ConditionBRVisitor: Remove GDM checking
rGf962485adad9: [analyzer] ConditionBRVisitor: Remove GDM checking
rL356318: [analyzer] ConditionBRVisitor: Remove GDM checking
rC356318: [analyzer] ConditionBRVisitor: Remove GDM checking
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.
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.
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 :/)
- 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.
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.
I think that haveSameConstraints(S1, S2) is a slightly more friendly API.
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!