[PATCH] D48474: [analyzer][ctu] fix unsortable diagnostics

[Rafael Stahl via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC336275: [analyzer][ctu] fix unsortable diagnostics (authored 
by r.stahl, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48474?vs=152433=154107#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48474

Files:
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/externalFnMap.txt
  test/Analysis/ctu-hdr.h
  test/Analysis/ctu-main.cpp


Index: test/Analysis/ctu-hdr.h
===
--- test/Analysis/ctu-hdr.h
+++ test/Analysis/ctu-hdr.h
@@ -0,0 +1,3 @@
+#define MACRODIAG() clang_analyzer_warnIfReached()
+
+void clang_analyzer_warnIfReached();
Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -4,6 +4,8 @@
 // RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir 
-verify %s
 
+#include "ctu-hdr.h"
+
 void clang_analyzer_eval(int);
 
 int f(int);
@@ -41,6 +43,7 @@
 }
 
 int fun_using_anon_struct(int);
+int other_macro_diag(int);
 
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
@@ -58,4 +61,8 @@
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // 
expected-warning{{TRUE}}
+
+  clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
+  // expected-warning@Inputs/ctu-other.cpp:75{{REACHABLE}}
+  MACRODIAG(); // expected-warning{{REACHABLE}}
 }
Index: test/Analysis/Inputs/externalFnMap.txt
===
--- test/Analysis/Inputs/externalFnMap.txt
+++ test/Analysis/Inputs/externalFnMap.txt
@@ -12,3 +12,4 @@
 c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
 c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
 c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
+c:@F@other_macro_diag#I# ctu-other.cpp.ast
Index: test/Analysis/Inputs/ctu-other.cpp
===
--- test/Analysis/Inputs/ctu-other.cpp
+++ test/Analysis/Inputs/ctu-other.cpp
@@ -1,3 +1,5 @@
+#include "../ctu-hdr.h"
+
 int callback_to_main(int x);
 int f(int x) {
   return x - 1;
@@ -68,3 +70,8 @@
 
 typedef struct { int n; } Anonymous;
 int fun_using_anon_struct(int n) { Anonymous anon; anon.n = n; return anon.n; }
+
+int other_macro_diag(int x) {
+  MACRODIAG();
+  return x;
+}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -406,11 +406,15 @@
   std::pair InSameTU = SM.isInTheSameTranslationUnit(XOffs, YOffs);
   if (InSameTU.first)
 return XL.isBeforeInTranslationUnitThan(YL);
-  const FileEntry *XFE = SM.getFileEntryForID(XL.getFileID());
-  const FileEntry *YFE = SM.getFileEntryForID(YL.getFileID());
+  const FileEntry *XFE = SM.getFileEntryForID(XL.getSpellingLoc().getFileID());
+  const FileEntry *YFE = SM.getFileEntryForID(YL.getSpellingLoc().getFileID());
   if (!XFE || !YFE)
 return XFE && !YFE;
-  return XFE->getName() < YFE->getName();
+  int NameCmp = XFE->getName().compare(YFE->getName());
+  if (NameCmp != 0)
+return NameCmp == -1;
+  // Last resort: Compare raw file IDs that are possibly expansions.
+  return XL.getFileID() < YL.getFileID();
 }
 
 static bool compare(const PathDiagnostic , const PathDiagnostic ) {


Index: test/Analysis/ctu-hdr.h
===
--- test/Analysis/ctu-hdr.h
+++ test/Analysis/ctu-hdr.h
@@ -0,0 +1,3 @@
+#define MACRODIAG() clang_analyzer_warnIfReached()
+
+void clang_analyzer_warnIfReached();
Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -4,6 +4,8 @@
 // RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir -verify %s
 
+#include "ctu-hdr.h"
+
 void clang_analyzer_eval(int);
 
 int f(int);
@@ -41,6 +43,7 @@
 }
 
 int fun_using_anon_struct(int);
+int other_macro_diag(int);
 
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
@@ -58,4 +61,8 @@
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
+  // 

[PATCH] D48474: [analyzer][ctu] fix unsortable diagnostics

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov accepted this revision.
george.karpenkov added a comment.
This revision is now accepted and ready to land.

> My fear of using file IDs is that I don't know whether they are stable

IIRC they are currently stable, but that's relying on implementation details.
Still better than not differentiating at all.


Repository:
  rC Clang

https://reviews.llvm.org/D48474



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

[PATCH] D48474: [analyzer][ctu] fix unsortable diagnostics

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

In https://reviews.llvm.org/D30691#879838, @xazax.hun wrote:

> In https://reviews.llvm.org/D30691#878830, @r.stahl wrote:
>
> > For my purposes I replaced the return statement of the 
> > compareCrossTUSourceLocs function with:
> >
> >   return XL.getFileID() < YL.getFileID();
> >
> >
> > A more correct fix would create only one unique diagnostic for both cases.
>
>
> Thank you for the report, I could reproduce this after modifying the null 
> dereference checker. My fear of using file IDs is that I don't know whether 
> they are stable. So in subsequent runs, different paths might be chosen by 
> the analyzer and this could be confusing to the user.  I will think about a 
> workaround that both stable and solves this assertion.
>
> I see two possible ways to do the proper fix. One is to check explicitly for 
> this case when the same function appears in multiple translation units. A 
> better approach would be to have the ASTImporter handle this case. I think 
> the proper fix is better addressed in a separate patch.


Here is my improper fix with test case. Since CTU is still an experimental 
feature and this is a rare case to encounter I believe it's okay to risk 
confusing the user, rather than keep it broken.

Unfortunately I will not have the time to work on a proper fix.


Repository:
  rC Clang

https://reviews.llvm.org/D48474



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

[PATCH] D48474: [analyzer][ctu] fix unsortable diagnostics

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl created this revision.
r.stahl added reviewers: xazax.hun, NoQ, dcoughlin.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, rnkovacs, 
szepet.
Herald added a reviewer: george.karpenkov.

In the provided test case the PathDiagnostic compare function was not able to 
find a difference.


Repository:
  rC Clang

https://reviews.llvm.org/D48474

Files:
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/externalFnMap.txt
  test/Analysis/ctu-hdr.h
  test/Analysis/ctu-main.cpp


Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -4,6 +4,8 @@
 // RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir 
-verify %s
 
+#include "ctu-hdr.h"
+
 void clang_analyzer_eval(int);
 
 int f(int);
@@ -41,6 +43,7 @@
 }
 
 int fun_using_anon_struct(int);
+int other_macro_diag(int);
 
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
@@ -58,4 +61,8 @@
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // 
expected-warning{{TRUE}}
+
+  clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
+  // expected-warning@Inputs/ctu-other.cpp:75{{REACHABLE}}
+  MACRODIAG(); // expected-warning{{REACHABLE}}
 }
Index: test/Analysis/ctu-hdr.h
===
--- /dev/null
+++ test/Analysis/ctu-hdr.h
@@ -0,0 +1,3 @@
+#define MACRODIAG() clang_analyzer_warnIfReached()
+
+void clang_analyzer_warnIfReached();
Index: test/Analysis/Inputs/externalFnMap.txt
===
--- test/Analysis/Inputs/externalFnMap.txt
+++ test/Analysis/Inputs/externalFnMap.txt
@@ -12,3 +12,4 @@
 c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
 c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
 c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
+c:@F@other_macro_diag#I# ctu-other.cpp.ast
Index: test/Analysis/Inputs/ctu-other.cpp
===
--- test/Analysis/Inputs/ctu-other.cpp
+++ test/Analysis/Inputs/ctu-other.cpp
@@ -1,3 +1,5 @@
+#include "../ctu-hdr.h"
+
 int callback_to_main(int x);
 int f(int x) {
   return x - 1;
@@ -68,3 +70,8 @@
 
 typedef struct { int n; } Anonymous;
 int fun_using_anon_struct(int n) { Anonymous anon; anon.n = n; return anon.n; }
+
+int other_macro_diag(int x) {
+  MACRODIAG();
+  return x;
+}
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -406,11 +406,15 @@
   std::pair InSameTU = SM.isInTheSameTranslationUnit(XOffs, YOffs);
   if (InSameTU.first)
 return XL.isBeforeInTranslationUnitThan(YL);
-  const FileEntry *XFE = SM.getFileEntryForID(XL.getFileID());
-  const FileEntry *YFE = SM.getFileEntryForID(YL.getFileID());
+  const FileEntry *XFE = SM.getFileEntryForID(XL.getSpellingLoc().getFileID());
+  const FileEntry *YFE = SM.getFileEntryForID(YL.getSpellingLoc().getFileID());
   if (!XFE || !YFE)
 return XFE && !YFE;
-  return XFE->getName() < YFE->getName();
+  int NameCmp = XFE->getName().compare(YFE->getName());
+  if (NameCmp != 0)
+return NameCmp == -1;
+  // Last resort: Compare raw file IDs that are possibly expansions.
+  return XL.getFileID() < YL.getFileID();
 }
 
 static bool compare(const PathDiagnostic , const PathDiagnostic ) {


Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -4,6 +4,8 @@
 // RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
 // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir -verify %s
 
+#include "ctu-hdr.h"
+
 void clang_analyzer_eval(int);
 
 int f(int);
@@ -41,6 +43,7 @@
 }
 
 int fun_using_anon_struct(int);
+int other_macro_diag(int);
 
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
@@ -58,4 +61,8 @@
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
+  // expected-warning@Inputs/ctu-other.cpp:75{{REACHABLE}}
+  MACRODIAG(); // expected-warning{{REACHABLE}}
 }
Index: test/Analysis/ctu-hdr.h
===
--- /dev/null
+++