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

2017-10-30 Thread Phabricator via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL316900: [analyzer] Use the signature of the primary template 
for issue hash calculation (authored by xazax).

Changed prior to commit:
  https://reviews.llvm.org/D38728?vs=118788=120804#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38728

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/IssueHash.cpp
  cfe/trunk/test/Analysis/bug_hash_test.cpp
  cfe/trunk/test/Analysis/edges-new.mm


Index: cfe/trunk/test/Analysis/bug_hash_test.cpp
===
--- cfe/trunk/test/Analysis/bug_hash_test.cpp
+++ cfe/trunk/test/Analysis/bug_hash_test.cpp
@@ -71,15 +71,13 @@
 
 template 
 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 
 struct TX {
   void f(T) {
-clang_analyzer_hashDump(5); // expected-warning 
{{debug.ExprInspection$void 
TX::f(double)$29$clang_analyzer_hashDump(5);$Category}}
- // 
expected-warning@-1{{debug.ExprInspection$void 
TX::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}}
   }
 };
 
@@ -99,11 +97,17 @@
 struct TTX {
   template
   void f(T, S) {
-clang_analyzer_hashDump(5); // expected-warning 
{{debug.ExprInspection$void TTX::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 and TX 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 x;
   TX y;
   TX xl;
Index: cfe/trunk/test/Analysis/edges-new.mm
===
--- cfe/trunk/test/Analysis/edges-new.mm
+++ cfe/trunk/test/Analysis/edges-new.mm
@@ -20288,7 +20288,7 @@
 // CHECK-NEXT:typeBad deallocator
 // CHECK-NEXT:
check_nameunix.MismatchedDeallocator
 // CHECK-NEXT:
-// CHECK-NEXT:
issue_hash_content_of_line_in_contextd9dbbf68db41ab74e2158f4b131abe34
+// CHECK-NEXT:
issue_hash_content_of_line_in_context046c88d1c91ff46d6506dff5ff880756
 // CHECK-NEXT:   issue_hash_function_offset0
 // CHECK-NEXT:   location
 // CHECK-NEXT:   
Index: cfe/trunk/lib/StaticAnalyzer/Core/IssueHash.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/IssueHash.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/IssueHash.cpp
@@ -33,6 +33,13 @@
 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(Target) && !isa(Target) &&
   !isa(Target))
 Signature.append(Target->getReturnType().getAsString()).append(" ");


Index: cfe/trunk/test/Analysis/bug_hash_test.cpp
===
--- cfe/trunk/test/Analysis/bug_hash_test.cpp
+++ cfe/trunk/test/Analysis/bug_hash_test.cpp
@@ -71,15 +71,13 @@
 
 template 
 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 
 struct TX {
   void f(T) {
-clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TX::f(double)$29$clang_analyzer_hashDump(5);$Category}}
- // expected-warning@-1{{debug.ExprInspection$void TX::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}}
   }
 };
 
@@ -99,11 +97,17 @@
 struct TTX {
   template
   void f(T, S) {
-clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TTX::f(int,

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

2017-10-27 Thread Anna Zaks via Phabricator via cfe-commits 
zaks.anna accepted this revision.
zaks.anna added a comment.

LGTM!


https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-10-12 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun updated this revision to Diff 118788.
xazax.hun added a comment.

- Rebase based on the dependent revision and minor cleanups


https://reviews.llvm.org/D38728

Files:
  lib/StaticAnalyzer/Core/IssueHash.cpp
  test/Analysis/bug_hash_test.cpp
  test/Analysis/edges-new.mm


Index: test/Analysis/edges-new.mm
===
--- test/Analysis/edges-new.mm
+++ test/Analysis/edges-new.mm
@@ -20288,7 +20288,7 @@
 // CHECK-NEXT:typeBad deallocator
 // CHECK-NEXT:
check_nameunix.MismatchedDeallocator
 // CHECK-NEXT:
-// CHECK-NEXT:
issue_hash_content_of_line_in_contextd9dbbf68db41ab74e2158f4b131abe34
+// CHECK-NEXT:
issue_hash_content_of_line_in_context046c88d1c91ff46d6506dff5ff880756
 // CHECK-NEXT:   issue_hash_function_offset0
 // CHECK-NEXT:   location
 // CHECK-NEXT:   
Index: test/Analysis/bug_hash_test.cpp
===
--- test/Analysis/bug_hash_test.cpp
+++ test/Analysis/bug_hash_test.cpp
@@ -71,15 +71,13 @@
 
 template 
 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 
 struct TX {
   void f(T) {
-clang_analyzer_hashDump(5); // expected-warning 
{{debug.ExprInspection$void 
TX::f(double)$29$clang_analyzer_hashDump(5);$Category}}
- // 
expected-warning@-1{{debug.ExprInspection$void 
TX::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}}
   }
 };
 
@@ -99,11 +97,17 @@
 struct TTX {
   template
   void f(T, S) {
-clang_analyzer_hashDump(5); // expected-warning 
{{debug.ExprInspection$void TTX::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 and TX 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 x;
   TX y;
   TX xl;
Index: lib/StaticAnalyzer/Core/IssueHash.cpp
===
--- lib/StaticAnalyzer/Core/IssueHash.cpp
+++ lib/StaticAnalyzer/Core/IssueHash.cpp
@@ -33,6 +33,13 @@
 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(Target) && !isa(Target) &&
   !isa(Target))
 Signature.append(Target->getReturnType().getAsString()).append(" ");


Index: test/Analysis/edges-new.mm
===
--- test/Analysis/edges-new.mm
+++ test/Analysis/edges-new.mm
@@ -20288,7 +20288,7 @@
 // CHECK-NEXT:typeBad deallocator
 // CHECK-NEXT:check_nameunix.MismatchedDeallocator
 // CHECK-NEXT:
-// CHECK-NEXT:issue_hash_content_of_line_in_contextd9dbbf68db41ab74e2158f4b131abe34
+// CHECK-NEXT:issue_hash_content_of_line_in_context046c88d1c91ff46d6506dff5ff880756
 // CHECK-NEXT:   issue_hash_function_offset0
 // CHECK-NEXT:   location
 // CHECK-NEXT:   
Index: test/Analysis/bug_hash_test.cpp
===
--- test/Analysis/bug_hash_test.cpp
+++ test/Analysis/bug_hash_test.cpp
@@ -71,15 +71,13 @@
 
 template 
 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 
 struct TX {
   void f(T) {
-clang_analyzer_hashDump(5); // expected-warning {{debug.ExprInspection$void TX::f(double)$29$clang_analyzer_hashDump(5);$Category}}
- // expected-warning@-1{{debug.ExprInspection$void TX::f(int)$29$clang_analyzer_hashDump(5);$Category}}
+clang_analyzer_hashDump(5); // expected-warning

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

2017-10-12 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

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


https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-10-12 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

In https://reviews.llvm.org/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?


https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-10-12 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

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




Comment at: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:88
+::analyzerNumTimesReached)
+  .Case("clang_analyzer_hash_dump",
+::analyzerDumpHash)

`clang_analyzer_hashDump` would be more consistent.


https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-10-11 Thread Gabor Marton via Phabricator via cfe-commits 
martong added inline comments.



Comment at: test/Analysis/bug_hash_test.cpp:105
+void g() {
+  TX x;
+  TX xl;

As we discussed, the checking of the equality of the `IssueString` in case of 
`TX` and `TX` 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.


https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-10-11 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Core/IssueHash.cpp:39
+  // primary template.
+  if (const FunctionDecl *InstantiatedFrom =
+  Target->getInstantiatedFromMemberFunction())

martong wrote:
> Could we use here FunctionDecl::getPrimaryTemplate() ? That seems more 
> general, it handles both specializations and instantiations.
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.



Comment at: test/Analysis/bug_hash_test.cpp:61
 
-// CHECK: diagnostics
+template 
+T f(T i) {

martong wrote:
> We could add a few more test cases:
> - a function template in class template
> - specializations vs instantiations
> - the combination of the above two (?)
> 
Good point.



Comment at: test/Analysis/bug_hash_test.cpp:1363
 // CHECK-NEXT:  
+// CHECK-NEXT:  
+// CHECK-NEXT:   path

martong wrote:
> 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
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. 


https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2017-10-10 Thread Gabor Marton via Phabricator via cfe-commits 
martong added inline comments.



Comment at: lib/StaticAnalyzer/Core/IssueHash.cpp:39
+  // primary template.
+  if (const FunctionDecl *InstantiatedFrom =
+  Target->getInstantiatedFromMemberFunction())

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



Comment at: test/Analysis/bug_hash_test.cpp:61
 
-// CHECK: diagnostics
+template 
+T f(T i) {

We could add a few more test cases:
- a function template in class template
- specializations vs instantiations
- the combination of the above two (?)




Comment at: test/Analysis/bug_hash_test.cpp:1363
 // CHECK-NEXT:  
+// CHECK-NEXT:  
+// CHECK-NEXT:   path

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


Repository:
  rL LLVM

https://reviews.llvm.org/D38728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits