This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] A testing facility for testing relationships between symbols.
ClosedPublic

Authored by NoQ on Sep 14 2018, 8:28 PM.

Details

Summary

A test introduced in rC329780 was disabled in rC342317 for the reason explained in https://reviews.llvm.org/D41938?id=130403#inline-371070 - tests shouldn't test dump infrastructure when all they care about is how symbols relate to each other.

Add a new feature to ExprInspection: clang_analyzer_denote() and clang_analyzer_explain(). The former adds a notation to a symbol, the latter expresses another symbol in terms of previously denoted symbols.

It's currently a bit wonky - doesn't print parentheses and only supports denoting atomic symbols. But it's even more readable that way.

I also noticed that an important use case was omitted in these tests. Namely, tests for unsigned integer rearrangement are done with signed-type symbols stored in unsigned variables and we also need to test unsigned symbols stored in unsigned variables (and ideally also unsigned symbols stored in signed variables).

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Sep 14 2018, 8:28 PM
MTC added a subscriber: MTC.Sep 16 2018, 7:41 PM

This looks better than using the "raw" dumps.

Should not it we have its own test in expr-inspection.c?

lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
65 ↗(On Diff #165635)

Why const void *?

NoQ updated this revision to Diff 165810.Sep 17 2018, 12:53 PM

Address comments. Add more sanity checks and test them.

NoQ marked an inline comment as done.Sep 17 2018, 12:55 PM

Should not it we have its own test in expr-inspection.c?

This isn't usually necessary when we're testing all code paths anyway, but i guess it's worth it to test our sanity-check warnings.

lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
65 ↗(On Diff #165635)

Whoops, i was incorrectly recalling that it doesn't work because the respective type trait structures aren't defined for non-void pointers, but that must have been some other data structure.

@NoQ Actually I agree with @baloghadamsoftware that it makes sense to have a separate test, as this functionality should be tested regardless of svalbuilder-rearrange-comparisons existence.

NoQ marked an inline comment as done.Sep 17 2018, 1:15 PM

@NoQ Actually I agree with @baloghadamsoftware that it makes sense to have a separate test, as this functionality should be tested regardless of svalbuilder-rearrange-comparisons existence.

That's why i added a spearate test.

NoQ added a comment.Sep 17 2018, 1:16 PM

Mm, i mean, that reason as good as well.

george.karpenkov accepted this revision.Sep 17 2018, 1:17 PM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
273 ↗(On Diff #165810)

Sidenote: this code fragment is so common I wonder whether we should have a helper/facility for that.

This revision is now accepted and ready to land.Sep 17 2018, 1:17 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.