[gcmole] Fix traversing virtual methods

Since this produces a few false positives, also implemented a whitelist
mechanism to not report them.

Also, add a couple of tests and implemented automated testing against
test-expectations file.

Bug: v8:9321
Change-Id: I2915a29fe1891e8bbc51118bbd95ae072c8de023
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1773243
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63456}

tools/gcmole/BUILD.gn

@@ -9,10 +9,13 @@ group("v8_run_gcmole") {
 
   data = [
     "gccause.lua",
+    "GCMOLE.gn",
     "gcmole.lua",
     "gcmole-tools/",
     "parallel.py",
     "run-gcmole.py",
+    "suspects.whitelist",
+    "test-expectations.txt",
 
     # The following contains all relevant source and build files.
     "../debug_helper/debug-helper.h",

tools/gcmole/GCMOLE.gn

+action("gcmole") {
+  sources = [
+    ### gcmole(all) ###
+    "tools/gcmole/gcmole-test.cc",
+  ]
+}

tools/gcmole/README

@@ -71,6 +71,12 @@ can be ignored.
 
 If any errors were found driver exits with non-zero status.
 
+TESTING -----------------------------------------------------------------------
+
+Tests are automatically run by the main lua runner. Expectations are in
+test-expectations.txt and need to be updated whenever the sources of the tests
+in gcmole-test.cc are modified (line numbers also count).
+
 PACKAGING ---------------------------------------------------------------------
 
 gcmole is deployed on V8's buildbot infrastructure to run it as part of the

tools/gcmole/gcmole-test.cc

@@ -5,26 +5,43 @@
 #include "src/execution/isolate.h"
 #include "src/handles/handles-inl.h"
 #include "src/handles/handles.h"
+#include "src/objects/foreign-inl.h"
+#include "src/objects/managed.h"
 #include "src/objects/maybe-object.h"
 #include "src/objects/object-macros.h"
 
 namespace v8 {
 namespace internal {
 
+// ------- Test simple argument evaluation order problems ---------
+
 Handle<Object> CauseGC(Handle<Object> obj, Isolate* isolate) {
   isolate->heap()->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting);
 
   return obj;
 }
 
+Object CauseGCRaw(Object obj, Isolate* isolate) {
+  isolate->heap()->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting);
+
+  return obj;
+}
+
+Managed<Smi> CauseGCManaged(int i, Isolate* isolate) {
+  isolate->heap()->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting);
+
+  return Managed<Smi>::cast(Smi::FromInt(i));
+}
+
 void TwoArgumentsFunction(Object a, Object b) {
-  a->Print();
-  b->Print();
+  a.Print();
+  b.Print();
 }
 
 void TestTwoArguments(Isolate* isolate) {
   Handle<JSObject> obj1 = isolate->factory()->NewJSObjectWithNullProto();
   Handle<JSObject> obj2 = isolate->factory()->NewJSObjectWithNullProto();
+  // Should cause warning.
   TwoArgumentsFunction(*CauseGC(obj1, isolate), *CauseGC(obj2, isolate));
 }
 
@@ -36,13 +53,16 @@ void TwoSizeTArgumentsFunction(size_t a, size_t b) {
 void TestTwoSizeTArguments(Isolate* isolate) {
   Handle<JSObject> obj1 = isolate->factory()->NewJSObjectWithNullProto();
   Handle<JSObject> obj2 = isolate->factory()->NewJSObjectWithNullProto();
+  // Should cause warning.
   TwoSizeTArgumentsFunction(sizeof(*CauseGC(obj1, isolate)),
                             sizeof(*CauseGC(obj2, isolate)));
 }
 
+// --------- Test problems with method arguments ----------
+
 class SomeObject : public Object {
  public:
-  void Method(Object a) { a->Print(); }
+  void Method(Object a) { a.Print(); }
 
   SomeObject& operator=(const Object& b) {
     this->Print();
@@ -58,14 +78,57 @@ void TestMethodCall(Isolate* isolate) {
   SomeObject obj;
   Handle<SomeObject> so = handle(obj, isolate);
   Handle<JSObject> obj1 = isolate->factory()->NewJSObjectWithNullProto();
+  // Should cause warning.
   so->Method(*CauseGC(obj1, isolate));
+  // Should cause warning.
+  so->Method(CauseGCRaw(*obj1, isolate));
 }
 
 void TestOperatorCall(Isolate* isolate) {
   SomeObject obj;
   Handle<JSObject> obj1 = isolate->factory()->NewJSObjectWithNullProto();
+  // Should not cause warning.
   obj = *CauseGC(obj1, isolate);
 }
 
+// --------- Test for templated sub-classes of Object ----------
+
+void TestFollowingTemplates(Isolate* isolate) {
+  // Should cause warning.
+  CauseGCManaged(42, isolate);
+}
+
+// --------- Test for correctly resolving virtual methods ----------
+
+class BaseObject {
+ public:
+  virtual Handle<Object> VirtualCauseGC(Handle<Object> obj, Isolate* isolate) {
+    return obj;
+  }
+};
+
+class DerivedObject : public BaseObject {
+ public:
+  Handle<Object> VirtualCauseGC(Handle<Object> obj, Isolate* isolate) override {
+    isolate->heap()->CollectGarbage(OLD_SPACE,
+                                    GarbageCollectionReason::kTesting);
+
+    return obj;
+  }
+};
+
+void TestFollowingVirtualFunctions(Isolate* isolate) {
+  DerivedObject derived;
+  BaseObject* base = &derived;
+  Handle<JSObject> obj1 = isolate->factory()->NewJSObjectWithNullProto();
+
+  SomeObject so;
+  Handle<SomeObject> so_handle = handle(so, isolate);
+  // Should cause warning.
+  so_handle->Method(*derived.VirtualCauseGC(obj1, isolate));
+  // Should cause warning.
+  so_handle->Method(*base->VirtualCauseGC(obj1, isolate));
+}
+
 }  // namespace internal
 }  // namespace v8

tools/gcmole/gcmole.cc

@@ -47,6 +47,7 @@ namespace {
 
 typedef std::string MangledName;
 typedef std::set<MangledName> CalleesSet;
+typedef std::map<MangledName, MangledName> CalleesMap;
 
 static bool GetMangledName(clang::MangleContext* ctx,
                            const clang::NamedDecl* decl,
@@ -138,14 +139,16 @@ class CalleesPrinter : public clang::RecursiveASTVisitor<CalleesPrinter> {
   virtual bool VisitDeclRefExpr(clang::DeclRefExpr* expr) {
     // If function mentions EXTERNAL VMState add artificial garbage collection
     // mark.
-    if (IsExternalVMState(expr->getDecl())) AddCallee("CollectGarbage");
+    if (IsExternalVMState(expr->getDecl()))
+      AddCallee("CollectGarbage", "CollectGarbage");
     return true;
   }
 
   void AnalyzeFunction(const clang::FunctionDecl* f) {
     MangledName name;
     if (InV8Namespace(f) && GetMangledName(ctx_, f, &name)) {
-      AddCallee(name);
+      const std::string& function = f->getNameAsString();
+      AddCallee(name, function);
 
       const clang::FunctionDecl* body = NULL;
       if (f->hasBody(body) && !Analyzed(name)) {
@@ -176,21 +179,22 @@ class CalleesPrinter : public clang::RecursiveASTVisitor<CalleesPrinter> {
     scopes_.pop();
   }
 
-  void AddCallee(const MangledName& name) {
+  void AddCallee(const MangledName& name, const MangledName& function) {
     if (!scopes_.empty()) scopes_.top()->insert(name);
+    mangled_to_function_[name] = function;
   }
 
   void PrintCallGraph() {
     for (Callgraph::const_iterator i = callgraph_.begin(), e = callgraph_.end();
          i != e;
          ++i) {
-      std::cout << i->first << "\n";
+      std::cout << i->first << "," << mangled_to_function_[i->first] << "\n";
 
       CalleesSet* callees = i->second;
       for (CalleesSet::const_iterator j = callees->begin(), e = callees->end();
            j != e;
            ++j) {
-        std::cout << "\t" << *j << "\n";
+        std::cout << "\t" << *j << "," << mangled_to_function_[*j] << "\n";
       }
     }
   }
@@ -200,6 +204,7 @@ class CalleesPrinter : public clang::RecursiveASTVisitor<CalleesPrinter> {
 
   std::stack<CalleesSet* > scopes_;
   Callgraph callgraph_;
+  CalleesMap mangled_to_function_;
 };
 
 
@@ -234,23 +239,40 @@ class FunctionDeclarationFinder
   CalleesPrinter* callees_printer_;
 };
 
-
-static bool loaded = false;
+static bool gc_suspects_loaded = false;
 static CalleesSet gc_suspects;
-
+static CalleesSet gc_functions;
+static bool whitelist_loaded = false;
+static CalleesSet suspects_whitelist;
 
 static void LoadGCSuspects() {
-  if (loaded) return;
+  if (gc_suspects_loaded) return;
 
   std::ifstream fin("gcsuspects");
-  std::string s;
+  std::string mangled, function;
 
-  while (fin >> s) gc_suspects.insert(s);
+  while (!fin.eof()) {
+    std::getline(fin, mangled, ',');
+    gc_suspects.insert(mangled);
+    std::getline(fin, function);
+    gc_functions.insert(function);
+  }
 
-  loaded = true;
+  gc_suspects_loaded = true;
 }
 
+static void LoadSuspectsWhitelist() {
+  if (whitelist_loaded) return;
+
+  std::ifstream fin("tools/gcmole/suspects.whitelist");
+  std::string s;
+
+  while (fin >> s) suspects_whitelist.insert(s);
+
+  whitelist_loaded = true;
+}
 
+// Looks for exact match of the mangled name
 static bool KnownToCauseGC(clang::MangleContext* ctx,
                            const clang::FunctionDecl* decl) {
   LoadGCSuspects();
@@ -265,6 +287,25 @@ static bool KnownToCauseGC(clang::MangleContext* ctx,
   return false;
 }
 
+// Looks for partial match of only the function name
+static bool SuspectedToCauseGC(clang::MangleContext* ctx,
+                               const clang::FunctionDecl* decl) {
+  LoadGCSuspects();
+
+  if (!InV8Namespace(decl)) return false;
+
+  LoadSuspectsWhitelist();
+  if (suspects_whitelist.find(decl->getNameAsString()) !=
+      suspects_whitelist.end()) {
+    return false;
+  }
+
+  if (gc_functions.find(decl->getNameAsString()) != gc_functions.end()) {
+    return true;
+  }
+
+  return false;
+}
 
 static const int kNoEffect = 0;
 static const int kCausesGC = 1;
@@ -910,10 +951,32 @@ class FunctionAnalyzer {
         RepresentsRawPointerType(call->getType()));
 
     clang::FunctionDecl* callee = call->getDirectCallee();
-    if ((callee != NULL) && KnownToCauseGC(ctx_, callee)) {
+    if (callee != NULL) {
+      if (KnownToCauseGC(ctx_, callee)) {
         out.setGC();
       }
 
+      clang::CXXMethodDecl* method =
+          llvm::dyn_cast_or_null<clang::CXXMethodDecl>(callee);
+      if (method != NULL && method->isVirtual()) {
+        clang::CXXMemberCallExpr* memcall =
+            llvm::dyn_cast_or_null<clang::CXXMemberCallExpr>(call);
+        if (memcall != NULL) {
+          clang::CXXMethodDecl* target = method->getDevirtualizedMethod(
+              memcall->getImplicitObjectArgument(), false);
+          if (target != NULL) {
+            if (KnownToCauseGC(ctx_, target)) {
+              out.setGC();
+            }
+          } else {
+            if (SuspectedToCauseGC(ctx_, method)) {
+              out.setGC();
+            }
+          }
+        }
+      }
+    }
+
     return out;
   }
 

tools/gcmole/gcmole.lua

@@ -183,12 +183,19 @@ end
 
 -------------------------------------------------------------------------------
 
-local function ParseGNFile()
+local function ParseGNFile(for_test)
    local result = {}
-   local gn_files = {
+   local gn_files
+   if for_test then
+      gn_files = {
+         { "tools/gcmole/GCMOLE.gn",             '"([^"]-%.cc)"',      ""         }
+      }
+   else
+      gn_files = {
          { "BUILD.gn",             '"([^"]-%.cc)"',      ""         },
          { "test/cctest/BUILD.gn", '"(test-[^"]-%.cc)"', "test/cctest/" }
       }
+   end
 
    for i = 1, #gn_files do
       local filename = gn_files[i][1]
@@ -231,7 +238,8 @@ local function BuildFileList(sources, props)
 end
 
 
-local gn_sources = ParseGNFile()
+local gn_sources = ParseGNFile(false)
+local gn_test_sources = ParseGNFile(true)
 
 local function FilesForArch(arch)
    return BuildFileList(gn_sources, { os = 'linux',
@@ -240,6 +248,13 @@ local function FilesForArch(arch)
                                       simulator = ''})
 end
 
+local function FilesForTest(arch)
+   return BuildFileList(gn_test_sources, { os = 'linux',
+                                      arch = arch,
+                                      mode = 'debug',
+                                      simulator = ''})
+end
+
 local mtConfig = {}
 
 mtConfig.__index = mtConfig
@@ -393,8 +408,13 @@ end
 --------------------------------------------------------------------------------
 -- Analysis
 
-local function CheckCorrectnessForArch(arch)
-   local files = FilesForArch(arch)
+local function CheckCorrectnessForArch(arch, for_test)
+   local files
+   if for_test then
+      files = FilesForTest(arch)
+   else
+      files = FilesForArch(arch)
+   end
    local cfg = ARCHITECTURES[arch]
 
    if not FLAGS.reuse_gcsuspects then
@@ -403,6 +423,7 @@ local function CheckCorrectnessForArch(arch)
 
    local processed_files = 0
    local errors_found = false
+   local output = ""
    local function SearchForErrors(filename, lines)
       processed_files = processed_files + 1
       for l in lines do
@@ -410,9 +431,13 @@ local function CheckCorrectnessForArch(arch)
             l:match "^[^:]+:%d+:%d+:" or
             l:match "error" or
             l:match "warning"
+         if for_test then
+            output = output.."\n"..l
+         else
             print(l)
          end
       end
+   end
 
    log("** Searching for evaluation order problems%s for %s",
        FLAGS.dead_vars and " and dead variables" or "",
@@ -427,18 +452,34 @@ local function CheckCorrectnessForArch(arch)
        processed_files,
        errors_found and "Errors found" or "No errors found")
 
-   return errors_found
+   return errors_found, output
 end
 
-local function SafeCheckCorrectnessForArch(arch)
-   local status, errors = pcall(CheckCorrectnessForArch, arch)
+local function SafeCheckCorrectnessForArch(arch, for_test)
+   local status, errors, output = pcall(CheckCorrectnessForArch, arch, for_test)
    if not status then
       print(string.format("There was an error: %s", errors))
       errors = true
    end
-   return errors
+   return errors, output
 end
 
+local function TestRun()
+   local errors, output = SafeCheckCorrectnessForArch('x64', true)
+
+   local filename = "tools/gcmole/test-expectations.txt"
+   local exp_file = assert(io.open(filename), "failed to open test expectations file")
+   local expectations = exp_file:read('*all')
+
+   if output ~= expectations then
+      log("** Output mismatch from running tests. Please run them manually.")
+   else
+      log("** Tests ran successfully")
+   end
+end
+
+TestRun()
+
 local errors = false
 
 for _, arch in ipairs(ARCHS) do
@@ -446,7 +487,7 @@ for _, arch in ipairs(ARCHS) do
       error ("Unknown arch: " .. arch)
    end
 
-   errors = SafeCheckCorrectnessForArch(arch, report) or errors
+   errors = SafeCheckCorrectnessForArch(arch, false) or errors
 end
 
 os.exit(errors and 1 or 0)

tools/gcmole/suspects.whitelist

+IsConstructor
+IsEval
+IsAsync
+IsPromiseAll

tools/gcmole/test-expectations.txt

+
+tools/gcmole/gcmole-test.cc:45:3: warning: Possible problem with evaluation order.
+  TwoArgumentsFunction(*CauseGC(obj1, isolate), *CauseGC(obj2, isolate));
+  ^
+tools/gcmole/gcmole-test.cc:57:3: warning: Possible problem with evaluation order.
+  TwoSizeTArgumentsFunction(sizeof(*CauseGC(obj1, isolate)),
+  ^
+tools/gcmole/gcmole-test.cc:82:7: warning: Possible problem with evaluation order.
+  so->Method(*CauseGC(obj1, isolate));
+      ^
+tools/gcmole/gcmole-test.cc:84:7: warning: Possible problem with evaluation order.
+  so->Method(CauseGCRaw(*obj1, isolate));
+      ^
+tools/gcmole/gcmole-test.cc:128:14: warning: Possible problem with evaluation order.
+  so_handle->Method(*derived.VirtualCauseGC(obj1, isolate));
+             ^
+tools/gcmole/gcmole-test.cc:130:14: warning: Possible problem with evaluation order.
+  so_handle->Method(*base->VirtualCauseGC(obj1, isolate));
+             ^
+6 warnings generated.