[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros

2019-03-08 Thread Kristóf Umann via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL355705: [analyzer] Fix infinite recursion in printing macros 
(authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57891?vs=189670=189863#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57891

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp

Index: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -22,6 +22,7 @@
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 
@@ -776,10 +777,20 @@
 /// As we expand the last line, we'll immediately replace PRINT(str) with
 /// print(x). The information that both 'str' and 'x' refers to the same string
 /// is an information we have to forward, hence the argument \p PrevArgs.
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap );
+///
+/// To avoid infinite recursion we maintain the already processed tokens in
+/// a set. This is carried as a parameter through the recursive calls. The set
+/// is extended with the currently processed token and after processing it, the
+/// token is removed. If the token is already in the set, then recursion stops:
+///
+/// #define f(y) x
+/// #define x f(x)
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const MacroArgMap ,
+llvm::SmallPtrSet );
 
 /// Retrieves the name of the macro and what it's arguments expand into
 /// at \p ExpanLoc.
@@ -828,19 +839,35 @@
   llvm::SmallString<200> ExpansionBuf;
   llvm::raw_svector_ostream OS(ExpansionBuf);
   TokenPrinter Printer(OS, PP);
+  llvm::SmallPtrSet AlreadyProcessedTokens;
+
   std::string MacroName =
-getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{});
+getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{},
+ AlreadyProcessedTokens);
   return { MacroName, OS.str() };
 }
 
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap ) {
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const MacroArgMap ,
+llvm::SmallPtrSet ) {
 
   const SourceManager  = PP.getSourceManager();
 
   MacroNameAndArgs Info = getMacroNameAndArgs(SM.getExpansionLoc(MacroLoc), PP);
+  IdentifierInfo* IDInfo = PP.getIdentifierInfo(Info.Name);
+
+  // TODO: If the macro definition contains another symbol then this function is
+  // called recursively. In case this symbol is the one being defined, it will
+  // be an infinite recursion which is stopped by this "if" statement. However,
+  // in this case we don't get the full expansion text in the Plist file. See
+  // the test file where "value" is expanded to "garbage_" instead of
+  // "garbage_value".
+  if (AlreadyProcessedTokens.find(IDInfo) != AlreadyProcessedTokens.end())
+return Info.Name;
+  AlreadyProcessedTokens.insert(IDInfo);
 
   if (!Info.MI)
 return Info.Name;
@@ -867,7 +894,8 @@
 // macro.
 if (const MacroInfo *MI =
  getMacroInfoForLocation(PP, SM, II, T.getLocation())) {
-  getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args);
+  getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args,
+AlreadyProcessedTokens);
 
   // If this is a function-like macro, skip its arguments, as
   // getExpandedMacro() already printed them. If this is the case, let's
@@ -899,7 +927,7 @@
 }
 
 getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
-  Info.Args);
+  Info.Args, AlreadyProcessedTokens);
 if (MI->getNumParams() != 0)
   ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
   }
@@ -911,6 +939,8 @@

[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros

2019-03-07 Thread 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.

Well, yea, getting rid of a crash is great, it's kind of bummer that can't 
expand the macro properly in the testfile. I really fear that we need a far 
greater overhaul of this entire effort to support them properly -- which should 
be the task of another patch.

Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D57891



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

[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros

2019-03-07 Thread Tibor Brunner via Phabricator via cfe-commits 
bruntib updated this revision to Diff 189670.
bruntib added a comment.
Herald added subscribers: Charusso, jdoerfert, whisperity.

I added a test case for this recursive case. There is also a TODO in the code 
indicating the place where an additional fix will be required.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57891

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  test/Analysis/plist-macros-with-expansion.cpp

Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -440,3 +440,14 @@
 }
 // CHECK: nameYET_ANOTHER_SET_TO_NULL
 // CHECK-NEXT: expansionprint((void *)5); print((void *)Remember the Vasa); ptr = nullptr;
+
+int garbage_value;
+
+#define REC_MACRO_FUNC(REC_MACRO_PARAM) garbage_##REC_MACRO_PARAM
+#define value REC_MACRO_FUNC(value)
+
+void recursiveMacroUser() {
+  if (value == 0)
+1 / value; // expected-warning{{Division by zero}}
+   // expected-warning@-1{{expression result unused}}
+}
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5443,6 +5443,140 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line450
+   col3
+   file0
+  
+  
+   line450
+   col4
+   file0
+  
+ 
+end
+ 
+  
+   line450
+   col7
+   file0
+  
+  
+   line450
+   col11
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line450
+  col7
+  file0
+ 
+ ranges
+ 
+   
+
+ line450
+ col7
+ file0
+
+
+ line450
+ col16
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Assuming garbage_value is equal to 0
+ message
+ Assuming garbage_value is equal to 0
+
+
+ kindevent
+ location
+ 
+  line451
+  col7
+  file0
+ 
+ ranges
+ 
+   
+
+ line451
+ col5
+ file0
+
+
+ line451
+ col13
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line450
+  col7
+  file0
+ 
+ namevalue
+ expansiongarbage_
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context1f3c94860e67b6b863e956bd67e49f1d
+  issue_context_kindfunction
+  issue_contextrecursiveMacroUser
+  issue_hash_function_offset2
+  location
+  
+   line451
+   col7
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+449
+450
+451
+   
+  
+  
  
  files
  
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -22,6 +22,7 @@
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 
@@ -776,10 +777,20 @@
 /// As we expand the last line, we'll immediately replace PRINT(str) with
 /// print(x). The information that both 'str' and 'x' refers to the same string
 /// is an information we have to forward, hence the argument \p PrevArgs.
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap );
+///
+/// To avoid infinite recursion we maintain the already processed tokens in
+/// a set. This is carried as a parameter through the recursive calls. The set
+/// is extended with the currently processed token and after processing it, the
+/// token is removed. If the token is already in the set, then recursion stops:
+///
+/// #define f(y) x
+/// #define x f(x)
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const

[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros

2019-02-07 Thread Kristóf Umann via Phabricator via cfe-commits 
Szelethus added a comment.

Is there a chance of obtaining a testcase for this? I would very strongly 
prefer one.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57891



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

[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros

2019-02-07 Thread Tibor Brunner via Phabricator via cfe-commits 
bruntib created this revision.
bruntib added reviewers: NoQ, george.karpenkov, Szelethus, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: clang.

#define f(y) x
#define x f(x)
int main() { x; }

This example results a compilation error since "x" in the first line was not 
defined earlier. However, the macro expression printer goes to an infinite 
recursion on this example.


Repository:
  rC Clang

https://reviews.llvm.org/D57891

Files:
  lib/StaticAnalyzer/Core/PlistDiagnostics.cpp


Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -23,6 +23,7 @@
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 
@@ -734,10 +735,20 @@
 /// As we expand the last line, we'll immediately replace PRINT(str) with
 /// print(x). The information that both 'str' and 'x' refers to the same string
 /// is an information we have to forward, hence the argument \p PrevArgs.
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap );
+///
+/// To avoid infinite recursion we maintain the already processed tokens in
+/// a set. This is carried as a parameter through the recursive calls. The set
+/// is extended with the currently processed token and after processing it, the
+/// token is removed. If the token is already in the set, then recursion stops:
+///
+/// #define f(y) x
+/// #define x f(x)
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const MacroArgMap ,
+llvm::SmallPtrSet );
 
 /// Retrieves the name of the macro and what it's arguments expand into
 /// at \p ExpanLoc.
@@ -786,19 +797,28 @@
   llvm::SmallString<200> ExpansionBuf;
   llvm::raw_svector_ostream OS(ExpansionBuf);
   TokenPrinter Printer(OS, PP);
+  llvm::SmallPtrSet AlreadyProcessedTokens;
 
-  return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{}),
+  return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{},
+ AlreadyProcessedTokens),
OS.str() };
 }
 
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap ) {
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const MacroArgMap ,
+llvm::SmallPtrSet ) {
 
   const SourceManager  = PP.getSourceManager();
 
   MacroNameAndArgs Info = getMacroNameAndArgs(SM.getExpansionLoc(MacroLoc), 
PP);
+  IdentifierInfo* IDInfo = PP.getIdentifierInfo(Info.Name);
+
+  if (AlreadyProcessedTokens.find(IDInfo) != AlreadyProcessedTokens.end())
+return Info.Name;
+  AlreadyProcessedTokens.insert(IDInfo);
 
   // Manually expand its arguments from the previous macro.
   Info.Args.expandFromPrevMacro(PrevArgs);
@@ -822,7 +842,8 @@
 // macro.
 if (const MacroInfo *MI =
  getMacroInfoForLocation(PP, SM, II, T.getLocation())) 
{
-  getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args);
+  getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args,
+AlreadyProcessedTokens);
 
   // If this is a function-like macro, skip its arguments, as
   // getExpandedMacro() already printed them. If this is the case, let's
@@ -854,7 +875,7 @@
 }
 
 getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
-  Info.Args);
+  Info.Args, AlreadyProcessedTokens);
 if (MI->getNumParams() != 0)
   ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
   }
@@ -866,6 +887,8 @@
 Printer.printToken(T);
   }
 
+  AlreadyProcessedTokens.erase(IDInfo);
+
   return Info.Name;
 }
 


Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -23,6 +23,7 @@
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/Statistic.h"
+#include