Skip to content

Commit

Permalink
Ensure there is a smi check of the receiver for global load and call …
Browse files Browse the repository at this point in the history
…ICs.

There was a comment that, for such ICs specialized to the global object,
they were always contextual loads.  This is very brittle.  It is a
micro-optimization that relies too much on the way that things happen to
work today.

Instead, never omit the smi check because it's safer.

R=vegorov@chromium.org
BUG=117794
TEST=regress-117794.js

Review URL: https://chromiumcodereview.appspot.com/9691038

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@11022 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
  • Loading branch information
kmillikin@chromium.org committed Mar 13, 2012
1 parent 235e9e2 commit 7d6fd56
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 54 deletions.
16 changes: 2 additions & 14 deletions src/arm/stub-cache-arm.cc
Expand Up @@ -1387,14 +1387,8 @@ void CallStubCompiler::GenerateGlobalReceiverCheck(Handle<JSObject> object,
// Get the receiver from the stack.
__ ldr(r0, MemOperand(sp, argc * kPointerSize));

// If the object is the holder then we know that it's a global
// object which can only happen for contextual calls. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(r0, miss);
}

// Check that the maps haven't changed.
__ JumpIfSmi(r0, miss);
CheckPrototypes(object, r0, holder, r3, r1, r4, name, miss);
}

Expand Down Expand Up @@ -2813,14 +2807,8 @@ Handle<Code> LoadStubCompiler::CompileLoadGlobal(
// -----------------------------------
Label miss;

// If the object is the holder then we know that it's a global
// object which can only happen for contextual calls. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(r0, &miss);
}

// Check that the map of the global has not changed.
__ JumpIfSmi(r0, &miss);
CheckPrototypes(object, r0, holder, r3, r4, r1, name, &miss);

// Get the value from the cell.
Expand Down
15 changes: 2 additions & 13 deletions src/ia32/stub-cache-ia32.cc
Expand Up @@ -1245,14 +1245,9 @@ void CallStubCompiler::GenerateGlobalReceiverCheck(Handle<JSObject> object,
// Get the receiver from the stack.
__ mov(edx, Operand(esp, (argc + 1) * kPointerSize));

// If the object is the holder then we know that it's a global
// object which can only happen for contextual calls. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(edx, miss);
}

// Check that the maps haven't changed.
__ JumpIfSmi(edx, miss);
CheckPrototypes(object, edx, holder, ebx, eax, edi, name, miss);
}

Expand Down Expand Up @@ -2829,14 +2824,8 @@ Handle<Code> LoadStubCompiler::CompileLoadGlobal(
// -----------------------------------
Label miss;

// If the object is the holder then we know that it's a global
// object which can only happen for contextual loads. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(eax, &miss);
}

// Check that the maps haven't changed.
__ JumpIfSmi(eax, &miss);
CheckPrototypes(object, eax, holder, ebx, edx, edi, name, &miss);

// Get the value from the cell.
Expand Down
16 changes: 2 additions & 14 deletions src/mips/stub-cache-mips.cc
Expand Up @@ -1394,14 +1394,8 @@ void CallStubCompiler::GenerateGlobalReceiverCheck(Handle<JSObject> object,
// Get the receiver from the stack.
__ lw(a0, MemOperand(sp, argc * kPointerSize));

// If the object is the holder then we know that it's a global
// object which can only happen for contextual calls. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(a0, miss);
}

// Check that the maps haven't changed.
__ JumpIfSmi(a0, miss);
CheckPrototypes(object, a0, holder, a3, a1, t0, name, miss);
}

Expand Down Expand Up @@ -2819,14 +2813,8 @@ Handle<Code> LoadStubCompiler::CompileLoadGlobal(
// -----------------------------------
Label miss;

// If the object is the holder then we know that it's a global
// object which can only happen for contextual calls. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(a0, &miss);
}

// Check that the map of the global has not changed.
__ JumpIfSmi(a0, &miss);
CheckPrototypes(object, a0, holder, a3, t0, a1, name, &miss);

// Get the value from the cell.
Expand Down
15 changes: 2 additions & 13 deletions src/x64/stub-cache-x64.cc
Expand Up @@ -1224,14 +1224,9 @@ void CallStubCompiler::GenerateGlobalReceiverCheck(Handle<JSObject> object,
// Get the receiver from the stack.
__ movq(rdx, Operand(rsp, (argc + 1) * kPointerSize));

// If the object is the holder then we know that it's a global
// object which can only happen for contextual calls. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(rdx, miss);
}

// Check that the maps haven't changed.
__ JumpIfSmi(rdx, miss);
CheckPrototypes(object, rdx, holder, rbx, rax, rdi, name, miss);
}

Expand Down Expand Up @@ -2665,14 +2660,8 @@ Handle<Code> LoadStubCompiler::CompileLoadGlobal(
// -----------------------------------
Label miss;

// If the object is the holder then we know that it's a global
// object which can only happen for contextual loads. In this case,
// the receiver cannot be a smi.
if (!object.is_identical_to(holder)) {
__ JumpIfSmi(rax, &miss);
}

// Check that the maps haven't changed.
__ JumpIfSmi(rax, &miss);
CheckPrototypes(object, rax, holder, rbx, rdx, rdi, name, &miss);

// Get the value from the cell.
Expand Down
57 changes: 57 additions & 0 deletions test/mjsunit/regress/regress-117794.js
@@ -0,0 +1,57 @@
// Copyright 2012 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.

// Loads specialized to be from the global object should not omit the
// smi check on the receiver. The code below should not crash.

print = function() {}

function constructor() {};

function assertHasOwnProperties(object, limit) {
for (var i = 0; i < limit; i++) { }
}

try {
Object.keys();
} catch(exc2) {
print(exc2.stack);
}

var x1 = new Object();

try {
new Function("A Man Called Horse", x1.d);
} catch(exc3) {
print(exc3.stack);
}

try {
(-(true)).toPrecision(0x30, 'lib1-f1');
} catch(exc1) {
print(exc1.stack);
}

0 comments on commit 7d6fd56

Please sign in to comment.