From b4cdfe8e7fe6d04a9e5e9cb574314367e3c3ba6d Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Fri, 8 Mar 2019 16:26:29 +0000 Subject: [PATCH] [analyzer] Fix infinite recursion in printing macros In the commited testfile, macro expansion (the one implemented for the plist output) runs into an infinite recursion. The issue originates from the algorithm being faulty, as in #define value REC_MACRO_FUNC(value) the "value" is being (or at least attempted) expanded from the same macro. The solved this issue by gathering already visited macros in a set, which does resolve the crash, but will result in an incorrect macro expansion, that would preferably be fixed down the line. Patch by Tibor Brunner! Differential Revision: https://reviews.llvm.org/D57891 llvm-svn: 355705 --- .../StaticAnalyzer/Core/PlistDiagnostics.cpp | 52 +++++-- .../plist-macros-with-expansion.cpp.plist | 134 ++++++++++++++++++ .../Analysis/plist-macros-with-expansion.cpp | 11 ++ 3 files changed, 186 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp index bda72e870040..92d0720448ef 100644 --- a/clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp +++ b/clang/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 @@ public: /// 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 &Printer, - SourceLocation MacroLoc, - const Preprocessor &PP, - const MacroArgMap &PrevArgs); +/// +/// 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 &Printer, + SourceLocation MacroLoc, + const Preprocessor &PP, + const MacroArgMap &PrevArgs, + llvm::SmallPtrSet &AlreadyProcessedTokens); /// Retrieves the name of the macro and what it's arguments expand into /// at \p ExpanLoc. @@ -828,19 +839,35 @@ static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc, 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 &Printer, - SourceLocation MacroLoc, - const Preprocessor &PP, - const MacroArgMap &PrevArgs) { +static std::string getMacroNameAndPrintExpansion( + TokenPrinter &Printer, + SourceLocation MacroLoc, + const Preprocessor &PP, + const MacroArgMap &PrevArgs, + llvm::SmallPtrSet &AlreadyProcessedTokens) { const SourceManager &SM = 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 @@ static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer, // 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 @@ static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer, } getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP, - Info.Args); + Info.Args, AlreadyProcessedTokens); if (MI->getNumParams() != 0) ArgIt = getMatchingRParen(++ArgIt, ArgEnd); } @@ -911,6 +939,8 @@ static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer, Printer.printToken(T); } + AlreadyProcessedTokens.erase(IDInfo); + return Info.Name; } diff --git a/clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist b/clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist index 4d1d42438ef9..68f02a38fdf5 100644 --- a/clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist +++ b/clang/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 diff --git a/clang/test/Analysis/plist-macros-with-expansion.cpp b/clang/test/Analysis/plist-macros-with-expansion.cpp index c3175a332115..057a165f9465 100644 --- a/clang/test/Analysis/plist-macros-with-expansion.cpp +++ b/clang/test/Analysis/plist-macros-with-expansion.cpp @@ -440,3 +440,14 @@ void test() { } // 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}} +}