From 7e8a62e34a827d7d9a505521cdda6ee7c09ee58a Mon Sep 17 00:00:00 2001 From: mstarzinger Date: Tue, 17 Mar 2015 05:36:57 -0700 Subject: [PATCH] [turbofan] Fix C++ evaluation order in AstGraphBuilder. The evaluation order of receiver versus arguments is not properly defined by C++. This caused issues with Clang where the environment changed after the receiveing environment was already loaded. R=jarin@chromium.org BUG=chromium:467531 TEST=mjsunit/regress/regress-crbug-467531 LOG=N Review URL: https://codereview.chromium.org/1015683002 Cr-Commit-Position: refs/heads/master@{#27238} --- src/compiler/ast-graph-builder.cc | 17 ++++++++----- test/mjsunit/regress/regress-crbug-467531.js | 25 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 test/mjsunit/regress/regress-crbug-467531.js diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc index 00d09f3d84..daced7afbf 100644 --- a/src/compiler/ast-graph-builder.cc +++ b/src/compiler/ast-graph-builder.cc @@ -1395,9 +1395,10 @@ void AstGraphBuilder::VisitTryFinallyStatement(TryFinallyStatement* stmt) { // The result value, dispatch token and message is expected on the operand // stack (this is in sync with FullCodeGenerator::EnterFinallyBlock). + Node* message = BuildLoadExternal(message_object, kMachAnyTagged); environment()->Push(token); // TODO(mstarzinger): Cook token! environment()->Push(result); - environment()->Push(BuildLoadExternal(message_object, kMachAnyTagged)); + environment()->Push(message); // Evaluate the finally-block. Visit(stmt->finally_block()); @@ -1405,9 +1406,10 @@ void AstGraphBuilder::VisitTryFinallyStatement(TryFinallyStatement* stmt) { // The result value, dispatch token and message is restored from the operand // stack (this is in sync with FullCodeGenerator::ExitFinallyBlock). - BuildStoreExternal(message_object, kMachAnyTagged, environment()->Pop()); + message = environment()->Pop(); result = environment()->Pop(); token = environment()->Pop(); // TODO(mstarzinger): Uncook token! + BuildStoreExternal(message_object, kMachAnyTagged, message); // Dynamic dispatch after the finally-block. commands->ApplyDeferredCommands(token, result); @@ -1739,8 +1741,9 @@ void AstGraphBuilder::VisitObjectLiteral(ObjectLiteral* expr) { environment()->Push(literal); // Duplicate receiver. VisitForValue(property->key()); - environment()->Push(BuildToName(environment()->Pop(), - expr->GetIdForProperty(property_index))); + Node* name = BuildToName(environment()->Pop(), + expr->GetIdForProperty(property_index)); + environment()->Push(name); // TODO(mstarzinger): For ObjectLiteral::Property::PROTOTYPE the key should // not be on the operand stack while the value is being evaluated. Come up // with a repro for this and fix it. Also find a nice way to do so. :) @@ -2564,7 +2567,8 @@ Node* AstGraphBuilder::BuildPatchReceiverToGlobalProxy(Node* receiver) { Node* check = NewNode(javascript()->StrictEqual(), receiver, undefined); receiver_check.If(check); receiver_check.Then(); - environment()->Push(BuildLoadGlobalProxy()); + Node* proxy = BuildLoadGlobalProxy(); + environment()->Push(proxy); receiver_check.Else(); environment()->Push(receiver); receiver_check.End(); @@ -2663,7 +2667,8 @@ Node* AstGraphBuilder::BuildHoleCheckThrow(Node* value, Variable* variable, Node* check = NewNode(javascript()->StrictEqual(), value, the_hole); hole_check.If(check); hole_check.Then(); - environment()->Push(BuildThrowReferenceError(variable, bailout_id)); + Node* error = BuildThrowReferenceError(variable, bailout_id); + environment()->Push(error); hole_check.Else(); environment()->Push(not_hole); hole_check.End(); diff --git a/test/mjsunit/regress/regress-crbug-467531.js b/test/mjsunit/regress/regress-crbug-467531.js new file mode 100644 index 0000000000..73256c7acc --- /dev/null +++ b/test/mjsunit/regress/regress-crbug-467531.js @@ -0,0 +1,25 @@ +// Copyright 2015 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. + +// Flags: --turbo-filter=* --always-opt + +assertThrows(function() { + "use strict"; + try { + x = ref_error; + let x = 0; + } catch (e) { + throw e; + } +}, ReferenceError); + +assertThrows(function() { + "use strict"; + try { + x = ref_error; + let x = 0; + } finally { + // re-throw + } +}, ReferenceError);