Merge pull request #52 from envp/fix-partial-macro

ENG2STAAR-2224: Fix bug in `clangmetatool::SourceUtil::isPartialMacro`

src/source_util.cpp

@@ -216,7 +216,22 @@ bool SourceUtil::isPartialMacro(const clang::SourceRange &sourceRange,
 
   clang::SourceLocation begin = sourceRange.getBegin();
 
+  auto usesGccVarargExtensionAtLoc = [&sourceManager,
+                                      &preprocessor](const auto &loc) {
+    if (auto *macroInfo = getMacroInfo(loc, sourceManager, preprocessor)) {
+      return macroInfo->isVariadic() && macroInfo->hasCommaPasting();
+    }
+    return false;
+  };
+
   while (begin.isMacroID()) {
+    // If a macro in the heierarchy uses the GCC '##' extension (see [1])
+    // we can't easily trace up the context stack how the statement is formed
+    // from component macros. Cop out, return true
+    // [1]: https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html
+    if (usesGccVarargExtensionAtLoc(begin)) {
+      return true;
+    }
     // Only process macros where the statement is in the body, not ones where
     // it is an argument.
 
@@ -228,6 +243,9 @@ bool SourceUtil::isPartialMacro(const clang::SourceRange &sourceRange,
           getMacroTokens(begin, sourceManager, preprocessor);
       if (!tokens.empty()) {
         clang::SourceLocation macroStart = tokens.front().getLocation();
+        // FIXME
+        // There is potentially a bug here, this is unable to deal with macros
+        // that expand to more than one access expression
         clang::SourceLocation statementStart =
             sourceManager.getSpellingLoc(begin);
 
@@ -250,6 +268,13 @@ bool SourceUtil::isPartialMacro(const clang::SourceRange &sourceRange,
   const clang::MacroInfo *prevMacro = nullptr;
 
   while (end.isMacroID()) {
+    // If a macro in the heierarchy uses the GCC '##' extension (see [1])
+    // we can't easily trace up the context stack how the statement is formed
+    // from component macros. Cop out, return true
+    // [1]: https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html
+    if (usesGccVarargExtensionAtLoc(end)) {
+      return true;
+    }
     // Only process macros where the statement is in the body, not ones where
     // it is an argument.
 

t/033-detect-partial-macro-expansion.t.cpp

@@ -0,0 +1,145 @@
+#include "clangmetatool-testconfig.h"
+
+#include <llvm/Support/raw_ostream.h>
+
+#include <clang/ASTMatchers/ASTMatchFinder.h>
+#include <clang/ASTMatchers/ASTMatchers.h>
+#include <clang/Frontend/CompilerInstance.h>
+#include <clang/Tooling/CommonOptionsParser.h>
+#include <clang/Tooling/Core/Replacement.h>
+#include <clang/Tooling/Refactoring.h>
+#include <clang/Tooling/Tooling.h>
+
+#include <clangmetatool/meta_tool.h>
+#include <clangmetatool/meta_tool_factory.h>
+#include <clangmetatool/source_util.h>
+
+#include <exception>
+#include <functional>
+#include <memory>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+namespace {
+using namespace clang::ast_matchers;
+
+// Custom matcher for the type of variable we're looking for:
+// i.e. one that has global storage
+auto globalArrayRefWithName = [](const auto &name, const auto &parentName) {
+  return arraySubscriptExpr(
+             hasBase(ignoringParenImpCasts(
+                 declRefExpr(to(varDecl(hasGlobalStorage(), hasName(name))))
+                     .bind("base"))),
+             hasAncestor(functionDecl(hasName(parentName))))
+      .bind("context");
+};
+
+} // namespace
+
+class CollectDeclRef : public MatchFinder::MatchCallback {
+public:
+  std::set<const clang::Expr *> d_refs;
+  CollectDeclRef() : d_refs() {}
+
+  virtual void run(const MatchFinder::MatchResult &result) override {
+    if (const auto *ref = result.Nodes.getNodeAs<clang::Expr>("context")) {
+      d_refs.insert(ref);
+    }
+  }
+};
+
+class TestTool {
+public:
+  typedef std::string ArgTypes;
+
+private:
+  clang::CompilerInstance *d_ci;
+  StatementMatcher d_globalVariableRef;
+  CollectDeclRef d_callback;
+  ArgTypes d_parentFuncDeclName;
+
+public:
+  TestTool(clang::CompilerInstance *ci, MatchFinder *f, ArgTypes parentName)
+      : d_ci(ci),
+        d_globalVariableRef(globalArrayRefWithName("_GLOBALARRAY", parentName)),
+        d_callback(), d_parentFuncDeclName(parentName) {
+    f->addMatcher(d_globalVariableRef, &d_callback);
+  }
+
+  void postProcessing(
+      std::map<std::string, clang::tooling::Replacements> &replacementsMap) {
+    // These were all the refs to the variable we declared
+    for (auto expr : d_callback.d_refs) {
+      if (d_parentFuncDeclName == "positive") {
+        ASSERT_TRUE(clangmetatool::SourceUtil::isPartialMacro(
+            expr->getSourceRange(), d_ci->getSourceManager(),
+            d_ci->getPreprocessor()));
+      } else if (d_parentFuncDeclName == "negative") {
+        ASSERT_FALSE(clangmetatool::SourceUtil::isPartialMacro(
+            expr->getSourceRange(), d_ci->getSourceManager(),
+            d_ci->getPreprocessor()));
+      } else {
+        llvm::errs() << "Invalid branch\n";
+        std::terminate();
+      }
+    }
+  }
+};
+
+TEST(partialMacroExpansion_Detect, positive) {
+  llvm::cl::OptionCategory category("my-tool options");
+  int argc = 4;
+  const char *argv[] = {
+      "foo",
+      CMAKE_SOURCE_DIR
+      "/t/data/033-detect-partial-macro-expansion/partial-macro-usages.cpp",
+      "--", "-xc++"};
+
+  clang::tooling::CommonOptionsParser optionsParser(argc, argv, category);
+  clang::tooling::RefactoringTool tool(optionsParser.getCompilations(),
+                                       optionsParser.getSourcePathList());
+  std::string positive("positive");
+  clangmetatool::MetaToolFactory<clangmetatool::MetaTool<TestTool>> raf(
+      tool.getReplacements(), positive);
+  int r = tool.runAndSave(&raf);
+  ASSERT_EQ(0, r);
+}
+
+TEST(partialMacroExpansion_Detect, negative) {
+  llvm::cl::OptionCategory category("my-tool options");
+  int argc = 4;
+  const char *argv[] = {
+      "foo",
+      CMAKE_SOURCE_DIR
+      "/t/data/033-detect-partial-macro-expansion/partial-macro-usages.cpp",
+      "--", "-xc++"};
+
+  clang::tooling::CommonOptionsParser optionsParser(argc, argv, category);
+  clang::tooling::RefactoringTool tool(optionsParser.getCompilations(),
+                                       optionsParser.getSourcePathList());
+  std::string negative("negative");
+  clangmetatool::MetaToolFactory<clangmetatool::MetaTool<TestTool>> raf(
+      tool.getReplacements(), negative);
+  int r = tool.runAndSave(&raf);
+  ASSERT_EQ(0, r);
+}
+
+TEST(partialMacroExpansion_Detect, DISABLED_FalseNegatives) {
+  llvm::cl::OptionCategory category("my-tool options");
+  int argc = 4;
+  const char *argv[] = {
+      "foo",
+      CMAKE_SOURCE_DIR
+      "/t/data/033-detect-partial-macro-expansion/partial-macro-usages.cpp",
+      "--", "-xc++"};
+
+  clang::tooling::CommonOptionsParser optionsParser(argc, argv, category);
+  clang::tooling::RefactoringTool tool(optionsParser.getCompilations(),
+                                       optionsParser.getSourcePathList());
+  std::string negative("falseNegative");
+  clangmetatool::MetaToolFactory<clangmetatool::MetaTool<TestTool>> raf(
+      tool.getReplacements(), negative);
+  int r = tool.runAndSave(&raf);
+  ASSERT_EQ(0, r);
+}

t/CMakeLists.txt

@@ -49,6 +49,7 @@ foreach(
   030-cstring-propagation-bug
   031-validate-include-graph
   032-find-functions
+  033-detect-partial-macro-expansion
   )
 
   add_executable(${TEST}.t ${TEST}.t.cpp)

t/data/033-detect-partial-macro-expansion/partial-macro-usages.cpp

@@ -0,0 +1,66 @@
+extern int vararg_func(int x, ...);
+extern int func(int x, int y, int z);
+
+extern char _GLOBALARRAY[256];
+
+#define M_ARRAY _GLOBALARRAY
+
+#define BEFORE_EXPR(x) 1 + M_ARRAY[x]
+
+#define NESTED2 M_ARRAY
+#define NESTED(x) NESTED2[x]
+
+#define EXPR(x) M_ARRAY[x]
+#define EXTRA_CHARS(x) ( (        \
+                        (( M_ARRAY  \
+                           [ x ] ) \
+                       ) ))
+
+#define AFTER_EXPR(x) M_ARRAY[x] + 1
+
+#define INNER_MACRO(x, y) (x + y)
+#define OUTER_MACRO(x) (INNER_MACRO(x, M_ARRAY[0]))
+
+#define ONE 1
+#define FORWARD_TO_EXPR(x) EXPR(x)
+#define ALIAS_FOR_EXPR EXPR(1)
+#define M_ARRAY_SECOND M_ARRAY[ONE]
+
+#define NONVARARG_TO_VARARG(x) vararg_func(x)
+#define VARARG_TO_VARARG_DIRECT(...) vararg_func(__VA_ARGS__)
+#define VARARG_TO_VARARG_PASTED(x, y, ...) vararg_func(x, y, ##__VA_ARGS__)
+#define VARARG_TO_NONVARARG_DIRECT(...) func(__VA_ARGS__, 0, 1)
+#define VARARG_TO_NONVARARG_PASTED(...) func(0 ## 1, ##__VA_ARGS__, 1)
+
+#define MACRO_AS_ARG(array_like_macro_name) \
+  array_like_macro_name[0] + array_like_macro_name[1] + array_like_macro_name[2]
+
+
+int positive()
+{
+  return
+      BEFORE_EXPR(0)
+    + AFTER_EXPR(4)
+    + OUTER_MACRO(6)
+    + VARARG_TO_VARARG_PASTED(0, 1, M_ARRAY[1])
+    + VARARG_TO_NONVARARG_PASTED(M_ARRAY[1]);
+}
+
+int negative() {
+  return
+      NESTED(1)
+    + EXPR(0)
+    + EXTRA_CHARS(2)
+    + INNER_MACRO(5, M_ARRAY[0])
+    + FORWARD_TO_EXPR(0)
+    + ALIAS_FOR_EXPR
+    + M_ARRAY_SECOND
+    + NONVARARG_TO_VARARG(M_ARRAY[1])
+    + VARARG_TO_VARARG_DIRECT(0, M_ARRAY[1])
+    + VARARG_TO_NONVARARG_DIRECT(M_ARRAY[1]);
+
+}
+
+int falseNegative() {
+  return MACRO_AS_ARG(M_ARRAY);
+}