Extend GCMole with poor man's data flow analysis to catch dead raw pointer vars.

Fix various places in the code found by improved GCMole.

Review URL: http://codereview.chromium.org/6973063

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@7895 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/arm/macro-assembler-arm.cc

@@ -1757,7 +1757,7 @@ MaybeObject* MacroAssembler::TryTailCallStub(CodeStub* stub, Condition cond) {
   { MaybeObject* maybe_result = stub->TryGetCode();
     if (!maybe_result->ToObject(&result)) return maybe_result;
   }
-  Jump(stub->GetCode(), RelocInfo::CODE_TARGET, cond);
+  Jump(Handle<Code>(Code::cast(result)), RelocInfo::CODE_TARGET, cond);
   return result;
 }
 

src/arm/stub-cache-arm.cc

@@ -3528,7 +3528,8 @@ MaybeObject* ExternalArrayStubCompiler::CompileKeyedLoadStub(
       __ Ret();
     } else {
       WriteInt32ToHeapNumberStub stub(value, r0, r3);
-      __ TailCallStub(&stub);
+      MaybeObject* stub_code = masm()->TryTailCallStub(&stub);
+      if (stub_code->IsFailure()) return stub_code;
     }
   } else if (array_type == kExternalUnsignedIntArray) {
     // The test is different for unsigned int values. Since we need

src/bootstrapper.cc

@@ -1701,14 +1701,14 @@ void Genesis::InstallBuiltinFunctionIds() {
   F(16, global_context()->regexp_function())
 
 
-static FixedArray* CreateCache(int size, JSFunction* factory_function) {
+static FixedArray* CreateCache(int size, Handle<JSFunction> factory_function) {
   Factory* factory = factory_function->GetIsolate()->factory();
   // Caches are supposed to live for a long time, allocate in old space.
   int array_size = JSFunctionResultCache::kEntriesIndex + 2 * size;
   // Cannot use cast as object is not fully initialized yet.
   JSFunctionResultCache* cache = reinterpret_cast<JSFunctionResultCache*>(
       *factory->NewFixedArrayWithHoles(array_size, TENURED));
-  cache->set(JSFunctionResultCache::kFactoryIndex, factory_function);
+  cache->set(JSFunctionResultCache::kFactoryIndex, *factory_function);
   cache->MakeZeroSize();
   return cache;
 }
@@ -1726,7 +1726,7 @@ void Genesis::InstallJSFunctionResultCaches() {
   int index = 0;
 
 #define F(size, func) do {                                              \
-    FixedArray* cache = CreateCache((size), (func)); \
+    FixedArray* cache = CreateCache((size), Handle<JSFunction>(func));  \
     caches->set(index++, cache);                                        \
   } while (false)
 

src/liveedit.cc

@@ -527,12 +527,12 @@ static Handle<Object> UnwrapJSValue(Handle<JSValue> jsValue) {
 
 // Wraps any object into a OpaqueReference, that will hide the object
 // from JavaScript.
-static Handle<JSValue> WrapInJSValue(Object* object) {
+static Handle<JSValue> WrapInJSValue(Handle<Object> object) {
   Handle<JSFunction> constructor =
       Isolate::Current()->opaque_reference_function();
   Handle<JSValue> result =
       Handle<JSValue>::cast(FACTORY->NewJSObject(constructor));
-  result->set_value(object);
+  result->set_value(*object);
   return result;
 }
 
@@ -599,17 +599,17 @@ class FunctionInfoWrapper : public JSArrayBasedStruct<FunctionInfoWrapper> {
   }
   void SetFunctionCode(Handle<Code> function_code,
       Handle<Object> code_scope_info) {
-    Handle<JSValue> code_wrapper = WrapInJSValue(*function_code);
+    Handle<JSValue> code_wrapper = WrapInJSValue(function_code);
     this->SetField(kCodeOffset_, code_wrapper);
 
-    Handle<JSValue> scope_wrapper = WrapInJSValue(*code_scope_info);
+    Handle<JSValue> scope_wrapper = WrapInJSValue(code_scope_info);
     this->SetField(kCodeScopeInfoOffset_, scope_wrapper);
   }
   void SetOuterScopeInfo(Handle<Object> scope_info_array) {
     this->SetField(kOuterScopeInfoOffset_, scope_info_array);
   }
   void SetSharedFunctionInfo(Handle<SharedFunctionInfo> info) {
-    Handle<JSValue> info_holder = WrapInJSValue(*info);
+    Handle<JSValue> info_holder = WrapInJSValue(info);
     this->SetField(kSharedFunctionInfoOffset_, info_holder);
   }
   int GetParentIndex() {
@@ -666,7 +666,7 @@ class SharedInfoWrapper : public JSArrayBasedStruct<SharedInfoWrapper> {
                      Handle<SharedFunctionInfo> info) {
     HandleScope scope;
     this->SetField(kFunctionNameOffset_, name);
-    Handle<JSValue> info_holder = WrapInJSValue(*info);
+    Handle<JSValue> info_holder = WrapInJSValue(info);
     this->SetField(kSharedInfoOffset_, info_holder);
     this->SetSmiValueField(kStartPositionOffset_, start_position);
     this->SetSmiValueField(kEndPositionOffset_, end_position);

src/objects.cc

@@ -2873,8 +2873,9 @@ MaybeObject* JSObject::DeleteElement(uint32_t index, DeleteMode mode) {
           // exception. dictionary->DeleteProperty will return false_value()
           // if a non-configurable property is being deleted.
           HandleScope scope;
+          Handle<Object> self(this);
           Handle<Object> i = isolate->factory()->NewNumberFromUint(index);
-          Handle<Object> args[2] = { i, Handle<Object>(this) };
+          Handle<Object> args[2] = { i, self };
           return isolate->Throw(*isolate->factory()->NewTypeError(
               "strict_delete_property", HandleVector(args, 2)));
         }
@@ -7292,7 +7293,7 @@ MaybeObject* JSObject::GetElementWithCallback(Object* receiver,
 
   // api style callbacks.
   if (structure->IsAccessorInfo()) {
-    AccessorInfo* data = AccessorInfo::cast(structure);
+    Handle<AccessorInfo> data(AccessorInfo::cast(structure));
     Object* fun_obj = data->getter();
     v8::AccessorGetter call_fun = v8::ToCData<v8::AccessorGetter>(fun_obj);
     HandleScope scope(isolate);
@@ -7349,14 +7350,16 @@ MaybeObject* JSObject::SetElementWithCallback(Object* structure,
 
   if (structure->IsAccessorInfo()) {
     // api style callbacks
-    AccessorInfo* data = AccessorInfo::cast(structure);
+    Handle<JSObject> self(this);
+    Handle<JSObject> holder_handle(JSObject::cast(holder));
+    Handle<AccessorInfo> data(AccessorInfo::cast(structure));
     Object* call_obj = data->setter();
     v8::AccessorSetter call_fun = v8::ToCData<v8::AccessorSetter>(call_obj);
     if (call_fun == NULL) return value;
     Handle<Object> number = isolate->factory()->NewNumberFromUint(index);
     Handle<String> key(isolate->factory()->NumberToString(number));
-    LOG(isolate, ApiNamedPropertyAccess("store", this, *key));
-    CustomArguments args(isolate, data->data(), this, JSObject::cast(holder));
+    LOG(isolate, ApiNamedPropertyAccess("store", *self, *key));
+    CustomArguments args(isolate, data->data(), *self, *holder_handle);
     v8::AccessorInfo info(args.end());
     {
       // Leaving JavaScript.
@@ -7558,8 +7561,8 @@ MaybeObject* JSObject::SetElementWithoutInterceptor(uint32_t index,
           // If put fails instrict mode, throw exception.
           if (!dictionary->ValueAtPut(entry, value) &&
               strict_mode == kStrictMode) {
-            Handle<Object> number(isolate->factory()->NewNumberFromUint(index));
             Handle<Object> holder(this);
+            Handle<Object> number(isolate->factory()->NewNumberFromUint(index));
             Handle<Object> args[2] = { number, holder };
             return isolate->Throw(
                 *isolate->factory()->NewTypeError("strict_read_only_property",

src/runtime.cc

@@ -7928,12 +7928,13 @@ static ObjectPair LoadContextSlotHelper(Arguments args,
     // If the "property" we were looking for is a local variable or an
     // argument in a context, the receiver is the global object; see
     // ECMA-262, 3rd., 10.1.6 and 10.2.3.
-    JSObject* receiver =
-        isolate->context()->global()->global_receiver();
+    // GetElement below can cause GC.
+    Handle<JSObject> receiver(
+        isolate->context()->global()->global_receiver());
     MaybeObject* value = (holder->IsContext())
         ? Context::cast(*holder)->get(index)
         : JSObject::cast(*holder)->GetElement(index);
-    return MakePair(Unhole(isolate->heap(), value, attributes), receiver);
+    return MakePair(Unhole(isolate->heap(), value, attributes), *receiver);
   }
 
   // If the holder is found, we read the property from it.
@@ -7948,10 +7949,14 @@ static ObjectPair LoadContextSlotHelper(Arguments args,
     } else {
       receiver = ComputeReceiverForNonGlobal(isolate, object);
     }
+
+    // GetProperty below can cause GC.
+    Handle<JSObject> receiver_handle(receiver);
+
     // No need to unhole the value here. This is taken care of by the
     // GetProperty function.
     MaybeObject* value = object->GetProperty(*name);
-    return MakePair(value, receiver);
+    return MakePair(value, *receiver_handle);
   }
 
   if (throw_error) {

src/uri.js

@@ -166,7 +166,10 @@ function URIDecodeOctets(octets, result, index) {
 // ECMA-262, section 15.1.3
 function Encode(uri, unescape) {
   var uriLength = uri.length;
-  var result = new $Array(uriLength);
+  // We are going to pass result to %StringFromCharCodeArray
+  // which does not expect any getters/setters installed
+  // on the incoming array.
+  var result = new InternalArray(uriLength);
   var index = 0;
   for (var k = 0; k < uriLength; k++) {
     var cc1 = uri.charCodeAt(k);
@@ -192,7 +195,10 @@ function Encode(uri, unescape) {
 // ECMA-262, section 15.1.3
 function Decode(uri, reserved) {
   var uriLength = uri.length;
-  var result = new $Array(uriLength);
+  // We are going to pass result to %StringFromCharCodeArray
+  // which does not expect any getters/setters installed
+  // on the incoming array.
+  var result = new InternalArray(uriLength);
   var index = 0;
   for (var k = 0; k < uriLength; k++) {
     var ch = uri.charAt(k);

tools/gcmole/gccause.lua

+-- Copyright 2011 the V8 project authors. All rights reserved.
+-- Redistribution and use in source and binary forms, with or without
+-- modification, are permitted provided that the following conditions are
+-- met:
+--
+--     * Redistributions of source code must retain the above copyright
+--       notice, this list of conditions and the following disclaimer.
+--     * Redistributions in binary form must reproduce the above
+--       copyright notice, this list of conditions and the following
+--       disclaimer in the documentation and/or other materials provided
+--       with the distribution.
+--     * Neither the name of Google Inc. nor the names of its
+--       contributors may be used to endorse or promote products derived
+--       from this software without specific prior written permission.
+--
+-- THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+-- "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+-- LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+-- A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+-- OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+-- SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+-- LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+-- DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+-- THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+-- (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+-- OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+-- This is an auxiliary tool that reads gccauses file generated by
+-- gcmole.lua and prints tree of the calls that can potentially cause a GC
+-- inside a given function.
+--
+-- Usage: lua tools/gcmole/gccause.lua <function-name-pattern>
+--
+
+assert(loadfile "gccauses")()
+
+local P = ...
+
+local T = {}
+
+local function TrackCause(name, lvl)
+   io.write(("  "):rep(lvl or 0), name, "\n")
+   if GC[name] then
+      local causes = GC[name]
+      for i = 1, #causes do
+	 local f = causes[i]
+	 if not T[f] then
+	    T[f] = true
+	    TrackCause(f, (lvl or 0) + 1)
+	 end
+      end
+   end
+end
+
+for name, _ in pairs(GC) do
+   if name:match(P) then
+      T = {}
+      TrackCause(name)
+   end
+end

tools/gcmole/gcmole.lua

@@ -30,7 +30,43 @@
 
 local DIR = arg[0]:match("^(.+)/[^/]+$")
 
-local ARCHS = arg[1] and { arg[1] } or { 'ia32', 'arm', 'x64' }
+local FLAGS = {
+   -- Do not build gcsuspects file and reuse previously generated one.
+   reuse_gcsuspects = false;
+
+   -- Print commands to console before executing them.
+   verbose = false;
+
+   -- Perform dead variable analysis (generates many false positives).
+   -- TODO add some sort of whiteliste to filter out false positives.
+   dead_vars = 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
+   -- below.
+   whitelist = true;
+}
+local ARGS = {}
+
+for i = 1, #arg do
+   local flag = arg[i]:match "^%-%-([%w_-]+)$"
+   if flag then
+      local no, real_flag = flag:match "^(no)([%w_-]+)$"
+      if real_flag then flag = real_flag end
+
+      flag = flag:gsub("%-", "_")
+      if FLAGS[flag] ~= nil then
+         FLAGS[flag] = (no ~= "no")
+      else
+         error("Unknown flag: " .. flag)
+      end
+   else
+      table.insert(ARGS, arg[i])
+   end
+end
+
+local ARCHS = ARGS[1] and { ARGS[1] } or { 'ia32', 'arm', 'x64' }
 
 local io = require "io"
 local os = require "os"
@@ -49,9 +85,16 @@ if not CLANG_BIN or CLANG_BIN == "" then
    error "CLANG_BIN not set"
 end
 
-local function MakeClangCommandLine(plugin, triple, arch_define)
+local function MakeClangCommandLine(plugin, plugin_args, triple, arch_define)
+   if plugin_args then
+     for i = 1, #plugin_args do
+        plugin_args[i] = "-plugin-arg-" .. plugin .. " " .. plugin_args[i]
+     end
+     plugin_args = " " .. table.concat(plugin_args, " ")
+   end
    return CLANG_BIN .. "/clang -cc1 -load " .. DIR .. "/libgcmole.so"
       .. " -plugin "  .. plugin
+      .. (plugin_args or "")
       .. " -triple " .. triple
       .. " -D" .. arch_define
       .. " -DENABLE_VMSTATE_TRACKING"
@@ -62,14 +105,14 @@ end
 
 function InvokeClangPluginForEachFile(filenames, cfg, func)
    local cmd_line = MakeClangCommandLine(cfg.plugin,
+                                         cfg.plugin_args,
                                          cfg.triple,
                                          cfg.arch_define)
 
    for _, filename in ipairs(filenames) do
       log("-- %s", filename)
-
       local action = cmd_line .. " src/" .. filename .. " 2>&1"
-
+      if FLAGS.verbose then print('popen ', action) end
       local pipe = io.popen(action)
       func(filename, pipe:lines())
       pipe:close()
@@ -159,8 +202,35 @@ local ARCHITECTURES = {
 -------------------------------------------------------------------------------
 -- GCSuspects Generation
 
-local gc = {}
-local funcs = {}
+local gc, gc_caused, funcs
+
+local WHITELIST = {
+   -- The following functions call CEntryStub which is always present.
+   "MacroAssembler.*CallExternalReference",
+   "MacroAssembler.*CallRuntime",
+   "CompileCallLoadPropertyWithInterceptor",
+   "CallIC.*GenerateMiss",
+
+   -- DirectCEntryStub is a special stub used on ARM. 
+   -- It is pinned and always present.
+   "DirectCEntryStub.*GenerateCall",  
+
+   -- TODO GCMole currently is sensitive enough to understand that certain 
+   --      functions only cause GC and return Failure simulataneously. 
+   --      Callsites of such functions are safe as long as they are properly 
+   --      check return value and propagate the Failure to the caller.
+   --      It should be possible to extend GCMole to understand this.
+   "Heap.*AllocateFunctionPrototype"
+};
+
+local function AddCause(name, cause)
+   local t = gc_caused[name]
+   if not t then
+      t = {}
+      gc_caused[name] = t
+   end
+   table.insert(t, cause)
+end
 
 local function resolve(name)
    local f = funcs[name]
@@ -169,7 +239,18 @@ local function resolve(name)
       f = {}
       funcs[name] = f
 
-      if name:match "Collect.*Garbage" then gc[name] = true end
+      if name:match "Collect.*Garbage" then
+         gc[name] = true
+         AddCause(name, "<GC>")
+      end
+
+      if FLAGS.whitelist then
+         for i = 1, #WHITELIST do
+            if name:match(WHITELIST[i]) then
+               gc[name] = false
+            end
+         end
+      end
    end
 
     return f
@@ -192,21 +273,25 @@ end
 local function propagate ()
    log "** Propagating GC information"
 
-   local function mark(callers)
+   local function mark(from, callers)
       for caller, _ in pairs(callers) do
-	 if not gc[caller] then
+         if gc[caller] == nil then
             gc[caller] = true
-	    mark(funcs[caller]) 
+            mark(caller, funcs[caller])
          end
+         AddCause(caller, from)
       end
    end
 
    for funcname, callers in pairs(funcs) do
-      if gc[funcname] then mark(callers) end
+      if gc[funcname] then mark(funcname, callers) end
    end
 end
 
 local function GenerateGCSuspects(arch, files, cfg)
+   -- Reset the global state.
+   gc, gc_caused, funcs = {}, {}, {}
+
    log ("** Building GC Suspects for %s", arch)
    InvokeClangPluginForEachFile (files,
                                  cfg:extend { plugin = "dump-callees" },
@@ -215,19 +300,32 @@ local function GenerateGCSuspects(arch, files, cfg)
    propagate()
 
    local out = assert(io.open("gcsuspects", "w"))
-   for name, _ in pairs(gc) do out:write (name, '\n') end
+   for name, value in pairs(gc) do if value then out:write (name, '\n') end end
    out:close()
+
+   local out = assert(io.open("gccauses", "w"))
+   out:write "GC = {"
+   for name, causes in pairs(gc_caused) do
+      out:write("['", name, "'] = {")
+      for i = 1, #causes do out:write ("'", causes[i], "';") end
+      out:write("};\n")
+   end
+   out:write "}"
+   out:close()
+
    log ("** GCSuspects generated for %s", arch)
 end
 
--------------------------------------------------------------------------------
+--------------------------------------------------------------------------------
 -- Analysis
 
 local function CheckCorrectnessForArch(arch)
    local files = FilesForArch(arch)
    local cfg = ARCHITECTURES[arch]
 
+   if not FLAGS.reuse_gcsuspects then
       GenerateGCSuspects(arch, files, cfg)
+   end
 
    local processed_files = 0
    local errors_found = false
@@ -242,9 +340,14 @@ local function CheckCorrectnessForArch(arch)
       end
    end
 
-   log("** Searching for evaluation order problems for %s", arch)
+   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
    InvokeClangPluginForEachFile(files,
-				cfg:extend { plugin = "find-problems" },
+                                cfg:extend { plugin = "find-problems",
+                                             plugin_args = plugin_args },
                                 SearchForErrors)
    log("** Done processing %d files. %s",
        processed_files,