Skip to content

Commit d3b8e12

Browse files
tadeuzagallotadeuzagallo
committed
Baseline version of get_by_id may corrupt metadata
https://bugs.webkit.org/show_bug.cgi?id=193085 <rdar://problem/23453006> Reviewed by Saam Barati. JSTests: * stress/get-by-id-change-mode.js: Added. (forEach): Source/JavaScriptCore: The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell` if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things: 1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by `CodeBlock::getOrAddArrayProfile`. 2) get_by_id's mode *is* ArrayLength and we generate the array profiling code pointing to the ArrayProfile that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens, the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode. Add a check to the Baseline version of get_by_id so that we only do the ArrayProfiling if the get_by_id's mode is ArrayLength * bytecode/BytecodeList.rb: * bytecode/CodeBlock.cpp: (JSC::CodeBlock::getArrayProfile): (JSC::CodeBlock::addArrayProfile): Deleted. (JSC::CodeBlock::getOrAddArrayProfile): Deleted. * bytecode/CodeBlock.h: (JSC::CodeBlock::numberOfArrayProfiles const): Deleted. (JSC::CodeBlock::arrayProfiles): Deleted. * bytecode/CodeBlockInlines.h: (JSC::CodeBlock::forEachArrayProfile): * jit/JIT.h: * jit/JITInlines.h: (JSC::JIT::emitArrayProfilingSiteForBytecodeIndexWithCell): Deleted. * jit/JITPropertyAccess.cpp: (JSC::JIT::emit_op_get_by_id): * jit/JITPropertyAccess32_64.cpp: (JSC::JIT::emit_op_get_by_id): * llint/LLIntSlowPaths.cpp: (JSC::LLInt::LLINT_SLOW_PATH_DECL): Canonical link: https://commits.webkit.org/207654@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239626 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 9ec7c34 commit d3b8e12

12 files changed

Lines changed: 79 additions & 55 deletions

File tree

‎JSTests/ChangeLog‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
2019-01-04 Tadeu Zagallo <tzagallo@apple.com>
2+
3+
Baseline version of get_by_id may corrupt metadata
4+
https://bugs.webkit.org/show_bug.cgi?id=193085
5+
<rdar://problem/23453006>
6+
7+
Reviewed by Saam Barati.
8+
9+
* stress/get-by-id-change-mode.js: Added.
10+
(forEach):
11+
112
2019-01-02 Yusuke Suzuki <yusukesuzuki@slowstart.org>
213

314
[JSC] Optimize Object.prototype.toString

‎JSTests/stress/get-by-id-change-mode.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//@ runDefault("--useConcurrentJIT=0")
2+
3+
forEach({ length: 5 }, function() {
4+
for (var i = 0; i < 10; i++) {
5+
forEach([1], function() {});
6+
}
7+
});
8+
9+
function forEach(a, b) {
10+
for (var c = 0; c < a.length; c++)
11+
b();
12+
}

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,43 @@
1+
2019-01-04 Tadeu Zagallo <tzagallo@apple.com>
2+
3+
Baseline version of get_by_id may corrupt metadata
4+
https://bugs.webkit.org/show_bug.cgi?id=193085
5+
<rdar://problem/23453006>
6+
7+
Reviewed by Saam Barati.
8+
9+
The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell`
10+
if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry
11+
in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things:
12+
1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by
13+
`CodeBlock::getOrAddArrayProfile`.
14+
2) get_by_id's mode *is* ArrayLength and we generate the array profiling code pointing to the ArrayProfile
15+
that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens,
16+
the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode.
17+
18+
Add a check to the Baseline version of get_by_id so that we only do the ArrayProfiling if the get_by_id's
19+
mode is ArrayLength
20+
21+
* bytecode/BytecodeList.rb:
22+
* bytecode/CodeBlock.cpp:
23+
(JSC::CodeBlock::getArrayProfile):
24+
(JSC::CodeBlock::addArrayProfile): Deleted.
25+
(JSC::CodeBlock::getOrAddArrayProfile): Deleted.
26+
* bytecode/CodeBlock.h:
27+
(JSC::CodeBlock::numberOfArrayProfiles const): Deleted.
28+
(JSC::CodeBlock::arrayProfiles): Deleted.
29+
* bytecode/CodeBlockInlines.h:
30+
(JSC::CodeBlock::forEachArrayProfile):
31+
* jit/JIT.h:
32+
* jit/JITInlines.h:
33+
(JSC::JIT::emitArrayProfilingSiteForBytecodeIndexWithCell): Deleted.
34+
* jit/JITPropertyAccess.cpp:
35+
(JSC::JIT::emit_op_get_by_id):
36+
* jit/JITPropertyAccess32_64.cpp:
37+
(JSC::JIT::emit_op_get_by_id):
38+
* llint/LLIntSlowPaths.cpp:
39+
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
40+
141
2019-01-02 Yusuke Suzuki <yusukesuzuki@slowstart.org>
242

343
[JSC] Optimize Object.prototype.toString

‎Source/JavaScriptCore/bytecode/BytecodeList.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -413,8 +413,8 @@
413413
},
414414
metadata: {
415415
mode: GetByIdMode,
416-
modeMetadata: GetByIdModeMetadata,
417416
hitCountForLLIntCaching: unsigned,
417+
modeMetadata: GetByIdModeMetadata,
418418
profile: ValueProfile,
419419
}
420420

@@ -473,8 +473,8 @@
473473
oldStructure: StructureID,
474474
offset: unsigned,
475475
newStructure: StructureID,
476-
structureChain: WriteBarrierBase[StructureChain],
477476
flags: PutByIdFlags,
477+
structureChain: WriteBarrierBase[StructureChain],
478478
},
479479
metadata_initializers: {
480480
flags: :flags

‎Source/JavaScriptCore/bytecode/CodeBlock.cpp‎

Lines changed: 1 addition & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -2498,16 +2498,11 @@ ArrayProfile* CodeBlock::getArrayProfile(const ConcurrentJSLocker&, unsigned byt
24982498
return &metadata.modeMetadata.arrayLengthMode.arrayProfile;
24992499
break;
25002500
}
2501-
25022501
default:
25032502
break;
25042503
}
25052504

2506-
for (auto& m_arrayProfile : m_arrayProfiles) {
2507-
if (m_arrayProfile.bytecodeOffset() == bytecodeOffset)
2508-
return &m_arrayProfile;
2509-
}
2510-
return 0;
2505+
return nullptr;
25112506
}
25122507

25132508
ArrayProfile* CodeBlock::getArrayProfile(unsigned bytecodeOffset)
@@ -2516,33 +2511,6 @@ ArrayProfile* CodeBlock::getArrayProfile(unsigned bytecodeOffset)
25162511
return getArrayProfile(locker, bytecodeOffset);
25172512
}
25182513

2519-
ArrayProfile* CodeBlock::addArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset)
2520-
{
2521-
m_arrayProfiles.append(ArrayProfile(bytecodeOffset));
2522-
return &m_arrayProfiles.last();
2523-
}
2524-
2525-
ArrayProfile* CodeBlock::addArrayProfile(unsigned bytecodeOffset)
2526-
{
2527-
ConcurrentJSLocker locker(m_lock);
2528-
return addArrayProfile(locker, bytecodeOffset);
2529-
}
2530-
2531-
ArrayProfile* CodeBlock::getOrAddArrayProfile(const ConcurrentJSLocker& locker, unsigned bytecodeOffset)
2532-
{
2533-
ArrayProfile* result = getArrayProfile(locker, bytecodeOffset);
2534-
if (result)
2535-
return result;
2536-
return addArrayProfile(locker, bytecodeOffset);
2537-
}
2538-
2539-
ArrayProfile* CodeBlock::getOrAddArrayProfile(unsigned bytecodeOffset)
2540-
{
2541-
ConcurrentJSLocker locker(m_lock);
2542-
return getOrAddArrayProfile(locker, bytecodeOffset);
2543-
}
2544-
2545-
25462514
#if ENABLE(DFG_JIT)
25472515
Vector<CodeOrigin, 0, UnsafeVectorOverflow>& CodeBlock::codeOrigins()
25482516
{

‎Source/JavaScriptCore/bytecode/CodeBlock.h‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -464,14 +464,8 @@ class CodeBlock : public JSCell {
464464

465465
bool couldTakeSpecialFastCase(InstructionStream::Offset bytecodeOffset);
466466

467-
unsigned numberOfArrayProfiles() const { return m_arrayProfiles.size(); }
468-
const ArrayProfileVector& arrayProfiles() { return m_arrayProfiles; }
469-
ArrayProfile* addArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset);
470-
ArrayProfile* addArrayProfile(unsigned bytecodeOffset);
471467
ArrayProfile* getArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset);
472468
ArrayProfile* getArrayProfile(unsigned bytecodeOffset);
473-
ArrayProfile* getOrAddArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset);
474-
ArrayProfile* getOrAddArrayProfile(unsigned bytecodeOffset);
475469

476470
// Exception handling support
477471

@@ -987,7 +981,6 @@ class CodeBlock : public JSCell {
987981
RefCountedArray<ValueProfile> m_argumentValueProfiles;
988982
Vector<std::unique_ptr<ValueProfileAndOperandBuffer>> m_catchProfiles;
989983
SegmentedVector<RareCaseProfile, 8> m_rareCaseProfiles;
990-
ArrayProfileVector m_arrayProfiles;
991984

992985
// Constant Pool
993986
COMPILE_ASSERT(sizeof(Register) == sizeof(WriteBarrier<Unknown>), Register_must_be_same_size_as_WriteBarrier_Unknown);

‎Source/JavaScriptCore/bytecode/CodeBlockInlines.h‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,6 @@ void CodeBlock::forEachValueProfile(const Functor& func)
5151
template<typename Functor>
5252
void CodeBlock::forEachArrayProfile(const Functor& func)
5353
{
54-
for (auto& arrayProfile : m_arrayProfiles)
55-
func(arrayProfile);
56-
5754
if (m_metadata) {
5855
m_metadata->forEach<OpGetById>([&] (auto& metadata) {
5956
if (metadata.mode == GetByIdMode::ArrayLength)

‎Source/JavaScriptCore/jit/JIT.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,6 @@ namespace JSC {
365365
emitValueProfilingSiteIfProfiledOpcode(Op bytecode);
366366

367367
void emitArrayProfilingSiteWithCell(RegisterID cell, RegisterID indexingType, ArrayProfile*);
368-
void emitArrayProfilingSiteForBytecodeIndexWithCell(RegisterID cell, RegisterID indexingType, unsigned bytecodeIndex);
369368
void emitArrayProfileStoreToHoleSpecialCase(ArrayProfile*);
370369
void emitArrayProfileOutOfBoundsSpecialCase(ArrayProfile*);
371370

‎Source/JavaScriptCore/jit/JITInlines.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -354,11 +354,6 @@ inline void JIT::emitArrayProfilingSiteWithCell(RegisterID cell, RegisterID inde
354354
load8(Address(cell, JSCell::indexingTypeAndMiscOffset()), indexingType);
355355
}
356356

357-
inline void JIT::emitArrayProfilingSiteForBytecodeIndexWithCell(RegisterID cell, RegisterID indexingType, unsigned bytecodeIndex)
358-
{
359-
emitArrayProfilingSiteWithCell(cell, indexingType, m_codeBlock->getOrAddArrayProfile(bytecodeIndex));
360-
}
361-
362357
inline void JIT::emitArrayProfileStoreToHoleSpecialCase(ArrayProfile* arrayProfile)
363358
{
364359
store8(TrustedImm32(1), arrayProfile->addressOfMayStoreToHole());

‎Source/JavaScriptCore/jit/JITPropertyAccess.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,7 @@ void JIT::emitSlow_op_get_by_id_direct(const Instruction* currentInstruction, Ve
572572
void JIT::emit_op_get_by_id(const Instruction* currentInstruction)
573573
{
574574
auto bytecode = currentInstruction->as<OpGetById>();
575+
auto& metadata = bytecode.metadata(m_codeBlock);
575576
int resultVReg = bytecode.dst.offset();
576577
int baseVReg = bytecode.base.offset();
577578
const Identifier* ident = &(m_codeBlock->identifier(bytecode.property));
@@ -580,8 +581,11 @@ void JIT::emit_op_get_by_id(const Instruction* currentInstruction)
580581

581582
emitJumpSlowCaseIfNotJSCell(regT0, baseVReg);
582583

583-
if (*ident == m_vm->propertyNames->length && shouldEmitProfiling())
584-
emitArrayProfilingSiteForBytecodeIndexWithCell(regT0, regT1, m_bytecodeOffset);
584+
if (*ident == m_vm->propertyNames->length && shouldEmitProfiling()) {
585+
Jump notArrayLengthMode = branch8(NotEqual, AbsoluteAddress(&metadata.mode), TrustedImm32(static_cast<uint8_t>(GetByIdMode::ArrayLength)));
586+
emitArrayProfilingSiteWithCell(regT0, regT1, &metadata.modeMetadata.arrayLengthMode.arrayProfile);
587+
notArrayLengthMode.link(this);
588+
}
585589

586590
JITGetByIdGenerator gen(
587591
m_codeBlock, CodeOrigin(m_bytecodeOffset), CallSiteIndex(m_bytecodeOffset), RegisterSet::stubUnavailableRegisters(),

0 commit comments

Comments
 (0)