Implement new Function.prototype.toString and fix CreateDynamicFunction parsing

src/compiler.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/compiler.h"
 
 #include <algorithm>
 #include <memory>
 
 #include "src/asmjs/asm-js.h"
@@ skipping 1247 matching lines @@
       return BASELINE;
     }
   } else {
     return OPTIMIZED;
   }
 }
 
 MaybeHandle<JSFunction> Compiler::GetFunctionFromEval(
     Handle<String> source, Handle<SharedFunctionInfo> outer_info,
     Handle<Context> context, LanguageMode language_mode,
-    ParseRestriction restriction, int eval_scope_position, int eval_position,
-    int line_offset, int column_offset, Handle<Object> script_name,
+    ParseRestriction restriction, int parameters_end_pos,
+    int eval_scope_position, int eval_position, int line_offset,
+    int column_offset, Handle<Object> script_name,
     ScriptOriginOptions options) {
   Isolate* isolate = source->GetIsolate();
   int source_length = source->length();
   isolate->counters()->total_eval_size()->Increment(source_length);
   isolate->counters()->total_compile_size()->Increment(source_length);
 
+  // The cache lookup key needs to be aware of the separation between the
+  // parameters and the body to prevent this valid invocation:
+  //   Function("", "function anonymous(\n/**/) {\n}");
+  // from adding an entry that falsely approves this invalid invocation:
+  //   Function("\n/**/) {\nfunction anonymous(", "}");
+  // The actual eval_scope_position for indirect eval and CreateDynamicFunction
+  // is unused (just 0), which means it's an available field to use to indicate
+  // this separation. But to make sure we're not causing other false hits, we
+  // negate the scope position.
+  if (FLAG_harmony_function_tostring &&
+      restriction == ONLY_SINGLE_FUNCTION_LITERAL &&
+      parameters_end_pos != kNoSourcePosition) {
+    // use the parameters_end_pos as the eval_scope_position in the eval cache.
+    DCHECK_EQ(eval_scope_position, 0);
+    eval_scope_position = -parameters_end_pos;

[Dan Ehrenberg, 2016/12/06 00:32:13]

This seems like a valid implementation strategy, taking advantage of the way
that the cache stores a tuple in a nice way to reduce code duplication. However,
it's a little confusing to a casual reader which might not start looking through
the code from this well-written comment. The only other comment I found that
documents what eval_scope_position is for was in
CompilationCacheTable::LookupEval in src/objects.cc; at least that comment
should be updated. It may also be a good idea to change the name of this
parameter ("position"? or something generic to indicate that this is an
otherwise-uninterpreted token? I am not sure what would be best) to indicate its
multiple uses.

[jwolfe, 2017/01/13 00:28:47]

I added documentation to the StringSharedKey constructor and removed the comment
in objects.cc.
I have not done this yet. I'll rebase before renaming anything.

+  }
   CompilationCache* compilation_cache = isolate->compilation_cache();
   MaybeHandle<SharedFunctionInfo> maybe_shared_info =
       compilation_cache->LookupEval(source, outer_info, context, language_mode,
                                     eval_scope_position);
   Handle<SharedFunctionInfo> shared_info;
 
   Handle<Script> script;
   if (!maybe_shared_info.ToHandle(&shared_info)) {
     script = isolate->factory()->NewScript(source);
     if (FLAG_trace_deopt) Script::InitLineEnds(script);
     if (!script_name.is_null()) {
       script->set_name(*script_name);
       script->set_line_offset(line_offset);
       script->set_column_offset(column_offset);
     }
     script->set_origin_options(options);
     script->set_compilation_type(Script::COMPILATION_TYPE_EVAL);
     Script::SetEvalOrigin(script, outer_info, eval_position);
 
     Zone zone(isolate->allocator(), ZONE_NAME);
     ParseInfo parse_info(&zone, script);
     CompilationInfo info(&parse_info, Handle<JSFunction>::null());
     parse_info.set_eval();
     parse_info.set_language_mode(language_mode);
     parse_info.set_parse_restriction(restriction);
+    parse_info.set_parameters_end_pos(parameters_end_pos);
     if (!context->IsNativeContext()) {
       parse_info.set_outer_scope_info(handle(context->scope_info()));
     }
 
     shared_info = CompileToplevel(&info);
 
     if (shared_info.is_null()) {
       return MaybeHandle<JSFunction>();
     } else {
       // If caller is strict mode, the result must be in strict mode as well.
@@ skipping 31 matching lines @@
     // Callback set. Let it decide if code generation is allowed.
     VMState<EXTERNAL> state(isolate);
     return callback(v8::Utils::ToLocal(context));
   }
 }
 
 }  // namespace
 
 MaybeHandle<JSFunction> Compiler::GetFunctionFromString(
     Handle<Context> context, Handle<String> source,
-    ParseRestriction restriction) {
+    ParseRestriction restriction, int parameters_end_pos) {
   Isolate* const isolate = context->GetIsolate();
   Handle<Context> native_context(context->native_context(), isolate);
 
   // Check if native context allows code generation from
   // strings. Throw an exception if it doesn't.
   if (native_context->allow_code_gen_from_strings()->IsFalse(isolate) &&
       !CodeGenerationFromStringsAllowed(isolate, native_context)) {
     Handle<Object> error_message =
         native_context->ErrorMessageForCodeGenerationFromStrings();
     THROW_NEW_ERROR(isolate, NewEvalError(MessageTemplate::kCodeGenFromStrings,
                                           error_message),
                     JSFunction);
   }
 
   // Compile source string in the native context.
   int eval_scope_position = 0;
   int eval_position = kNoSourcePosition;
   Handle<SharedFunctionInfo> outer_info(native_context->closure()->shared());
   return Compiler::GetFunctionFromEval(source, outer_info, native_context,
-                                       SLOPPY, restriction, eval_scope_position,
-                                       eval_position);
+                                       SLOPPY, restriction, parameters_end_pos,
+                                       eval_scope_position, eval_position);
 }
 
 Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfoForScript(
     Handle<String> source, Handle<Object> script_name, int line_offset,
     int column_offset, ScriptOriginOptions resource_options,
     Handle<Object> source_map_url, Handle<Context> context,
     v8::Extension* extension, ScriptData** cached_data,
     ScriptCompiler::CompileOptions compile_options, NativesFlag natives,
     bool is_module) {
   Isolate* isolate = source->GetIsolate();
@@ skipping 339 matching lines @@
     DCHECK(shared->is_compiled());
     function->set_literals(cached.literals);
   } else if (shared->is_compiled()) {
     // TODO(mvstanton): pass pretenure flag to EnsureLiterals.
     JSFunction::EnsureLiterals(function);
   }
 }
 
 }  // namespace internal
 }  // namespace v8