Page MenuHomePhabricator

[analyzer] Use the signature of the primary template for issue hash calculation
ClosedPublic

Authored by xazax.hun on Oct 10 2017, 6:43 AM.

Details

Summary

Right now when a template is instantiated more times and there is a bug in the instantiations the issue hash will be different for each instantiation even if every other property of the bug (path, message, location) is the same.

This patch aims to resolve this issue. Note that explicit specializations still generate different hashes but that is intended.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun created this revision.Oct 10 2017, 6:43 AM
martong added inline comments.Oct 10 2017, 8:42 AM
lib/StaticAnalyzer/Core/IssueHash.cpp
39 ↗(On Diff #118374)

Could we use here FunctionDecl::getPrimaryTemplate() ? That seems more general, it handles both specializations and instantiations.

test/Analysis/bug_hash_test.cpp
61 ↗(On Diff #118374)

We could add a few more test cases:

  • a function template in class template
  • specializations vs instantiations
  • the combination of the above two (?)
1363 ↗(On Diff #118374)

I am not sure if this is possible, but could we add unit test just for the GetSignature function? Instead of these huge plist files?

I am thinking something like this:
https://github.com/martong/friend-stats/blob/ed0c69ea3669c933204c799f59b85cd7b2507c34/ut/FriendFunctionsTest.cpp#L31

xazax.hun updated this revision to Diff 118591.Oct 11 2017, 4:45 AM
xazax.hun marked 3 inline comments as done.
xazax.hun edited the summary of this revision. (Show Details)
  • Added more test cases
  • Made the tests more concise
lib/StaticAnalyzer/Core/IssueHash.cpp
39 ↗(On Diff #118374)

Unfortunately getPrimaryTemplate is not sufficient. The function might be a member function of a template class. In this case, there is no primary template for the function (only for the enclosing class) but it still depends on a template parameter.

test/Analysis/bug_hash_test.cpp
61 ↗(On Diff #118374)

Good point.

1363 ↗(On Diff #118374)

I think it is more convenient to use regression test for this purpose than unittests. But I replaced the long plist checking with something much more concise.

martong added inline comments.Oct 11 2017, 5:41 AM
test/Analysis/bug_hash_test.cpp
105 ↗(On Diff #118591)

As we discussed, the checking of the equality of the IssueString in case of TX<int> and TX<double> is implicit. And as such it is hard to see that it is really tested. Perhaps a comment here could indicate this implicit checking mechanism.

xazax.hun updated this revision to Diff 118749.Oct 12 2017, 1:01 AM
xazax.hun marked an inline comment as done.
  • Added some comments to the tests
NoQ edited edge metadata.Oct 12 2017, 1:56 AM

Ideas behind both hashing change and new testing mechanism look great to me.

I think it would be great to split them into two different patches, to be able to easily see how the change in the hashing affects the tests (and maybe revert easily if something goes wrong).

lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
88 ↗(On Diff #118749)

clang_analyzer_hashDump would be more consistent.

In D38728#895669, @NoQ wrote:

I think it would be great to split them into two different patches, to be able to easily see how the change in the hashing affects the tests (and maybe revert easily if something goes wrong).

So you would commit the hash change first and the test change on the top of it? Or the other way around?

NoQ added a comment.Oct 12 2017, 2:01 AM

The other way round, i guess. I like the test change, it's easier to understand, so it's better to have it before starting to understand :)

martong accepted this revision.Oct 12 2017, 2:18 AM
This revision is now accepted and ready to land.Oct 12 2017, 2:18 AM
xazax.hun updated this revision to Diff 118788.Oct 12 2017, 7:36 AM
  • Rebase based on the dependent revision and minor cleanups
This revision was automatically updated to reflect the committed changes.