[gcmole] Relax gcmole reasoning about raw pointers

This CL ensures we care only about our internal pointer types and not
about raw C++ pointers, because normally special care is taken when
storing raw pointers to the managed heap. Furthermore, checking for raw
pointers produces too many false positives in the dead variable
analysis.

Bug: v8:9680, chromium:1000635
Change-Id: Ica9ea1fe09b7456c011910a6886149b6dfdda1f5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1924357
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65174}

tools/gcmole/README

 DESCRIPTION -------------------------------------------------------------------
 
 gcmole is a simple static analysis tool used to find possible evaluation order 
-dependent GC-unsafe places in the V8 codebase.
+dependent GC-unsafe places in the V8 codebase and "stale" pointers to the heap
+(ones whose addresses got invalidated by the GC).
 
 For example the following code is GC-unsafe:
 
     Handle<Object> Foo();  // Assume Foo can trigger a GC.
-    void Bar(Object*, Object*);
+    void Bar(Object, Object);
 
     Handle<Object> baz;
     baz->Qux(*Foo());  // (a)
-    Bar(*Foo(), *baz);  // (b)
+    Bar(*Foo(), *baz); // (b)
 
 Both in cases (a) and (b) compiler is free to evaluate call arguments (that 
 includes receiver) in any order. That means it can dereference baz before 
 calling to Foo and save a raw pointer to a heap object in the register or 
-on the stack.  
+on the stack.
+
+In terms of the AST analysis that gcmole does, it warns about places in the
+code which result in 2 subtrees, the order of execution of which is undefined
+by C++, one of which causes a GC and the other dereferences a Handle to a raw
+Object (or its subclasses).
+
+The following code triggers a stale variable warning (assuming that the Foo
+function was detected as potentially allocating, as in the previous example):
+
+    JSObject raw_obj = ...;
+    Foo();
+    raw_obj.Print();
+
+Since Foo can trigger a GC, it might have moved the raw_obj. The solution is
+simply to store it as a Handle.
 
 PREREQUISITES -----------------------------------------------------------------
 

tools/gcmole/gcmole-test.cc

@@ -130,5 +130,36 @@ void TestFollowingVirtualFunctions(Isolate* isolate) {
   so_handle->Method(*base->VirtualCauseGC(obj1, isolate));
 }
 
+// --------- Test for correctly resolving static methods ----------
+
+class SomeClass {
+ public:
+  static Handle<Object> StaticCauseGC(Handle<Object> obj, Isolate* isolate) {
+    isolate->heap()->CollectGarbage(OLD_SPACE,
+                                    GarbageCollectionReason::kTesting);
+
+    return obj;
+  }
+};
+
+void TestFollowingStaticFunctions(Isolate* isolate) {
+  SomeObject so;
+  Handle<SomeObject> so_handle = handle(so, isolate);
+
+  Handle<JSObject> obj1 = isolate->factory()->NewJSObjectWithNullProto();
+  // Should cause warning.
+  so_handle->Method(*SomeClass::StaticCauseGC(obj1, isolate));
+}
+
+// --------- Test basic dead variable analysis ----------
+
+void TestDeadVarAnalysis(Isolate* isolate) {
+  JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
+  CauseGCRaw(raw_obj, isolate);
+
+  // Should cause warning.
+  raw_obj.Print();
+}
+
 }  // namespace internal
 }  // namespace v8

tools/gcmole/gcmole.lua

@@ -44,6 +44,9 @@ local FLAGS = {
    -- TODO add some sort of whiteliste to filter out false positives.
    dead_vars = false;
 
+   -- Enable verbose tracing from the plugin itself.
+   verbose_trace = false;
+
    -- When building gcsuspects whitelist certain functions as if they
    -- can be causing GC. Currently used to reduce number of false
    -- positives in dead variables analysis. See TODO for WHITELIST
@@ -310,7 +313,10 @@ local WHITELIST = {
    "StateTag",
 
    -- Ignore printing of elements transition.
-   "PrintElementsTransition"
+   "PrintElementsTransition",
+
+   -- CodeCreateEvent receives AbstractCode (a raw ptr) as an argument.
+   "CodeCreateEvent",
 };
 
 local function AddCause(name, cause)
@@ -443,8 +449,9 @@ local function CheckCorrectnessForArch(arch, for_test)
    log("** Searching for evaluation order problems%s for %s",
        FLAGS.dead_vars and " and dead variables" or "",
        arch)
-   local plugin_args
-   if FLAGS.dead_vars then plugin_args = { "--dead-vars" } end
+   local plugin_args = {}
+   if FLAGS.dead_vars then table.insert(plugin_args, "--dead-vars") end
+   if FLAGS.verbose_trace then table.insert(plugin_args, '--verbose') end
    InvokeClangPluginForEachFile(files,
                                 cfg:extend { plugin = "find-problems",
                                              plugin_args = plugin_args },

tools/gcmole/test-expectations.txt

@@ -17,4 +17,7 @@ tools/gcmole/gcmole-test.cc:128:14: warning: Possible problem with evaluation or
 tools/gcmole/gcmole-test.cc:130:14: warning: Possible problem with evaluation order.
   so_handle->Method(*base->VirtualCauseGC(obj1, isolate));
              ^
-6 warnings generated.
+tools/gcmole/gcmole-test.cc:151:14: warning: Possible problem with evaluation order.
+  so_handle->Method(*SomeClass::StaticCauseGC(obj1, isolate));
+             ^
+7 warnings generated.