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

[xazax.hun]

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.

[martong]

lib/StaticAnalyzer/Core/IssueHash.cpp

39

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

test/Analysis/bug_hash_test.cpp

71–72

We could add a few more test cases:

• a function template in class template
• specializations vs instantiations
• the combination of the above two (?)

72

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]

• Added more test cases
• Made the tests more concise

lib/StaticAnalyzer/Core/IssueHash.cpp

39

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

71–72

Good point.

72

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]

test/Analysis/bug_hash_test.cpp

105

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]

• Added some comments to the tests

[NoQ]

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.

[xazax.hun]

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]

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 :)

[xazax.hun]

• Rebase based on the dependent revision and minor cleanups

[zaks.anna]

LGTM!

lib/StaticAnalyzer/Core/IssueHash.cpp

 
 // Get a string representation of the parts of the signature that can be 
 // overloaded on.
 static std::string GetSignature(const FunctionDecl *Target) {
   if (!Target)
     return "";
   std::string Signature;
 
+  // When a flow sensitive bug happens in templated code we should not generate
+  // distinct hash value for every instantiation. Use the signature from the
+  // primary template.
+  if (const FunctionDecl *InstantiatedFrom =
+          Target->getTemplateInstantiationPattern())
+    Target = InstantiatedFrom;
+
   if (!isa<CXXConstructorDecl>(Target) && !isa<CXXDestructorDecl>(Target) &&
       !isa<CXXConversionDecl>(Target))
     Signature.append(Target->getReturnType().getAsString()).append(" ");
   Signature.append(Target->getQualifiedNameAsString()).append("(");
 
   for (int i = 0, paramsCount = Target->getNumParams(); i < paramsCount; ++i) {
     if (i)
       Signature.append(", ");

test/Analysis/bug_hash_test.cpp

void AA::X::OutOfLine() {
   clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void AA::X::OutOfLine()$27$clang_analyzer_hashDump(5);$Category}}
 }
 
 void testLambda() {
   []() {
     clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void testLambda()::(anonymous class)::operator()() const$29$clang_analyzer_hashDump(5);$Category}}
   }();
 }
 
 template <typename T>
 void f(T) {
-  clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void f(double)$27$clang_analyzer_hashDump(5);$Category}}
-                               // expected-warning@-1{{debug.ExprInspection$void f(int)$27$clang_analyzer_hashDump(5);$Category}}
+  clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void f(T)$27$clang_analyzer_hashDump(5);$Category}}
 }
 
 template <typename T>
 struct TX {
   void f(T) {
-    clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TX<double>::f(double)$29$clang_analyzer_hashDump(5);$Category}}
-                                 // expected-warning@-1{{debug.ExprInspection$void TX<int>::f(int)$29$clang_analyzer_hashDump(5);$Category}}
+    clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TX::f(T)$29$clang_analyzer_hashDump(5);$Category}}
   }
 };
 
 template <>
 void f<long>(long) {
   clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void f(long)$27$clang_analyzer_hashDump(5);$Category}}
 }
 
 template <>
 struct TX<long> {
   void f(long) {
     clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TX<long>::f(long)$29$clang_analyzer_hashDump(5);$Category}}
   }
 };
 
 template <typename T>
 struct TTX {
   template<typename S>
   void f(T, S) {
-    clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TTX<int>::f(int, int)$29$clang_analyzer_hashDump(5);$Category}}
+    clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TTX::f(T, S)$29$clang_analyzer_hashDump(5);$Category}}
   }
 };
 
 void g() {
+  // TX<int> and TX<double> is instantiated from the same code with the same
+  // source locations. The same error happining in both of the instantiations
+  // should share the common hash. This means we should not include the
+  // template argument for these types in the function signature.
+  // Note that, we still want the hash to be different for explicit
+  // specializations.
   TX<int> x;
   TX<double> y;
   TX<long> xl;
   x.f(1);
   xl.f(1);
   f(5);
   f(3.0);
   y.f(2);
   TTX<int> z;
   z.f<int>(5, 5);
   f(5l);
 }

test/Analysis/edges-new.mm

 // CHECK-NEXT:      <string>Memory allocated by &apos;new[]&apos; should be deallocated by &apos;delete[]&apos;, not &apos;delete&apos;</string>
 // CHECK-NEXT:     </dict>
 // CHECK-NEXT:    </array>
 // CHECK-NEXT:    <key>description</key><string>Memory allocated by &apos;new[]&apos; should be deallocated by &apos;delete[]&apos;, not &apos;delete&apos;</string>
 // CHECK-NEXT:    <key>category</key><string>Memory error</string>
 // CHECK-NEXT:    <key>type</key><string>Bad deallocator</string>
 // CHECK-NEXT:    <key>check_name</key><string>unix.MismatchedDeallocator</string>
 // CHECK-NEXT:    <!-- This hash is experimental and going to change! -->
-// CHECK-NEXT:    <key>issue_hash_content_of_line_in_context</key><string>d9dbbf68db41ab74e2158f4b131abe34</string>
+// CHECK-NEXT:    <key>issue_hash_content_of_line_in_context</key><string>046c88d1c91ff46d6506dff5ff880756</string>
 // CHECK-NEXT:   <key>issue_hash_function_offset</key><string>0</string>
 // CHECK-NEXT:   <key>location</key>
 // CHECK-NEXT:   <dict>
 // CHECK-NEXT:    <key>line</key><integer>544</integer>
 // CHECK-NEXT:    <key>col</key><integer>19</integer>
 // CHECK-NEXT:    <key>file</key><integer>0</integer>
 // CHECK-NEXT:   </dict>
 // CHECK-NEXT:   </dict>