[tools] Add DisableGCMole scope

Make sure gcmole detects issue in DisallowGarbageCollection scopes.

DisallowGarbageCollection is widely used in the codebase to document
code that doesn't allocate. However, this has the rather unexpected
side-effect that gcmole is not run when such a scope is active.

This CL changes the default behavior of gcmole to run even with
DisallowGarbageCollection scopes present. This will give us the best
results of both worlds, dynamic checks by the fuzzer, and static
analysis by gcmole.

To allow crazy local raw pointer operations there is a new
DisableGCMole scope that explicitly disables gcmole.

Change-Id: I0a78fb3b4ceaad35be9bcf7293d917a41f90c91f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2615419Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72039}

src/builtins/builtins-array.cc

@@ -6,6 +6,7 @@
 #include "src/builtins/builtins-utils-inl.h"
 #include "src/builtins/builtins.h"
 #include "src/codegen/code-factory.h"
+#include "src/common/assert-scope.h"
 #include "src/debug/debug.h"
 #include "src/execution/isolate.h"
 #include "src/execution/protectors-inl.h"
@@ -969,6 +970,7 @@ void CollectElementIndices(Isolate* isolate, Handle<JSObject> object,
     case FAST_SLOPPY_ARGUMENTS_ELEMENTS:
     case SLOW_SLOPPY_ARGUMENTS_ELEMENTS: {
       DisallowGarbageCollection no_gc;
+      DisableGCMole no_gc_mole;
       FixedArrayBase elements = object->elements();
       JSObject raw_object = *object;
       ElementsAccessor* accessor = object->GetElementsAccessor();

src/common/assert-scope.cc

@@ -84,6 +84,7 @@ template class PerThreadAssertScope<CODE_DEPENDENCY_CHANGE_ASSERT, false>;
 template class PerThreadAssertScope<CODE_DEPENDENCY_CHANGE_ASSERT, true>;
 template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, false>;
 template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, true>;
+template class PerThreadAssertScope<GC_MOLE, false>;
 
 template class PerIsolateAssertScope<JAVASCRIPT_EXECUTION_ASSERT, false>;
 template class PerIsolateAssertScope<JAVASCRIPT_EXECUTION_ASSERT, true>;

src/common/assert-scope.h

@@ -26,7 +26,9 @@ enum PerThreadAssertType {
   HANDLE_ALLOCATION_ASSERT,
   HANDLE_DEREFERENCE_ASSERT,
   CODE_DEPENDENCY_CHANGE_ASSERT,
-  CODE_ALLOCATION_ASSERT
+  CODE_ALLOCATION_ASSERT,
+  // Dummy type for disabling GC mole.
+  GC_MOLE,
 };
 
 enum PerIsolateAssertType {
@@ -35,7 +37,7 @@ enum PerIsolateAssertType {
   JAVASCRIPT_EXECUTION_DUMP,
   DEOPTIMIZATION_ASSERT,
   COMPILATION_ASSERT,
-  NO_EXCEPTION_ASSERT
+  NO_EXCEPTION_ASSERT,
 };
 
 template <PerThreadAssertType kType, bool kAllow>
@@ -191,6 +193,11 @@ using AllowCodeAllocation =
 // DisallowHeapAllocation by also forbidding safepoints.
 using DisallowGarbageCollection =
     CombinationAssertScope<DisallowSafepoints, DisallowHeapAllocation>;
+
+// Scope to skip gc mole verification in places where we do tricky raw
+// work.
+using DisableGCMole = PerThreadAssertScopeDebugOnly<GC_MOLE, false>;
+
 // The DISALLOW_GARBAGE_COLLECTION macro can be used to define a
 // DisallowGarbageCollection field in classes that isn't present in release
 // builds.
@@ -215,15 +222,6 @@ using AllowHeapAccess =
     CombinationAssertScope<AllowCodeDependencyChange, AllowHandleDereference,
                            AllowHandleAllocation, AllowHeapAllocation>;
 
-// The DISALLOW_GARBAGE_COLLECTION macro can be used to define a
-// DisallowSafepoints field in classes that isn't present in release
-// builds.
-#ifdef DEBUG
-#define DISALLOW_GARBAGE_COLLECTION(name) DisallowGarbageCollection name;
-#else
-#define DISALLOW_GARBAGE_COLLECTION(name)
-#endif
-
 class DisallowHeapAccessIf {
  public:
   explicit DisallowHeapAccessIf(bool condition) {
@@ -328,6 +326,7 @@ extern template class PerThreadAssertScope<CODE_DEPENDENCY_CHANGE_ASSERT,
 extern template class PerThreadAssertScope<CODE_DEPENDENCY_CHANGE_ASSERT, true>;
 extern template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, false>;
 extern template class PerThreadAssertScope<CODE_ALLOCATION_ASSERT, true>;
+extern template class PerThreadAssertScope<GC_MOLE, false>;
 
 extern template class PerIsolateAssertScope<JAVASCRIPT_EXECUTION_ASSERT, false>;
 extern template class PerIsolateAssertScope<JAVASCRIPT_EXECUTION_ASSERT, true>;

src/regexp/experimental/experimental.cc

@@ -4,6 +4,7 @@
 
 #include "src/regexp/experimental/experimental.h"
 
+#include "src/common/assert-scope.h"
 #include "src/objects/js-regexp-inl.h"
 #include "src/regexp/experimental/experimental-compiler.h"
 #include "src/regexp/experimental/experimental-interpreter.h"
@@ -145,6 +146,8 @@ int32_t ExecRawImpl(Isolate* isolate, RegExp::CallOrigin call_origin,
                     int32_t* output_registers, int32_t output_register_count,
                     int32_t subject_index) {
   DisallowGarbageCollection no_gc;
+  // TODO(cbruni): remove once gcmole is fixed.
+  DisableGCMole no_gc_mole;
 
   int register_count_per_match =
       JSRegExp::RegistersForCaptureCount(capture_count);

src/regexp/regexp-macro-assembler.cc

@@ -204,22 +204,25 @@ int NativeRegExpMacroAssembler::CheckStackGuardState(
   bool is_one_byte = String::IsOneByteRepresentationUnderneath(*subject_handle);
   int return_value = 0;
 
-  if (js_has_overflowed) {
-    AllowGarbageCollection yes_gc;
-    isolate->StackOverflow();
-    return_value = EXCEPTION;
-  } else if (check.InterruptRequested()) {
-    AllowGarbageCollection yes_gc;
-    Object result = isolate->stack_guard()->HandleInterrupts();
-    if (result.IsException(isolate)) return_value = EXCEPTION;
-  }
+  {
+    DisableGCMole no_gc_mole;
+    if (js_has_overflowed) {
+      AllowGarbageCollection yes_gc;
+      isolate->StackOverflow();
+      return_value = EXCEPTION;
+    } else if (check.InterruptRequested()) {
+      AllowGarbageCollection yes_gc;
+      Object result = isolate->stack_guard()->HandleInterrupts();
+      if (result.IsException(isolate)) return_value = EXCEPTION;
+    }
 
-  if (*code_handle != re_code) {  // Return address no longer valid
-    // Overwrite the return address on the stack.
-    intptr_t delta = code_handle->address() - re_code.address();
-    Address new_pc = old_pc + delta;
-    // TODO(v8:10026): avoid replacing a signed pointer.
-    PointerAuthentication::ReplacePC(return_address, new_pc, 0);
+    if (*code_handle != re_code) {  // Return address no longer valid
+      // Overwrite the return address on the stack.
+      intptr_t delta = code_handle->address() - re_code.address();
+      Address new_pc = old_pc + delta;
+      // TODO(v8:10026): avoid replacing a signed pointer.
+      PointerAuthentication::ReplacePC(return_address, new_pc, 0);
+    }
   }
 
   // If we continue, we need to update the subject string addresses.

tools/gcmole/README

@@ -35,9 +35,9 @@ simply to store it as a Handle.
 
 PREREQUISITES -----------------------------------------------------------------
 
-(1) Install Lua 5.1
+(1) Install Python
 
-    $ sudo apt-get install lua5.1
+    $ sudo apt-get install python
 
 (2) Get LLVM 8.0 and Clang 8.0 sources and build them.
 
@@ -62,14 +62,14 @@ PREREQUISITES -----------------------------------------------------------------
 
 USING GCMOLE ------------------------------------------------------------------
 
-gcmole consists of driver script written in Lua and Clang plugin that does
+gcmole consists of driver script written in Python and Clang plugin that does
 C++ AST processing. Plugin (libgcmole.so) is expected to be in the same
-folder as driver (gcmole.lua).
+folder as driver (gcmole.py).
 
 To start analysis cd into the root of v8 checkout and execute the following
 command:
 
-CLANG_BIN=<path-to-clang-bin-folder> lua tools/gcmole/gcmole.lua [<arch>]
+CLANG_BIN=<path-to-clang-bin-folder> python tools/gcmole/gcmole.py [<arch>]
 
 where arch should be one of architectures supported by V8 (arm, ia32, x64).
 
@@ -89,7 +89,7 @@ If any errors were found driver exits with non-zero status.
 
 TESTING -----------------------------------------------------------------------
 
-Tests are automatically run by the main lua runner. Expectations are in
+Tests are automatically run by the main python 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).
 

tools/gcmole/bootstrap.sh

@@ -83,34 +83,40 @@ set -x
 
 NUM_JOBS=3
 if [[ "${OS}" = "Linux" ]]; then
-  NUM_JOBS="$(grep -c "^processor" /proc/cpuinfo)"
+  if [[ -e "/proc/cpuinfo" ]]; then
+    NUM_JOBS="$(grep -c "^processor" /proc/cpuinfo)"
+  else
+    # Hack when running in chroot
+    NUM_JOBS="32"
+  fi
 elif [ "${OS}" = "Darwin" ]; then
   NUM_JOBS="$(sysctl -n hw.ncpu)"
 fi
 
 # Build clang.
-if [ ! -e "${BUILD_DIR}" ]; then
-  mkdir "${BUILD_DIR}"
-fi
-cd "${BUILD_DIR}"
-cmake -GNinja -DCMAKE_CXX_FLAGS="-static-libstdc++" -DLLVM_ENABLE_TERMINFO=OFF \
-    -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=clang \
-    -DLLVM_ENABLE_Z3_SOLVER=OFF "${LLVM_PROJECT_DIR}/llvm"
-MACOSX_DEPLOYMENT_TARGET=10.5 ninja -j"${NUM_JOBS}"
-
-# Strip the clang binary.
-STRIP_FLAGS=
-if [ "${OS}" = "Darwin" ]; then
-  # See http://crbug.com/256342
-  STRIP_FLAGS=-x
-fi
-strip ${STRIP_FLAGS} bin/clang
-cd -
+# if [ ! -e "${BUILD_DIR}" ]; then
+#   mkdir "${BUILD_DIR}"
+# fi
+# cd "${BUILD_DIR}"
+# cmake -GNinja -DCMAKE_CXX_FLAGS="-static-libstdc++" -DLLVM_ENABLE_TERMINFO=OFF \
+#     -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=clang \
+#     -DLLVM_ENABLE_Z3_SOLVER=OFF "${LLVM_PROJECT_DIR}/llvm"
+# MACOSX_DEPLOYMENT_TARGET=10.5 ninja -j"${NUM_JOBS}"
+# 
+# # Strip the clang binary.
+# STRIP_FLAGS=
+# if [ "${OS}" = "Darwin" ]; then
+#   # See http://crbug.com/256342
+#   STRIP_FLAGS=-x
+# fi
+# strip ${STRIP_FLAGS} bin/clang
+# cd -
 
 # Build libgcmole.so
 make -C "${THIS_DIR}" clean
 make -C "${THIS_DIR}" LLVM_SRC_ROOT="${LLVM_PROJECT_DIR}/llvm" \
-    CLANG_SRC_ROOT="${LLVM_PROJECT_DIR}/clang" BUILD_ROOT="${BUILD_DIR}" libgcmole.so
+    CLANG_SRC_ROOT="${LLVM_PROJECT_DIR}/clang" \
+    BUILD_ROOT="${BUILD_DIR}" libgcmole.so
 
 set +x
 

tools/gcmole/gcmole-test.cc

@@ -175,55 +175,130 @@ void TestDeadVarBecauseOfSafepointAnalysis(Isolate* isolate) {
 void TestGuardedDeadVarAnalysis(Isolate* isolate) {
   JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
 
+  // Note: having DisableGCMole with the same function as CauseGC
+  // normally doesn't make sense, but we want to test whether the guards
+  // are recognized by GCMole.
+  DisableGCMole no_gc_mole;
+  CauseGCRaw(raw_obj, isolate);
+
+  // Shouldn't cause warning.
+  raw_obj.Print();
+}
+
+void TestGuardedDeadVarAnalysis2(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+
   // Note: having DisallowGarbageCollection with the same function as CauseGC
-  // normally doesn't make sense, but we want to test whether the gurads
+  // normally doesn't make sense, but we want to test whether the guards
   // are recognized by GCMole.
   DisallowGarbageCollection no_gc;
   CauseGCRaw(raw_obj, isolate);
 
-  // Shouldn't cause warning.
+  // Should cause warning.
   raw_obj.Print();
 }
 
 void TestGuardedAgainstSafepointDeadVarAnalysis(Isolate* isolate) {
   JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
 
+  // Note: having DisableGCMole with the same function as CauseGC
+  // normally doesn't make sense, but we want to test whether the guards
+  // are recognized by GCMole.
+  DisableGCMole no_gc_mole;
+  Safepoint();
+
+  // Shouldn't cause warning.
+  raw_obj.Print();
+}
+
+void TestGuardedAgainstSafepointDeadVarAnalysis2(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+
   // Note: having DisallowGarbageCollection with the same function as CauseGC
-  // normally doesn't make sense, but we want to test whether the gurads
+  // normally doesn't make sense, but we want to test whether the guards
   // are recognized by GCMole.
   DisallowGarbageCollection no_gc;
   Safepoint();
 
-  // Shouldn't cause warning.
+  // Should cause warning.
   raw_obj.Print();
 }
 
-void TestOnlyHeapGuardedDeadVarAnalysisInCompound(Isolate* isolate) {
+void TestGuardedAgainstSafepointDeadVarAnalysis3(Isolate* isolate) {
   JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  // Note: having DisallowGarbageCollection with the same function as CauseGC
+  // normally doesn't make sense, but we want to test whether the guards
+  // are recognized by GCMole.
+  DisallowGarbageCollection no_gc;
+  Safepoint();
+  // Should cause warning.
+  raw_obj.Print();
+  {
+    DisableGCMole no_gc_mole;
+    // Shouldn't cause warning.
+    raw_obj.Print();
+  }
+  // Should cause warning.
+  raw_obj.Print();
+}
 
+void TestOnlyHeapGuardedDeadVarAnalysisInCompound(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
   // {DisallowHeapAccess} has a {DisallowHeapAllocation}, but no
   // {DisallowSafepoints}, so it could see objects move due to safepoints.
   DisallowHeapAccess no_gc;
   CauseGCRaw(raw_obj, isolate);
+  // Should cause warning.
+  raw_obj.Print();
+}
 
+void TestOnlyHeapGuardedDeadVarAnalysisInCompound2(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  // {DisallowHeapAccess} has a {DisallowHeapAllocation}, but no
+  // {DisallowSafepoints}, so it could see objects move due to safepoints.
+  DisallowHeapAccess no_gc;
+  CauseGCRaw(raw_obj, isolate);
+  // Should cause warning.
+  raw_obj.Print();
+  DisableGCMole no_gc_mole;
   // Should cause warning.
   raw_obj.Print();
 }
 
 void TestGuardedDeadVarAnalysisNested(JSObject raw_obj, Isolate* isolate) {
   CauseGCRaw(raw_obj, isolate);
-
   // Should cause warning.
   raw_obj.Print();
 }
 
 void TestGuardedDeadVarAnalysisCaller(Isolate* isolate) {
-  DisallowHeapAccess no_gc;
+  DisableGCMole no_gc_mole;
   JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  TestGuardedDeadVarAnalysisNested(raw_obj, isolate);
+  // Shouldn't cause warning.
+  raw_obj.Print();
+}
+
+void TestGuardedDeadVarAnalysisCaller2(Isolate* isolate) {
+  DisallowGarbageCollection no_gc;
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  TestGuardedDeadVarAnalysisNested(raw_obj, isolate);
+  // Should cause warning.
+  raw_obj.Print();
+}
 
+void TestGuardedDeadVarAnalysisCaller3(Isolate* isolate) {
+  DisallowHeapAccess no_gc;
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
   TestGuardedDeadVarAnalysisNested(raw_obj, isolate);
+  // Should cause warning.
+  raw_obj.Print();
+}
 
-  // Shouldn't cause warning.
+void TestGuardedDeadVarAnalysisCaller4(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  TestGuardedDeadVarAnalysisNested(raw_obj, isolate);
+  // Should cause warning.
   raw_obj.Print();
 }
 
@@ -232,26 +307,47 @@ JSObject GuardedAllocation(Isolate* isolate) {
   return *isolate->factory()->NewJSObjectWithNullProto();
 }
 
+JSObject GuardedAllocation2(Isolate* isolate) {
+  DisableGCMole no_gc_mole;
+  return *isolate->factory()->NewJSObjectWithNullProto();
+}
+
 void TestNestedDeadVarAnalysis(Isolate* isolate) {
   JSObject raw_obj = GuardedAllocation(isolate);
   CauseGCRaw(raw_obj, isolate);
-
   // Should cause warning.
   raw_obj.Print();
 }
 
+void TestNestedDeadVarAnalysis2(Isolate* isolate) {
+  DisableGCMole no_gc_mole;
+  JSObject raw_obj = GuardedAllocation(isolate);
+  CauseGCRaw(raw_obj, isolate);
+  // Shouldn't cause warning.
+  raw_obj.Print();
+}
+
 // Test that putting a guard in the middle of the function doesn't
 // mistakenly cover the whole scope of the raw variable.
 void TestGuardedDeadVarAnalysisMidFunction(Isolate* isolate) {
   JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
-
   CauseGCRaw(raw_obj, isolate);
-
   // Guarding the rest of the function from triggering a GC.
   DisallowGarbageCollection no_gc;
   // Should cause warning.
   raw_obj.Print();
 }
 
+// Test that putting a guard in the middle of the function doesn't
+// mistakenly cover the whole scope of the raw variable.
+void TestGuardedDeadVarAnalysisMidFunction2(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  CauseGCRaw(raw_obj, isolate);
+  // Guarding the rest of the function from triggering a GC.
+  DisableGCMole no_gc_mole;
+  // Should cause warning.
+  raw_obj.Print();
+}
+
 }  // namespace internal
 }  // namespace v8

tools/gcmole/gcmole-tools.tar.gz.sha1

-2612625af89f79a6e8996fb12330397eed1e359d
+165dd133e7dff1583a6b6611e3d5a8382bcd0893

tools/gcmole/gcmole.cc

@@ -229,8 +229,9 @@ 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()))
+    if (IsExternalVMState(expr->getDecl())) {
       AddCallee("CollectGarbage", "CollectGarbage");
+    }
     return true;
   }
 
@@ -685,13 +686,13 @@ class FunctionAnalyzer {
   FunctionAnalyzer(clang::MangleContext* ctx, clang::CXXRecordDecl* object_decl,
                    clang::CXXRecordDecl* maybe_object_decl,
                    clang::CXXRecordDecl* smi_decl,
-                   clang::CXXRecordDecl* no_gc_decl,
+                   clang::CXXRecordDecl* no_gc_mole_decl,
                    clang::DiagnosticsEngine& d, clang::SourceManager& sm)
       : ctx_(ctx),
         object_decl_(object_decl),
         maybe_object_decl_(maybe_object_decl),
         smi_decl_(smi_decl),
-        no_gc_decl_(no_gc_decl),
+        no_gc_mole_decl_(no_gc_mole_decl),
         d_(d),
         sm_(sm),
         block_(NULL) {}
@@ -1278,9 +1279,7 @@ class FunctionAnalyzer {
   DECL_VISIT_STMT(ForStmt) {
     Block block (VisitStmt(stmt->getInit(), env), this);
     do {
-      block.Loop(stmt->getCond(),
-                 stmt->getBody(),
-                 stmt->getInc());
+      block.Loop(stmt->getCond(), stmt->getBody(), stmt->getInc());
     } while (block.changed());
     return block.out();
   }
@@ -1335,25 +1334,15 @@ class FunctionAnalyzer {
 
   const clang::CXXRecordDecl* GetDefinitionOrNull(
       const clang::CXXRecordDecl* record) {
-    if (record == NULL) {
-      return NULL;
-    }
-
+    if (record == NULL) return NULL;
     if (!InV8Namespace(record)) return NULL;
-
-    if (!record->hasDefinition()) {
-      return NULL;
-    }
-
+    if (!record->hasDefinition()) return NULL;
     return record->getDefinition();
   }
 
   bool IsDerivedFromInternalPointer(const clang::CXXRecordDecl* record) {
     const clang::CXXRecordDecl* definition = GetDefinitionOrNull(record);
-    if (!definition) {
-      return false;
-    }
-
+    if (!definition) return false;
     bool result = (IsDerivedFrom(record, object_decl_) &&
                    !IsDerivedFrom(record, smi_decl_)) ||
                   IsDerivedFrom(record, maybe_object_decl_);
@@ -1378,13 +1367,9 @@ class FunctionAnalyzer {
   // such. For V8 that means Object and MaybeObject instances.
   bool RepresentsRawPointerType(clang::QualType qtype) {
     // Not yet assigned pointers can't get moved by the GC.
-    if (qtype.isNull()) {
-      return false;
-    }
+    if (qtype.isNull()) return false;
     // nullptr can't get moved by the GC.
-    if (qtype->isNullPtrType()) {
-      return false;
-    }
+    if (qtype->isNullPtrType()) return false;
 
     const clang::PointerType* pointer_type =
         llvm::dyn_cast_or_null<clang::PointerType>(qtype.getTypePtrOrNull());
@@ -1396,21 +1381,15 @@ class FunctionAnalyzer {
   }
 
   bool IsGCGuard(clang::QualType qtype) {
-    if (qtype.isNull()) {
-      return false;
-    }
-    if (qtype->isNullPtrType()) {
-      return false;
-    }
+    if (!no_gc_mole_decl_) return false;
+    if (qtype.isNull()) return false;
+    if (qtype->isNullPtrType()) return false;
 
     const clang::CXXRecordDecl* record = qtype->getAsCXXRecordDecl();
     const clang::CXXRecordDecl* definition = GetDefinitionOrNull(record);
 
-    if (!definition) {
-      return false;
-    }
-
-    return no_gc_decl_ && IsDerivedFrom(definition, no_gc_decl_);
+    if (!definition) return false;
+    return no_gc_mole_decl_ == definition;
   }
 
   Environment VisitDecl(clang::Decl* decl, Environment& env) {
@@ -1494,7 +1473,7 @@ class FunctionAnalyzer {
   clang::CXXRecordDecl* object_decl_;
   clang::CXXRecordDecl* maybe_object_decl_;
   clang::CXXRecordDecl* smi_decl_;
-  clang::CXXRecordDecl* no_gc_decl_;
+  clang::CXXRecordDecl* no_gc_mole_decl_;
   clang::CXXRecordDecl* no_heap_access_decl_;
 
   clang::DiagnosticsEngine& d_;
@@ -1559,44 +1538,38 @@ class ProblemsFinder : public clang::ASTConsumer,
   }
 
   virtual void HandleTranslationUnit(clang::ASTContext &ctx) {
-    if (TranslationUnitIgnored()) {
-      return;
-    }
+    if (TranslationUnitIgnored()) return;
 
     Resolver r(ctx);
 
-    // It is a valid situation that no_gc_decl == NULL when the
-    // DisallowGarbageCollection is not included and can't be resolved.
-    // This is gracefully handled in the FunctionAnalyzer later.
-    clang::CXXRecordDecl* no_gc_decl =
-        r.ResolveNamespace("v8")
-            .ResolveNamespace("internal")
-            .ResolveTemplate("DisallowGarbageCollection");
+    // It is a valid situation that no_gc_mole_decl == NULL when DisableGCMole
+    // is not included and can't be resolved. This is gracefully handled in the
+    // FunctionAnalyzer later.
+    auto v8_internal = r.ResolveNamespace("v8").ResolveNamespace("internal");
+    clang::CXXRecordDecl* no_gc_mole_decl =
+        v8_internal.ResolveTemplate("DisableGCMole");
 
     clang::CXXRecordDecl* object_decl =
-        r.ResolveNamespace("v8").ResolveNamespace("internal").
-            Resolve<clang::CXXRecordDecl>("Object");
+        v8_internal.Resolve<clang::CXXRecordDecl>("Object");
 
     clang::CXXRecordDecl* maybe_object_decl =
-        r.ResolveNamespace("v8")
-            .ResolveNamespace("internal")
-            .Resolve<clang::CXXRecordDecl>("MaybeObject");
+        v8_internal.Resolve<clang::CXXRecordDecl>("MaybeObject");
 
     clang::CXXRecordDecl* smi_decl =
-        r.ResolveNamespace("v8").ResolveNamespace("internal").
-            Resolve<clang::CXXRecordDecl>("Smi");
+        v8_internal.Resolve<clang::CXXRecordDecl>("Smi");
 
     if (object_decl != NULL) object_decl = object_decl->getDefinition();
 
-    if (maybe_object_decl != NULL)
+    if (maybe_object_decl != NULL) {
       maybe_object_decl = maybe_object_decl->getDefinition();
+    }
 
     if (smi_decl != NULL) smi_decl = smi_decl->getDefinition();
 
     if (object_decl != NULL && smi_decl != NULL && maybe_object_decl != NULL) {
       function_analyzer_ = new FunctionAnalyzer(
           clang::ItaniumMangleContext::create(ctx, d_), object_decl,
-          maybe_object_decl, smi_decl, no_gc_decl, d_, sm_);
+          maybe_object_decl, smi_decl, no_gc_mole_decl, d_, sm_);
       TraverseDecl(ctx.getTranslationUnitDecl());
     } else {
       if (object_decl == NULL) {

tools/gcmole/gcmole.py

@@ -474,6 +474,7 @@ def CheckCorrectnessForArch(arch, for_test, flags, clang_bin_dir,
 
 
 def TestRun(flags, clang_bin_dir, clang_plugins_dir):
+  log("** Test Run")
   errors_found, output = CheckCorrectnessForArch("x64", True, flags,
                                                  clang_bin_dir,
                                                  clang_plugins_dir)
@@ -489,7 +490,7 @@ def TestRun(flags, clang_bin_dir, clang_plugins_dir):
   if output != expectations:
     log("** Output mismatch from running tests. Please run them manually.")
 
-    for line in difflib.context_diff(
+    for line in difflib.unified_diff(
         expectations.splitlines(),
         output.splitlines(),
         fromfile=filename,
@@ -498,6 +499,7 @@ def TestRun(flags, clang_bin_dir, clang_plugins_dir):
     ):
       log("{}", line)
 
+    log("------")
     log("--- Full output ---")
     log(output)
     log("------")
@@ -526,7 +528,7 @@ def main(args):
       #:n't use parallel python runner.
       "sequential": False,
       # Print commands to console before executing them.
-      "verbose": False,
+      "verbose": True,
       # Perform dead variable analysis.
       "dead_vars": True,
       # Enable verbose tracing from the plugin itself.

tools/gcmole/package.sh

@@ -47,6 +47,21 @@ echo You can find a packaged version of gcmole here:
 echo
 echo $(readlink -f "${PACKAGE_FILE}")
 echo
+echo Upload the update package to the chrome infra:
+echo
+echo 'gsutil.py cp tools/gcmole/gcmole-tools.tar.gz gs://chrome-v8-gcmole/$(cat tools/gcmole/gcmole-tools.tar.gz.sha1)'
+echo
+echo Run bootstrap.sh in chroot if glibc versions mismatch with bots:
+echo '# Create chroot'
+echo 'mkdir -p $CHROOT_DIR'
+echo 'sudo debootstrap $DEBIAN_VERSION $CHROOT_DIR'
+echo 'sudo chroot $CHROOT_DIR apt install g++ cmake python git'
+echo '# Mount ./depot_tools and ./v8 dirs in the chroot:'
+echo 'sudo chroot $CHROOT_DIR mkdir /docs'
+echo 'sudo mount --bind /path/to/workspace /docs'
+echo '# Build gcmole'
+echo "sudo chroot \$CHROOT_DIR bash -c 'PATH=/docs/depot_tools:\$PATH; /docs/v8/v8/tools/gcmole/bootstrap.sh'"
+echo
 echo You can now run gcmole using this command:
 echo
 echo CLANG_BIN=\"tools/gcmole/gcmole-tools/bin\" python tools/gcmole/gcmole.py

tools/gcmole/test-expectations.txt

@@ -28,19 +28,46 @@ tools/gcmole/gcmole-test.cc:164:3: warning: Possibly dead variable.
 tools/gcmole/gcmole-test.cc:172:3: warning: Possibly dead variable.
   raw_obj.Print();
   ^
-tools/gcmole/gcmole-test.cc:210:3: warning: Possibly dead variable.
+tools/gcmole/gcmole-test.cc:198:3: warning: Possibly dead variable.
   raw_obj.Print();
   ^
-tools/gcmole/gcmole-test.cc:217:3: warning: Possibly dead variable.
+tools/gcmole/gcmole-test.cc:224:3: warning: Possibly dead variable.
   raw_obj.Print();
   ^
-tools/gcmole/gcmole-test.cc:227:3: warning: Possibly dead variable.
+tools/gcmole/gcmole-test.cc:235:3: warning: Possibly dead variable.
   raw_obj.Print();
   ^
-tools/gcmole/gcmole-test.cc:240:3: warning: Possibly dead variable.
+tools/gcmole/gcmole-test.cc:242:3: warning: Possibly dead variable.
   raw_obj.Print();
   ^
-tools/gcmole/gcmole-test.cc:253:3: warning: Possibly dead variable.
+tools/gcmole/gcmole-test.cc:252:3: warning: Possibly dead variable.
   raw_obj.Print();
   ^
-15 warnings generated.
+tools/gcmole/gcmole-test.cc:262:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:265:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:271:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:287:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:295:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:302:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:319:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:338:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:349:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+24 warnings generated.