Fixing the detection of aliased eval so that it is exact.

Fixing the semantics of aliased eval so that it is conformant.
Review URL: http://codereview.chromium.org/11563

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

src/ast.cc

@@ -37,7 +37,7 @@ VariableProxySentinel VariableProxySentinel::this_proxy_(true);
 VariableProxySentinel VariableProxySentinel::identifier_proxy_(false);
 ValidLeftHandSideSentinel ValidLeftHandSideSentinel::instance_;
 Property Property::this_property_(VariableProxySentinel::this_proxy(), NULL, 0);
-Call Call::sentinel_(NULL, NULL, false, 0);
+Call Call::sentinel_(NULL, NULL, Call::ALIASED, 0);
 
 
 // ----------------------------------------------------------------------------

src/ast.h

@@ -864,13 +864,20 @@ class Property: public Expression {
 
 class Call: public Expression {
  public:
+  enum EvalType {
+    ALIASED,               // Either not eval or an aliased eval.
+    POTENTIALLY_DIRECT     // Looks like a direct eval at codegen time.
+                           // Needs to be determined at runtime whether the
+                           // eval is direct.
+  };
+
   Call(Expression* expression,
        ZoneList<Expression*>* arguments,
-       bool is_eval,
+       EvalType eval_type,
        int pos)
       : expression_(expression),
         arguments_(arguments),
-        is_eval_(is_eval),
+        eval_type_(eval_type),
         pos_(pos) { }
 
   virtual void Accept(Visitor* v);
@@ -880,7 +887,7 @@ class Call: public Expression {
 
   Expression* expression() const { return expression_; }
   ZoneList<Expression*>* arguments() const { return arguments_; }
-  bool is_eval()  { return is_eval_; }
+  EvalType eval_type()  { return eval_type_; }
   int position() { return pos_; }
 
   static Call* sentinel() { return &sentinel_; }
@@ -888,7 +895,7 @@ class Call: public Expression {
  private:
   Expression* expression_;
   ZoneList<Expression*>* arguments_;
-  bool is_eval_;
+  EvalType eval_type_;
   int pos_;
 
   static Call sentinel_;
@@ -898,7 +905,7 @@ class Call: public Expression {
 class CallNew: public Call {
  public:
   CallNew(Expression* expression, ZoneList<Expression*>* arguments, int pos)
-      : Call(expression, arguments, false, pos) { }
+      : Call(expression, arguments, ALIASED, pos) { }
 
   virtual void Accept(Visitor* v);
 };

src/codegen-arm.cc

@@ -2320,6 +2320,12 @@ void CodeGenerator::VisitCall(Call* node) {
   // is resolved in cache misses (this also holds for megamorphic calls).
   // ------------------------------------------------------------------------
 
+  if (node->eval_type() == Call::POTENTIALLY_DIRECT) {
+    __ CallRuntime(Runtime::kSetInPotentiallyDirectEval, 0);
+  } else {
+    __ CallRuntime(Runtime::kClearInPotentiallyDirectEval, 0);
+  }
+
   if (var != NULL && !var->is_this() && var->is_global()) {
     // ----------------------------------
     // JavaScript example: 'foo(1, 2, 3)'  // foo is global

src/codegen-ia32.cc

@@ -2690,6 +2690,12 @@ void CodeGenerator::VisitCall(Call* node) {
   // is resolved in cache misses (this also holds for megamorphic calls).
   // ------------------------------------------------------------------------
 
+  if (node->eval_type() == Call::POTENTIALLY_DIRECT) {
+    __ CallRuntime(Runtime::kSetInPotentiallyDirectEval, 0);
+  } else {
+    __ CallRuntime(Runtime::kClearInPotentiallyDirectEval, 0);
+  }
+
   if (var != NULL && !var->is_this() && var->is_global()) {
     // ----------------------------------
     // JavaScript example: 'foo(1, 2, 3)'  // foo is global

src/debug-delay.js

@@ -1117,7 +1117,7 @@ DebugCommandProcessor.prototype.scriptsCommandToJSONRequest_ = function(args) {
 DebugCommandProcessor.prototype.responseToText = function(json_response) {
   try {
     // Convert the JSON string to an object.
-    response = %CompileString('(' + json_response + ')', 0, false)();
+    response = %CompileString('(' + json_response + ')', 0)();
 
     if (!response.success) {
       return response.message;
@@ -1323,7 +1323,7 @@ DebugCommandProcessor.prototype.processDebugJSONRequest = function(json_request,
   try {
     try {
       // Convert the JSON string to an object.
-      request = %CompileString('(' + json_request + ')', 0, false)();
+      request = %CompileString('(' + json_request + ')', 0)();
 
       // Create an initial response.
       response = this.createResponse(request);
@@ -1776,7 +1776,7 @@ DebugCommandProcessor.prototype.scriptsRequest_ = function(request, response) {
 DebugCommandProcessor.prototype.isRunning = function(json_response) {
   try {
     // Convert the JSON string to an object.
-    response = %CompileString('(' + json_response + ')', 0, false)();
+    response = %CompileString('(' + json_response + ')', 0)();
 
     // Return whether VM should be running after this request.
     return response.running;

src/parser.cc

@@ -327,6 +327,8 @@ class ParserFactory BASE_EMBEDDED {
     return Handle<String>();
   }
 
+
+
   virtual Expression* NewProperty(Expression* obj, Expression* key, int pos) {
     if (obj == VariableProxySentinel::this_proxy()) {
       return Property::this_property();
@@ -337,7 +339,7 @@ class ParserFactory BASE_EMBEDDED {
 
   virtual Expression* NewCall(Expression* expression,
                               ZoneList<Expression*>* arguments,
-                              bool is_eval, int pos) {
+                              Call::EvalType eval_type, int pos) {
     return Call::sentinel();
   }
 
@@ -387,8 +389,8 @@ class AstBuildingParserFactory : public ParserFactory {
 
   virtual Expression* NewCall(Expression* expression,
                               ZoneList<Expression*>* arguments,
-                              bool is_eval, int pos) {
-    return new Call(expression, arguments, is_eval, pos);
+                              Call::EvalType eval_type, int pos) {
+    return new Call(expression, arguments, eval_type, pos);
   }
 
   virtual Statement* EmptyStatement() {
@@ -2294,38 +2296,32 @@ Expression* Parser::ParseLeftHandSideExpression(bool* ok) {
         ZoneList<Expression*>* args = ParseArguments(CHECK_OK);
 
         // Keep track of eval() calls since they disable all local variable
-        // optimizations. We can ignore locally declared variables with
-        // name 'eval' since they override the global 'eval' function. We
-        // only need to look at unresolved variables (VariableProxies).
+        // optimizations.
+        // The calls that need special treatment are the
+        // direct (i.e. not aliased) eval calls. These calls are all of the
+        // form eval(...) with no explicit receiver object where eval is not
+        // declared in the current scope chain. These calls are marked as
+        // potentially direct eval calls. Whether they are actually direct calls
+        // to eval is determined at run time.
 
+        Call::EvalType eval_type = Call::ALIASED;
         if (!is_pre_parsing_) {
-          // We assume that only a function called 'eval' can be used
-          // to invoke the global eval() implementation. This permits
-          // for massive optimizations.
           VariableProxy* callee = result->AsVariableProxy();
           if (callee != NULL && callee->IsVariable(Factory::eval_symbol())) {
-            // We do not allow direct calls to 'eval' in our internal
-            // JS files. Use builtin functions instead.
-            ASSERT(!Bootstrapper::IsActive());
-            top_scope_->RecordEvalCall();
-          } else {
-            // This is rather convoluted code to check if we're calling
-            // a function named 'eval' through a property access. If so,
-            // we mark it as a possible eval call (we don't know if the
-            // receiver will resolve to the global object or not), but
-            // we do not treat the call as an eval() call - we let the
-            // call get through to the JavaScript eval code defined in
-            // v8natives.js.
-            Property* property = result->AsProperty();
-            if (property != NULL) {
-              Literal* key = property->key()->AsLiteral();
-              if (key != NULL &&
-                  key->handle().is_identical_to(Factory::eval_symbol())) {
-                // We do not allow direct calls to 'eval' in our
-                // internal JS files. Use builtin functions instead.
-                ASSERT(!Bootstrapper::IsActive());
-                top_scope_->RecordEvalCall();
-              }
+            Handle<String> name = callee->name();
+            Variable* var = NULL;
+            for (Scope* scope = top_scope_;
+                 scope != NULL;
+                 scope = scope->outer_scope()) {
+              var = scope->Lookup(callee->name());
+              if (var != NULL) break;
+            }
+            if (var == NULL) {
+              // We do not allow direct calls to 'eval' in our internal
+              // JS files. Use builtin functions instead.
+              ASSERT(!Bootstrapper::IsActive());
+              top_scope_->RecordEvalCall();
+              eval_type = Call::POTENTIALLY_DIRECT;
             }
           }
         }
@@ -2342,7 +2338,7 @@ Expression* Parser::ParseLeftHandSideExpression(bool* ok) {
         if (is_eval && args->length() == 0) {
           result = NEW(Literal(Factory::undefined_value()));
         } else {
-          result = factory()->NewCall(result, args, is_eval, pos);
+          result = factory()->NewCall(result, args, eval_type, pos);
         }
         break;
       }

src/runtime.cc

@@ -3930,57 +3930,6 @@ static Object* Runtime_NumberIsFinite(Arguments args) {
 }
 
 
-static Object* EvalContext() {
-  // The topmost JS frame belongs to the eval function which called
-  // the CompileString runtime function. We need to unwind one level
-  // to get to the caller of eval.
-  StackFrameLocator locator;
-  JavaScriptFrame* frame = locator.FindJavaScriptFrame(1);
-
-  // TODO(900055): Right now we check if the caller of eval() supports
-  // eval to determine if it's an aliased eval or not. This may not be
-  // entirely correct in the unlikely case where a function uses both
-  // aliased and direct eval calls.
-  HandleScope scope;
-  if (!ScopeInfo<>::SupportsEval(frame->FindCode())) {
-    // Aliased eval: Evaluate in the global context of the eval
-    // function to support aliased, cross environment evals.
-    return *Top::global_context();
-  }
-
-  // Fetch the caller context from the frame.
-  Handle<Context> caller(Context::cast(frame->context()));
-
-  // Check for eval() invocations that cross environments. Use the
-  // context from the stack if evaluating in current environment.
-  Handle<Context> target = Top::global_context();
-  if (caller->global_context() == *target) return *caller;
-
-  // Otherwise, use the global context from the other environment.
-  return *target;
-}
-
-
-static Object* Runtime_EvalReceiver(Arguments args) {
-  ASSERT(args.length() == 1);
-  StackFrameLocator locator;
-  JavaScriptFrame* frame = locator.FindJavaScriptFrame(1);
-  // Fetch the caller context from the frame.
-  Context* caller = Context::cast(frame->context());
-
-  // Check for eval() invocations that cross environments. Use the
-  // top frames receiver if evaluating in current environment.
-  Context* global_context = Top::context()->global()->global_context();
-  if (caller->global_context() == global_context) {
-    return frame->receiver();
-  }
-
-  // Otherwise use the given argument (the global object of the
-  // receiving context).
-  return args[0];
-}
-
-
 static Object* Runtime_GlobalReceiver(Arguments args) {
   ASSERT(args.length() == 1);
   Object* global = args[0];
@@ -3991,37 +3940,100 @@ static Object* Runtime_GlobalReceiver(Arguments args) {
 
 static Object* Runtime_CompileString(Arguments args) {
   HandleScope scope;
-  ASSERT(args.length() == 3);
+  ASSERT(args.length() == 2);
   CONVERT_ARG_CHECKED(String, source, 0);
   CONVERT_ARG_CHECKED(Smi, line_offset, 1);
-  bool contextual = args[2]->IsTrue();
-  RUNTIME_ASSERT(contextual || args[2]->IsFalse());
-
-  // Compute the eval context.
-  Handle<Context> context;
-  if (contextual) {
-    // Get eval context. May not be available if we are calling eval
-    // through an alias, and the corresponding frame doesn't have a
-    // proper eval context set up.
-    Object* eval_context = EvalContext();
-    if (eval_context->IsFailure()) return eval_context;
-    context = Handle<Context>(Context::cast(eval_context));
-  } else {
-    context = Handle<Context>(Top::context()->global_context());
-  }
-
 
   // Compile source string.
-  bool is_global = context->IsGlobalContext();
   Handle<JSFunction> boilerplate =
-      Compiler::CompileEval(source, line_offset->value(), is_global);
+      Compiler::CompileEval(source, line_offset->value(), true);
   if (boilerplate.is_null()) return Failure::Exception();
+  Handle<Context> context(Top::context()->global_context());
   Handle<JSFunction> fun =
       Factory::NewFunctionFromBoilerplate(boilerplate, context);
   return *fun;
 }
 
 
+static Object* Runtime_ExecDirectEval(Arguments args) {
+  ASSERT(args.length() == 1);
+
+  if (!args[0]->IsString()) return args[0];
+
+  Handle<String> source = args.at<String>(0);
+
+  // Compute the eval context.
+  HandleScope scope;
+  StackFrameLocator locator;
+  JavaScriptFrame* frame = locator.FindJavaScriptFrame(1);
+  Handle<Context> context(Context::cast(frame->context()));
+  bool is_global = context->IsGlobalContext();
+
+  // Compile source string.
+  Handle<JSFunction> boilerplate =
+      Compiler::CompileEval(source, 0, is_global);
+  if (boilerplate.is_null()) return Failure::Exception();
+  Handle<JSFunction> fun =
+      Factory::NewFunctionFromBoilerplate(boilerplate, context);
+
+  // Call generated function
+  Handle<Object> receiver(frame->receiver());
+  bool has_pending_exception = false;
+  Handle<Object> result =
+      Execution::Call(fun, receiver, 0, NULL, &has_pending_exception);
+  if (has_pending_exception) {
+    ASSERT(Top::has_pending_exception());
+    return Failure::Exception();
+  }
+  return *result;
+}
+
+
+static Object* Runtime_SetInPotentiallyDirectEval(Arguments args) {
+  ASSERT(args.length() == 0);
+  Top::set_in_potentially_direct_eval(true);
+  return Heap::undefined_value();
+}
+
+
+static Object* Runtime_ClearInPotentiallyDirectEval(Arguments args) {
+  ASSERT(args.length() == 0);
+  Top::set_in_potentially_direct_eval(false);
+  return Heap::undefined_value();
+}
+
+
+static Object* Runtime_InDirectEval(Arguments args) {
+  ASSERT(args.length() == 0);
+
+  // No need to look further if the static analysis showed that this
+  // is aliased.
+  if (!Top::is_in_potentially_direct_eval()) {
+    return Heap::false_value();
+  }
+
+  // Find where the 'eval' symbol is bound. It is unaliased only if
+  // it is bound in the global object.
+  HandleScope scope;
+  StackFrameLocator locator;
+  JavaScriptFrame* frame = locator.FindJavaScriptFrame(1);
+  Context* context = Context::cast(frame->context());
+  int index;
+  PropertyAttributes attributes;
+  while (context && !context->IsGlobalContext()) {
+    context->Lookup(Factory::eval_symbol(), FOLLOW_PROTOTYPE_CHAIN,
+                    &index, &attributes);
+    if (attributes != ABSENT) return Heap::false_value();
+    if (context->is_function_context()) {
+      context = Context::cast(context->closure()->context());
+    } else {
+      context = context->previous();
+    }
+  }
+  return Heap::true_value();
+}
+
+
 static Object* Runtime_CompileScript(Arguments args) {
   HandleScope scope;
   ASSERT(args.length() == 4);

src/runtime.h

@@ -190,13 +190,16 @@ namespace v8 { namespace internal {
   F(NumberIsFinite, 1) \
   \
   /* Globals */ \
-  F(CompileString, 3) \
+  F(CompileString, 2) \
   F(CompileScript, 4) \
   F(GlobalPrint, 1) \
   \
   /* Eval */ \
-  F(EvalReceiver, 1) \
   F(GlobalReceiver, 1) \
+  F(SetInPotentiallyDirectEval, 0) \
+  F(ClearInPotentiallyDirectEval, 0) \
+  F(InDirectEval, 0) \
+  F(ExecDirectEval, 1) \
   \
   F(SetProperty, -1 /* 3 or 4 */) \
   F(IgnoreAttributesAndSetProperty, -1 /* 3 or 4 */) \

src/top.cc

@@ -102,6 +102,7 @@ void Top::InitializeThreadLocal() {
   clear_scheduled_exception();
   thread_local_.save_context_ = NULL;
   thread_local_.catcher_ = NULL;
+  thread_local_.in_potentially_direct_eval_ = false;
 }
 
 

src/top.h

@@ -67,6 +67,10 @@ class ThreadLocalTop BASE_EMBEDDED {
 
   // Call back function to report unsafe JS accesses.
   v8::FailedAccessCheckCallback failed_access_check_callback_;
+
+  // Flag for whether we are currently in a call that the static analysis
+  // marked as a potentially direct eval.
+  bool in_potentially_direct_eval_;
 };
 
 #define TOP_ADDRESS_LIST(C) \
@@ -165,6 +169,14 @@ class Top {
   }
   static inline Address* handler_address() { return &thread_local_.handler_; }
 
+  static inline void set_in_potentially_direct_eval(bool b) {
+    thread_local_.in_potentially_direct_eval_ = b;
+  }
+
+  static inline bool is_in_potentially_direct_eval() {
+    return thread_local_.in_potentially_direct_eval_;
+  }
+
   // Generated code scratch locations.
   static void* formal_count_address() { return &thread_local_.formal_count_; }
 

src/v8natives.js

@@ -101,7 +101,6 @@ function GlobalParseFloat(string) {
   return %StringParseFloat(ToString(string));
 }
 
-
 function GlobalEval(x) {
   if (!IS_STRING(x)) return x;
 
@@ -110,10 +109,14 @@ function GlobalEval(x) {
                          'be the global object from which eval originated');
   }
   
-  var f = %CompileString(x, 0, true);
+  if (%InDirectEval()) {
+    return %ExecDirectEval(x);
+  }
+  
+  var f = %CompileString(x, 0);
   if (!IS_FUNCTION(f)) return f;
 
-  return f.call(%EvalReceiver(this));
+  return f.call(this);
 }
 
 
@@ -121,7 +124,7 @@ function GlobalEval(x) {
 function GlobalExecScript(expr, lang) {
   // NOTE: We don't care about the character casing.
   if (!lang || /javascript/i.test(lang)) {
-    var f = %CompileString(ToString(expr), 0, false);
+    var f = %CompileString(ToString(expr), 0);
     f.call(%GlobalReceiver(global));
   }
   return null;
@@ -140,7 +143,7 @@ function SetupGlobal() {
 
   // ECMA-262 - 15.1.1.3.
   %SetProperty(global, "undefined", void 0, DONT_ENUM | DONT_DELETE);
-
+  
   // Setup non-enumerable function on the global object.
   InstallFunctions(global, DONT_ENUM, $Array(
     "isNaN", GlobalIsNaN,
@@ -521,7 +524,7 @@ function NewFunction(arg1) {  // length == 1
 
   // The call to SetNewFunctionAttributes will ensure the prototype
   // property of the resulting function is enumerable (ECMA262, 15.3.5.2).
-  var f = %CompileString(source, -1, false)();
+  var f = %CompileString(source, -1)();
   %FunctionSetName(f, "anonymous");
   return %SetNewFunctionAttributes(f);
 }

test/cctest/test-api.cc

@@ -4022,7 +4022,7 @@ THREADED_TEST(Eval) {
   v8::HandleScope scope;
   LocalContext current;
 
-  // Test that un-aliased eval uses local context.
+  // Test that un-aliased eval reads from local context.
   Local<Script> script =
       Script::Compile(v8_str("foo = 0;"
                              "(function() {"
@@ -4032,6 +4032,19 @@ THREADED_TEST(Eval) {
   Local<Value> result = script->Run();
   CHECK_EQ(2, result->Int32Value());
 
+  // Test that un-aliased eval writes to local context.
+  script =
+      Script::Compile(v8_str("foo = 0;"
+                             "(function() {"
+                             "  var foo = 1;"
+                             "  eval('foo = 2;');"
+                             "  return foo;"
+                             "})();"));
+  result = script->Run();
+  CHECK_EQ(0, current->Global()->Get(v8_str("foo"))->Int32Value());
+  CHECK(result->IsInt32());
+  CHECK_EQ(2, result->Int32Value());
+
   // Test that un-aliased eval has right this.
   script =
       Script::Compile(v8_str("function MyObject() { this.self = eval('this'); }"
@@ -4039,6 +4052,120 @@ THREADED_TEST(Eval) {
                              "o === o.self"));
   result = script->Run();
   CHECK(result->IsTrue());
+
+  // Test that aliased eval reads from global context.
+  script =
+      Script::Compile(v8_str("var e = eval;"
+                             "foo = 0;"
+                             "(function() {"
+                             "  var foo = 2;"
+                             "  return e('foo');"
+                             "})();"));
+  result = script->Run();
+  CHECK_EQ(0, result->Int32Value());
+
+  // Test that aliased eval writes to global context.
+  script =
+      Script::Compile(v8_str("var e = eval;"
+                             "foo = 0;"
+                             "(function() {"
+                             "  e('var foo = 2;');"
+                             "})();"
+                             "foo"));
+  result = script->Run();
+  CHECK_EQ(2, result->Int32Value());
+
+  // Test that aliased eval has right this.
+  script =
+      Script::Compile(v8_str("var e = eval;"
+                             "function MyObject() { this.self = e('this'); }"
+                             "var o = new MyObject();"
+                             "this === o.self"));
+  result = script->Run();
+  CHECK(result->IsTrue());
+
+  // Try to cheat the 'aliased eval' detection.
+  script =
+      Script::Compile(v8_str("var x = this;"
+                             "foo = 0;"
+                             "(function() {"
+                             "  var foo = 2;"
+                             "  return x.eval('foo');"
+                             "})();"));
+  result = script->Run();
+  CHECK_EQ(0, result->Int32Value());
+
+  script =
+      Script::Compile(v8_str("var e = eval;"
+                             "foo = 0;"
+                             "(function() {"
+                             "  var eval = function(x) { return x; };"
+                             "  var foo = eval(2);"
+                             "  return e('foo');"
+                             "})();"));
+  result = script->Run();
+  CHECK_EQ(0, result->Int32Value());
+
+  script =
+      Script::Compile(v8_str("(function() {"
+                             "  var eval = function(x) { return 2 * x; };"
+                             "  return (function() { return eval(2); })();"
+                             "})();"));
+  result = script->Run();
+  CHECK_EQ(4, result->Int32Value());
+
+  script =
+      Script::Compile(v8_str("eval = function(x) { return 2 * x; };"
+                             "(function() {"
+                             "  return (function() { return eval(2); })();"
+                             "})();"));
+  result = script->Run();
+  CHECK_EQ(4, result->Int32Value());
+}
+
+
+THREADED_TEST(EvalAliasedDynamic) {
+  v8::HandleScope scope;
+  LocalContext current;
+
+  // This sets 'global' to the real global object (as opposed to the
+  // proxy). It is highly implementation dependent, so take care.
+  current->Global()->Set(v8_str("global"), current->Global()->GetPrototype());
+
+  // Tests where aliased eval can only be resolved dynamically.
+  Local<Script> script =
+      Script::Compile(v8_str("function f(x) { "
+                             "  var foo = 2;"
+                             "  with (x) { return eval('foo'); }"
+                             "}"
+                             "foo = 0;"
+                             "var result = f(new Object()) == 2;"
+                             "result = result && (f(global) == 0);"
+                             "var x = new Object();"
+                             "x.eval = function(x) { return 1; };"
+                             "result = result && (f(x) == 1);"
+                             "result"));
+  Local<Value> result = script->Run();
+  CHECK(result->IsTrue());
+
+  v8::TryCatch try_catch;
+  script =
+    Script::Compile(v8_str("function f(x) { "
+                           "  var bar = 2;"
+                           "  with (x) { return eval('bar'); }"
+                           "}"
+                           "f(global)"));
+  result = script->Run();
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+
+  script =
+    Script::Compile(v8_str("this.eval = function (x) { return x + x; };"
+                           "foo = 2;"
+                           "eval('foo')"));
+  result = script->Run();
+  CHECK(result->IsString());
+  CHECK_EQ(v8_str("foofoo"), result);
 }