- Revision
- 147816
- Author
- mhahnenb...@apple.com
- Date
- 2013-04-05 16:52:20 -0700 (Fri, 05 Apr 2013)
Log Message
tryCacheGetByID sets StructureStubInfo accessType to an incorrect value
https://bugs.webkit.org/show_bug.cgi?id=114068
Reviewed by Geoffrey Garen.
In the case where we have a non-Value cacheable property, we set the StructureStubInfo accessType to
get_by_id_self, but then we don't patch self and instead patch in a get_by_id_self_fail. This leads to
incorrect profiling data so when the DFG compiles the function, it uses a GetByOffset rather than a GetById,
which leads to loading a GetterSetter directly out of an object.
Source/_javascript_Core:
* jit/JITStubs.cpp:
(JSC::tryCacheGetByID):
(JSC::DEFINE_STUB_FUNCTION):
LayoutTests:
* fast/js/jit-set-profiling-access-type-only-for-get-by-id-self-expected.txt: Added.
* fast/js/jit-set-profiling-access-type-only-for-get-by-id-self.html: Added.
* fast/js/script-tests/jit-set-profiling-access-type-only-for-get-by-id-self.js: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (147815 => 147816)
--- trunk/LayoutTests/ChangeLog 2013-04-05 23:51:15 UTC (rev 147815)
+++ trunk/LayoutTests/ChangeLog 2013-04-05 23:52:20 UTC (rev 147816)
@@ -1,3 +1,19 @@
+2013-04-05 Mark Hahnenberg <mhahnenb...@apple.com>
+
+ tryCacheGetByID sets StructureStubInfo accessType to an incorrect value
+ https://bugs.webkit.org/show_bug.cgi?id=114068
+
+ Reviewed by Geoffrey Garen.
+
+ In the case where we have a non-Value cacheable property, we set the StructureStubInfo accessType to
+ get_by_id_self, but then we don't patch self and instead patch in a get_by_id_self_fail. This leads to
+ incorrect profiling data so when the DFG compiles the function, it uses a GetByOffset rather than a GetById,
+ which leads to loading a GetterSetter directly out of an object.
+
+ * fast/js/jit-set-profiling-access-type-only-for-get-by-id-self-expected.txt: Added.
+ * fast/js/jit-set-profiling-access-type-only-for-get-by-id-self.html: Added.
+ * fast/js/script-tests/jit-set-profiling-access-type-only-for-get-by-id-self.js: Added.
+
2013-04-05 Chris Fleizach <cfleiz...@apple.com>
AX: Make SVG Group containers accessible elements
Added: trunk/LayoutTests/fast/js/jit-set-profiling-access-type-only-for-get-by-id-self-expected.txt (0 => 147816)
--- trunk/LayoutTests/fast/js/jit-set-profiling-access-type-only-for-get-by-id-self-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/jit-set-profiling-access-type-only-for-get-by-id-self-expected.txt 2013-04-05 23:52:20 UTC (rev 147816)
@@ -0,0 +1,208 @@
+Tests that we use the correct profiling accessType for the case we end up compiling/patching.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(new L_()) is 1365109051943.000000
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(new L_()) is 1365109051943.000000
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(new L_()) is 1365109051943.000000
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(new L_()) is 1365109051943.000000
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(new L_()) is 1365109051943.000000
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(new L_()) is 1365109051943.000000
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS f(129681120) is 129681120
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/jit-set-profiling-access-type-only-for-get-by-id-self.html (0 => 147816)
--- trunk/LayoutTests/fast/js/jit-set-profiling-access-type-only-for-get-by-id-self.html (rev 0)
+++ trunk/LayoutTests/fast/js/jit-set-profiling-access-type-only-for-get-by-id-self.html 2013-04-05 23:52:20 UTC (rev 147816)
@@ -0,0 +1,10 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Added: trunk/LayoutTests/fast/js/script-tests/jit-set-profiling-access-type-only-for-get-by-id-self.js (0 => 147816)
--- trunk/LayoutTests/fast/js/script-tests/jit-set-profiling-access-type-only-for-get-by-id-self.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/jit-set-profiling-access-type-only-for-get-by-id-self.js 2013-04-05 23:52:20 UTC (rev 147816)
@@ -0,0 +1,20 @@
+description(
+"Tests that we use the correct profiling accessType for the case we end up compiling/patching."
+);
+
+function L_() {
+ this.__defineGetter__("mg", function() { return 1365109051943.000000; });
+};
+
+function Q2() {};
+Q2.mg = 42;
+
+var f = function (Sk){ if(Sk instanceof L_){ return Sk.mg; }else if(Sk instanceof Q2){ return Sk.mg; }else{ return Number(Sk); } }
+
+for (var i = 1; i < 200; i++) {
+ if (i % 30 == 0)
+ shouldBe("f(new L_())", "1365109051943.000000");
+ else
+ shouldBe("f(129681120)", "129681120");
+}
+
Modified: trunk/Source/_javascript_Core/ChangeLog (147815 => 147816)
--- trunk/Source/_javascript_Core/ChangeLog 2013-04-05 23:51:15 UTC (rev 147815)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-04-05 23:52:20 UTC (rev 147816)
@@ -1,3 +1,19 @@
+2013-04-05 Mark Hahnenberg <mhahnenb...@apple.com>
+
+ tryCacheGetByID sets StructureStubInfo accessType to an incorrect value
+ https://bugs.webkit.org/show_bug.cgi?id=114068
+
+ Reviewed by Geoffrey Garen.
+
+ In the case where we have a non-Value cacheable property, we set the StructureStubInfo accessType to
+ get_by_id_self, but then we don't patch self and instead patch in a get_by_id_self_fail. This leads to
+ incorrect profiling data so when the DFG compiles the function, it uses a GetByOffset rather than a GetById,
+ which leads to loading a GetterSetter directly out of an object.
+
+ * jit/JITStubs.cpp:
+ (JSC::tryCacheGetByID):
+ (JSC::DEFINE_STUB_FUNCTION):
+
2013-04-05 Filip Pizlo <fpi...@apple.com>
If CallFrame::trueCallFrame() knows that it's about to read garbage instead of a valid CodeOrigin/InlineCallFrame, then it should give up and return 0 and all callers should be robust against this
Modified: trunk/Source/_javascript_Core/jit/JITStubs.cpp (147815 => 147816)
--- trunk/Source/_javascript_Core/jit/JITStubs.cpp 2013-04-05 23:51:15 UTC (rev 147815)
+++ trunk/Source/_javascript_Core/jit/JITStubs.cpp 2013-04-05 23:52:20 UTC (rev 147816)
@@ -940,12 +940,14 @@
// Cache hit: Specialize instruction and ref Structures.
if (slot.slotBase() == baseValue) {
- stubInfo->initGetByIdSelf(callFrame->globalData(), codeBlock->ownerExecutable(), structure);
+ RELEASE_ASSERT(stubInfo->accessType == access_unset);
if ((slot.cachedPropertyType() != PropertySlot::Value)
|| !MacroAssembler::isCompactPtrAlignedAddressOffset(maxOffsetRelativeToPatchedStorage(slot.cachedOffset())))
ctiPatchCallByReturnAddress(codeBlock, returnAddress, FunctionPtr(cti_op_get_by_id_self_fail));
- else
+ else {
JIT::patchGetByIdSelf(codeBlock, stubInfo, structure, slot.cachedOffset(), returnAddress);
+ stubInfo->initGetByIdSelf(callFrame->globalData(), codeBlock->ownerExecutable(), structure);
+ }
return;
}
@@ -1592,6 +1594,9 @@
PolymorphicAccessStructureList* polymorphicStructureList;
int listIndex = 1;
+ if (stubInfo->accessType == access_unset)
+ stubInfo->initGetByIdSelf(callFrame->globalData(), codeBlock->ownerExecutable(), baseValue.asCell()->structure());
+
if (stubInfo->accessType == access_get_by_id_self) {
ASSERT(!stubInfo->stubRoutine);
polymorphicStructureList = new PolymorphicAccessStructureList(callFrame->globalData(), codeBlock->ownerExecutable(), 0, stubInfo->u.getByIdSelf.baseObjectStructure.get(), true);
