[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC359037: [analyzer] Fix macro names in diagnostics within 
bigger macros. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59121?vs=192917&id=196329#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121

Files:
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  test/Analysis/diagnostics/macros.cpp

Index: test/Analysis/diagnostics/macros.cpp
===
--- test/Analysis/diagnostics/macros.cpp
+++ test/Analysis/diagnostics/macros.cpp
@@ -3,7 +3,7 @@
 #include "../Inputs/system-header-simulator.h"
 #include "../Inputs/system-header-simulator-cxx.h"
 
-void testIntMacro(unsigned int i) {
+void testUnsignedIntMacro(unsigned int i) {
   if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
  // expected-note@-1 {{Taking true branch}}
 char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
@@ -12,6 +12,20 @@
   }
 }
 
+
+// FIXME: 'i' can never be equal to UINT32_MAX - it doesn't even fit into its
+// type ('int'). This should say "Assuming 'i' is equal to -1".
+void testIntMacro(int i) {
+  if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
+ // expected-note@-1 {{Taking true branch}}
+char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
+*p = 7;  // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+ // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+
+
 void testNULLMacro(int *p) {
   if (p == NULL) { // expected-note {{Assuming 'p' is equal to NULL}}
// expected-note@-1 {{Taking true branch}}
@@ -47,3 +61,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2002,43 +2002,22 @@
   const Expr *OriginalExpr = Ex;
   Ex = Ex->IgnoreParenCasts();
 
-  // Use heuristics to determine if Ex is a macro expending to a literal and
-  // if so, use the macro's name.
-  SourceLocation LocStart = Ex->getBeginLoc();
-  SourceLocation LocEnd = Ex->getEndLoc();
-  if (LocStart.isMacroID() && LocEnd.isMacroID() &&
-  (isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex))) {
-StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
-
-bool partOfParentMacro = false;
-if (ParentEx->getBeginLoc().isMacroID()) {
-  StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
-  ParentEx->getBeginLoc(), BRC.getSourceManager(),
-  BRC.getASTContext().getLangOpts());
-  partOfParentMacro = PName.equals(StartName);
-}
-
-if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
-  // Get the location of the macro name as written by the caller.
-  SourceLocation Loc = LocStart;
-  while (LocStart.isMacroID()) {
-Loc = LocStart;
-LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+  if (isa(Ex) || isa(Ex) ||
+  isa(Ex) || isa(Ex) ||
+  isa(Ex)) {
+// Use heuristics to determine if the expression is a macro
+// expanding to a literal and if so, use the macro's name.
+SourceLocation BeginLoc = OriginalExpr->getBeginLoc();
+SourceLocation EndLoc = OriginalExpr->getEndLoc();
+if (BeginLoc.isMacroID() && EndLoc.isMacroID()) {
+  SourceManager &SM = BRC.getSourceManager();
+  const LangOptions &LO = BRC.getASTContext().getLangOpts();
+  if (Lexer::isAtStartOfMacroExpansion(BeginLoc, SM, LO) &&
+  Lexer::isAtEndOfMacroExpansion(EndLoc, SM, LO)) {
+CharSourceRange R = Lexer::getAsCharRange({BeginLoc, EndLoc}, SM, LO);
+Out << Lexe

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Kristóf Umann via Phabricator via cfe-commits]

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

LGTM! Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.

Nice catch!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 192917.
NoQ added a comment.

Also, this is kinda weird. According to my logic, we should have written 
`Assuming 'i' is equal to 4294967295` because that's what the user will see in 
the macro popup. However, that's incorrect for the same reason: `i` is an int, 
while `4294967295` doesn't fit into an int, so they can never be equal. Writing 
`Assuming 'i' is equal to UINT32_MAX` is incorrect for the same reason, and the 
current message is the only one that's technically the truth.

Writing what exactly we're assuming is, in my opinion, more important than 
repeating what's literally written in the code.

Added a FIXME test to document this problem.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/macros.cpp

Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -3,7 +3,7 @@
 #include "../Inputs/system-header-simulator.h"
 #include "../Inputs/system-header-simulator-cxx.h"
 
-void testIntMacro(unsigned int i) {
+void testUnsignedIntMacro(unsigned int i) {
   if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
  // expected-note@-1 {{Taking true branch}}
 char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
@@ -12,6 +12,20 @@
   }
 }
 
+
+// FIXME: 'i' can never be equal to UINT32_MAX - it doesn't even fit into its
+// type ('int'). This should say "Assuming 'i' is equal to -1".
+void testIntMacro(int i) {
+  if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
+ // expected-note@-1 {{Taking true branch}}
+char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
+*p = 7;  // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+ // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+  }
+}
+
+
+
 void testNULLMacro(int *p) {
   if (p == NULL) { // expected-note {{Assuming 'p' is equal to NULL}}
// expected-note@-1 {{Taking true branch}}
@@ -47,3 +61,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1969,43 +1969,22 @@
   const Expr *OriginalExpr = Ex;
   Ex = Ex->IgnoreParenCasts();
 
-  // Use heuristics to determine if Ex is a macro expending to a literal and
-  // if so, use the macro's name.
-  SourceLocation LocStart = Ex->getBeginLoc();
-  SourceLocation LocEnd = Ex->getEndLoc();
-  if (LocStart.isMacroID() && LocEnd.isMacroID() &&
-  (isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex))) {
-StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
-
-bool partOfParentMacro = false;
-if (ParentEx->getBeginLoc().isMacroID()) {
-  StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
-  ParentEx->getBeginLoc(), BRC.getSourceManager(),
-  BRC.getASTContext().getLangOpts());
-  partOfParentMacro = PName.equals(StartName);
-}
-
-if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
-  // Get the location of the macro name as written by the caller.
-  SourceLocation Loc = LocStart;
-  while (LocStart.isMacroID()) {
-Loc = LocStart;
-LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+  if (isa(Ex) || isa(Ex) ||
+  isa(Ex) || isa(Ex) ||
+  isa(Ex)) {
+// Use heuristics to determine if the expression is a macro
+// expanding to a literal and if so, use the macro's name.
+SourceLocation BeginLoc = OriginalExpr->getBeginLoc();
+SourceLocation EndLoc

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D59121#1448386 , @NoQ wrote:

> Mmm, that *is* an `Assuming...` piece, i.e., this is the same code, just the 
> structure of macros is more complicated than usual.


You told me we would like to see a value when we hover over a name, which is 
cool. If I think about C and they do not C# over the complicated macros, I 
would like to C names of macros. It is up to you, but your first intention 
working with my code very well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Mmm, that *is* an `Assuming...` piece, i.e., this is the same code, just the 
structure of macros is more complicated than usual.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D59121#1448367 , @NoQ wrote:

> On second thought, dunno. In the scan-build macro preview it wouldn't show 
> you UINT32_MAX anyway. Maybe let's keep this behavior.
>
> Cleaned up the patch a little bit.


Somehow on the `Assuming ...` pieces we write out the macro name, and my little 
code did that too. We could obtain the value but I think your first intention 
is the correct behaviour. Why are you not using my working example?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 192906.
NoQ added a comment.

On second thought, dunno. In the scan-build macro preview it wouldn't show you 
UINT32_MAX anyway. Maybe let's keep this behavior.

Cleaned up the patch a little bit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/macros.cpp


Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -47,3 +47,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded 
from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from 
variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from 
variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1969,43 +1969,22 @@
   const Expr *OriginalExpr = Ex;
   Ex = Ex->IgnoreParenCasts();
 
-  // Use heuristics to determine if Ex is a macro expending to a literal and
-  // if so, use the macro's name.
-  SourceLocation LocStart = Ex->getBeginLoc();
-  SourceLocation LocEnd = Ex->getEndLoc();
-  if (LocStart.isMacroID() && LocEnd.isMacroID() &&
-  (isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex))) {
-StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
-
-bool partOfParentMacro = false;
-if (ParentEx->getBeginLoc().isMacroID()) {
-  StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
-  ParentEx->getBeginLoc(), BRC.getSourceManager(),
-  BRC.getASTContext().getLangOpts());
-  partOfParentMacro = PName.equals(StartName);
-}
-
-if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
-  // Get the location of the macro name as written by the caller.
-  SourceLocation Loc = LocStart;
-  while (LocStart.isMacroID()) {
-Loc = LocStart;
-LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+  if (isa(Ex) || isa(Ex) ||
+  isa(Ex) || isa(Ex) ||
+  isa(Ex)) {
+// Use heuristics to determine if the expression is a macro
+// expanding to a literal and if so, use the macro's name.
+SourceLocation BeginLoc = OriginalExpr->getBeginLoc();
+SourceLocation EndLoc = OriginalExpr->getEndLoc();
+if (BeginLoc.isMacroID() && EndLoc.isMacroID()) {
+  SourceManager &SM = BRC.getSourceManager();
+  const LangOptions &LO = BRC.getASTContext().getLangOpts();
+  if (Lexer::isAtStartOfMacroExpansion(BeginLoc, SM, LO) &&
+  Lexer::isAtEndOfMacroExpansion(EndLoc, SM, LO)) {
+CharSourceRange R = Lexer::getAsCharRange({BeginLoc, EndLoc}, SM, LO);
+Out << Lexer::getSourceText(R, SM, LO);
+return false;
   }
-  StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
-Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-
-  // Return the macro name.
-  Out << MacroName;
-  return false;
 }
   }
 


Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -47,3 +47,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think the right thing to do here is "look at the immediate macro; while it 
expands exactly to our original expression, look at what it is an expansion of; 
write down the last macro we've reached". My code now gives up whenever we stop 
expanding to the original expression, but it should write down the highest 
macro it has reached instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 192765.
NoQ added a comment.

An elegant solution for a more civilized age. Unfortunately, doesn't preserve 
the `UINT32_MAX` macro in the newly added test - i'll try a bit harder to 
preserve it. Relies on D59977  to work.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/macros.cpp


Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -47,3 +47,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded 
from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from 
variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from 
variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1969,43 +1969,22 @@
   const Expr *OriginalExpr = Ex;
   Ex = Ex->IgnoreParenCasts();
 
-  // Use heuristics to determine if Ex is a macro expending to a literal and
-  // if so, use the macro's name.
-  SourceLocation LocStart = Ex->getBeginLoc();
-  SourceLocation LocEnd = Ex->getEndLoc();
-  if (LocStart.isMacroID() && LocEnd.isMacroID() &&
-  (isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex) ||
-   isa(Ex))) {
-StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
-  BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
-
-bool partOfParentMacro = false;
-if (ParentEx->getBeginLoc().isMacroID()) {
-  StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
-  ParentEx->getBeginLoc(), BRC.getSourceManager(),
-  BRC.getASTContext().getLangOpts());
-  partOfParentMacro = PName.equals(StartName);
-}
-
-if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
-  // Get the location of the macro name as written by the caller.
-  SourceLocation Loc = LocStart;
-  while (LocStart.isMacroID()) {
-Loc = LocStart;
-LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+  if (isa(Ex) || isa(Ex) ||
+  isa(Ex) || isa(Ex) ||
+  isa(Ex)) {
+// Use heuristics to determine if the expression is a macro
+// expanding to a literal and if so, use the macro's name.
+SourceLocation LocStart = OriginalExpr->getBeginLoc();
+SourceLocation LocEnd = OriginalExpr->getEndLoc();
+if (LocStart.isMacroID() && LocEnd.isMacroID()) {
+  SourceManager &SM = BRC.getSourceManager();
+  const LangOptions &LO = BRC.getASTContext().getLangOpts();
+  if (Lexer::isAtStartOfMacroExpansion(LocStart, SM, LO) &&
+  Lexer::isAtEndOfMacroExpansion(LocEnd, SM, LO)) {
+SourceRange R{LocStart, LocEnd};
+Out << Lexer::getSourceText(Lexer::getAsCharRange(R, SM, LO), SM, LO);
+return false;
   }
-  StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
-Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-
-  // Return the macro name.
-  Out << MacroName;
-  return false;
 }
   }
 


Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -47,3 +47,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}
Index: clang/lib/Static

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso requested changes to this revision.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1985
 bool partOfParentMacro = false;
+StringRef PName = "";
 if (ParentEx->getBeginLoc().isMacroID()) {

`ParentName`?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1996
+  StringRef MacroName = StartName;
+  while (true) {
 LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);

I was never cool in algorithms, but I think it is over-complicated. In Hungary 
we are about to build a new nuclear power plant so here it is really 
emphasized: never-ever create an infinite loop.
What about the following?:
```
  while (LocStart.isMacroID()) {
 
StringRef CurrentMacroName = 
Lexer::getImmediateMacroNameForDiagnostics( 
LocStart, BRC.getSourceManager(),   
 
BRC.getASTContext().getLangOpts()); 
 
LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart); 
 
if (CurrentMacroName != ParentName) 
  
  MacroName = CurrentMacroName; 
 
  }
```



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2003
+LocStart, BRC.getSourceManager(),
+BRC.getASTContext().getLangOpts());
+if (NextMacroName == PName)

There is too much duplication and it is difficult to understand what is going 
on. May here is the time for deduplicating?
Please note that, there is a function called `getMacroName(SourceLocation, 
BugReporterContext &)`, may you would put your own helper-function above that.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Uhh, it's such a chore to work on these things -- You can really feel that the 
preprocessor must've been the first thing implemented in clang. Unfortunately, 
you really have to counterweight it's shortcomings with excessive amount of 
comments. I can see an infinite loop and a lot of string comparisons, but 
someone without having this particular test case as a context, it's very hard 
to follow what's happening.

Could you please add some code examples in the comments too?




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1997-1999
 LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+if (!LocStart.isMacroID())
+  break;

> Gets the location of the immediate macro caller, one level up the stack 
> toward the initial macro typed into the source.
Hmm, is there a guarantee that at the end of the stack is not a macro location?



Comment at: clang/test/Analysis/diagnostics/macros.cpp:53
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to 
UINT32_MAX}}
+// expected-note@-1 {{Taking false branch}}

Ah, that looks pretty :D


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59121/new/

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, 
mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy, 
a.sidorin, szepet.
Herald added a project: clang.

On the newly included test case the previous behavior was

  Line 53: note: Assuming 'i' is equal to nested_null_split

which is meh.

I tried to make this piece of logic slightly more correct, but it's most likely 
still completely incorrect.


Repository:
  rC Clang

https://reviews.llvm.org/D59121

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/macros.cpp


Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -46,3 +46,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded 
from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to 
UINT32_MAX}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from 
variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from 
variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1982,8 +1982,9 @@
 bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
 
 bool partOfParentMacro = false;
+StringRef PName = "";
 if (ParentEx->getBeginLoc().isMacroID()) {
-  StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
+  PName = Lexer::getImmediateMacroNameForDiagnostics(
   ParentEx->getBeginLoc(), BRC.getSourceManager(),
   BRC.getASTContext().getLangOpts());
   partOfParentMacro = PName.equals(StartName);
@@ -1991,13 +1992,20 @@
 
 if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
   // Get the location of the macro name as written by the caller.
-  SourceLocation Loc = LocStart;
-  while (LocStart.isMacroID()) {
-Loc = LocStart;
+  StringRef MacroName = StartName;
+  while (true) {
 LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+if (!LocStart.isMacroID())
+  break;
+
+StringRef NextMacroName = Lexer::getImmediateMacroNameForDiagnostics(
+LocStart, BRC.getSourceManager(),
+BRC.getASTContext().getLangOpts());
+if (NextMacroName == PName)
+  break;
+
+MacroName = NextMacroName;
   }
-  StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
-Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
 
   // Return the macro name.
   Out << MacroName;


Index: clang/test/Analysis/diagnostics/macros.cpp
===
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -46,3 +46,14 @@
 // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }
 }
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+  nested_null_split(i); // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
+// expected-note@-1 {{Taking false branch}}
+  if (!p) // expected-note {{Assuming 'p' is null}}
+  // expected-note@-1 {{Taking true branch}}
+*p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+// expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1982,8 +1982,9 @@
 bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
 
 bool partOfParentMacro = false;
+StringRef PName = "";
 if (ParentEx->getBeginLoc().isMacroID()) {
-  StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
+  PName = Lexer::getImmediateMacroNameForDiagnostics(
   ParentEx->getBeginLoc(), BRC.getSourceManager(),
   BRC.getASTContext().getLangOpts());
   partOfParentMacro = PName.equals(StartName);
@@ -1991,13 +1992,20 @@
 
 if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
   // Get the locatio