Skip to content

Commit 3576f09

Browse files
ConstellationConstellation
committed
[JSC] Always create StructureStubInfo for op_get_by_val
https://bugs.webkit.org/show_bug.cgi?id=241669 rdar://75146284 Reviewed by Saam Barati and Mark Lam. DFG OSR exit requires StructureStubInfo for getter / setter calls. However very generic baseline JIT op_get_by_val does not create StructureStubInfo. It is possible that OSR exit crashes because of this missing StructureStubInfo. Let's consider the following edge case. 1. Now, Baseline detects that this is very generic op_get_by_val. So we do not create StructureStubInfo. 2. This function is inlined in DFG. And DFG emits IC for this GetByVal. 3. (2)'s DFG function collects information in DFG-level IC. And luckily, in this inlined call path, it was not so generic. 4. Then, due to different OSR exit or something, we recreate DFG code for this function with (2)'s inlining. 5. DFG detects that DFG-level IC has more specialized information. So it can inline getter call in this op_get_by_val. 6. Inside this getter, we perform OSR exit. 7. Looking into Baseline, and we found that there is no StructureStubInfo! We always create StructureStubInfo. In very generic op_get_by_val case, we create this with tookSlowPath = true. And we emit empty inline path to record doneLocation. So, OSR exit can jump to this place. We also clean up StructureStubInfo code. 1. "start" is renamed to startLocation. And we do not record it in DataIC case since it is not necessary. 2. Rename inlineSize to inlineCodeSize. 3. Add some assertions to ensure that this path is not used for DataIC case. 4. We also record opcode value in the crashing RELEASE_ASSERT to get more information if this does not fix the issue. * Source/JavaScriptCore/bytecode/InlineAccess.cpp: (JSC::linkCodeInline): (JSC::InlineAccess::generateArrayLength): (JSC::InlineAccess::generateStringLength): (JSC::InlineAccess::rewireStubAsJumpInAccessNotUsingInlineAccess): (JSC::InlineAccess::rewireStubAsJumpInAccess): (JSC::InlineAccess::resetStubAsJumpInAccess): * Source/JavaScriptCore/bytecode/StructureStubInfo.cpp: (JSC::StructureStubInfo::initializeFromUnlinkedStructureStubInfo): (JSC::StructureStubInfo::initializeFromDFGUnlinkedStructureStubInfo): * Source/JavaScriptCore/bytecode/StructureStubInfo.h: (JSC::StructureStubInfo::inlineCodeSize const): (JSC::StructureStubInfo::inlineSize const): Deleted. * Source/JavaScriptCore/dfg/DFGInlineCacheWrapperInlines.h: (JSC::DFG::InlineCacheWrapper<GeneratorType>::finalize): * Source/JavaScriptCore/dfg/DFGJITCode.h: * Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp: (JSC::DFG::callerReturnPC): * Source/JavaScriptCore/jit/JIT.cpp: (JSC::JIT::link): * Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp: (JSC::JITInlineCacheGenerator::finalize): (JSC::JITGetByValGenerator::generateEmptyPath): * Source/JavaScriptCore/jit/JITInlineCacheGenerator.h: * Source/JavaScriptCore/jit/JITPropertyAccess.cpp: (JSC::JIT::emit_op_get_by_val): * JSTests/stress/get-by-val-generic-structurestubinfo.js: Added. (let.program): (runMono.let.o.get x): (runMono): (runPoly): Canonical link: https://commits.webkit.org/251619@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295614 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent ea5f106 commit 3576f09

11 files changed

+115
-43
lines changed

‎JSTests/stress/get-by-val-generic-structurestubinfo.js

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
//@ requireOptions("--getByValICMaxNumberOfIdentifiers=2")
2+
3+
let program = `
4+
function shouldBe(actual, expected) {
5+
if (actual !== expected)
6+
throw new Error('bad value: ' + actual);
7+
}
8+
noInline(shouldBe);
9+
10+
function foo(o, p) {
11+
return o[p];
12+
}
13+
noInline(foo);
14+
15+
function runMono() {
16+
let o = {
17+
get x() {
18+
if ($vm.ftlTrue()) OSRExit();
19+
return 42;
20+
}
21+
};
22+
for (let i = 0; i < 1000000; ++i) {
23+
shouldBe(foo(o, "x"), 42);
24+
}
25+
}
26+
27+
function runPoly() {
28+
let o = {
29+
a: 1,
30+
b: 2,
31+
c: 4,
32+
d: 4,
33+
e: 4,
34+
f: 4,
35+
g: 4,
36+
};
37+
for (let i = 0; i < 1000000; ++i) {
38+
foo(o, "a");
39+
foo(o, "b");
40+
foo(o, "c");
41+
foo(o, "d");
42+
foo(o, "e");
43+
foo(o, "f");
44+
foo(o, "g");
45+
foo(o, "h");
46+
foo(o, "i");
47+
}
48+
}
49+
`;
50+
51+
let g1 = runString(program);
52+
g1.runPoly();
53+
54+
let g2 = runString(program);
55+
g2.runMono();

‎Source/JavaScriptCore/bytecode/InlineAccess.cpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -152,9 +152,9 @@ void InlineAccess::dumpCacheSizesAndCrash()
152152
template <typename Function>
153153
ALWAYS_INLINE static bool linkCodeInline(const char* name, CCallHelpers& jit, StructureStubInfo& stubInfo, const Function& function)
154154
{
155-
if (jit.m_assembler.buffer().codeSize() <= stubInfo.inlineSize()) {
155+
if (jit.m_assembler.buffer().codeSize() <= stubInfo.inlineCodeSize()) {
156156
bool needsBranchCompaction = true;
157-
LinkBuffer linkBuffer(jit, stubInfo.start, stubInfo.inlineSize(), LinkBuffer::Profile::InlineCache, JITCompilationMustSucceed, needsBranchCompaction);
157+
LinkBuffer linkBuffer(jit, stubInfo.startLocation, stubInfo.inlineCodeSize(), LinkBuffer::Profile::InlineCache, JITCompilationMustSucceed, needsBranchCompaction);
158158
ASSERT(linkBuffer.isValid());
159159
function(linkBuffer);
160160
FINALIZE_CODE(linkBuffer, NoPtrTag, "InlineAccessType: '%s'", name);
@@ -169,7 +169,7 @@ ALWAYS_INLINE static bool linkCodeInline(const char* name, CCallHelpers& jit, St
169169
constexpr bool failIfCantInline = false;
170170
if (failIfCantInline) {
171171
dataLog("Failure for: ", name, "\n");
172-
dataLog("real size: ", jit.m_assembler.buffer().codeSize(), " inline size:", stubInfo.inlineSize(), "\n");
172+
dataLog("real size: ", jit.m_assembler.buffer().codeSize(), " inline size:", stubInfo.inlineCodeSize(), "\n");
173173
CRASH();
174174
}
175175

@@ -310,6 +310,7 @@ bool InlineAccess::isCacheableArrayLength(CodeBlock* codeBlock, StructureStubInf
310310

311311
bool InlineAccess::generateArrayLength(CodeBlock* codeBlock, StructureStubInfo& stubInfo, JSArray* array)
312312
{
313+
ASSERT_UNUSED(codeBlock, !codeBlock->useDataIC());
313314
ASSERT_UNUSED(codeBlock, isCacheableArrayLength(codeBlock, stubInfo, array));
314315

315316
if (!stubInfo.hasConstantIdentifier)
@@ -348,6 +349,7 @@ bool InlineAccess::isCacheableStringLength(CodeBlock* codeBlock, StructureStubIn
348349

349350
bool InlineAccess::generateStringLength(CodeBlock* codeBlock, StructureStubInfo& stubInfo)
350351
{
352+
ASSERT_UNUSED(codeBlock, !codeBlock->useDataIC());
351353
ASSERT_UNUSED(codeBlock, isCacheableStringLength(codeBlock, stubInfo));
352354

353355
if (!stubInfo.hasConstantIdentifier)
@@ -416,7 +418,7 @@ void InlineAccess::rewireStubAsJumpInAccessNotUsingInlineAccess(CodeBlock* codeB
416418
return;
417419
}
418420

419-
CCallHelpers::emitJITCodeOver(stubInfo.start.retagged<JSInternalPtrTag>(), scopedLambda<void(CCallHelpers&)>([&](CCallHelpers& jit) {
421+
CCallHelpers::emitJITCodeOver(stubInfo.startLocation.retagged<JSInternalPtrTag>(), scopedLambda<void(CCallHelpers&)>([&](CCallHelpers& jit) {
420422
// We don't need a nop sled here because nobody should be jumping into the middle of an IC.
421423
auto jump = jit.jump();
422424
jit.addLinkTask([=] (LinkBuffer& linkBuffer) {
@@ -433,7 +435,7 @@ void InlineAccess::rewireStubAsJumpInAccess(CodeBlock* codeBlock, StructureStubI
433435
return;
434436
}
435437

436-
CCallHelpers::emitJITCodeOver(stubInfo.start.retagged<JSInternalPtrTag>(), scopedLambda<void(CCallHelpers&)>([&](CCallHelpers& jit) {
438+
CCallHelpers::emitJITCodeOver(stubInfo.startLocation.retagged<JSInternalPtrTag>(), scopedLambda<void(CCallHelpers&)>([&](CCallHelpers& jit) {
437439
// We don't need a nop sled here because nobody should be jumping into the middle of an IC.
438440
auto jump = jit.jump();
439441
jit.addLinkTask([=] (LinkBuffer& linkBuffer) {
@@ -450,7 +452,7 @@ void InlineAccess::resetStubAsJumpInAccess(CodeBlock* codeBlock, StructureStubIn
450452
return;
451453
}
452454

453-
CCallHelpers::emitJITCodeOver(stubInfo.start.retagged<JSInternalPtrTag>(), scopedLambda<void(CCallHelpers&)>([&](CCallHelpers& jit) {
455+
CCallHelpers::emitJITCodeOver(stubInfo.startLocation.retagged<JSInternalPtrTag>(), scopedLambda<void(CCallHelpers&)>([&](CCallHelpers& jit) {
454456
// We don't need a nop sled here because nobody should be jumping into the middle of an IC.
455457
auto jump = jit.jump();
456458
auto slowPathStartLocation = stubInfo.slowPathStartLocation;

‎Source/JavaScriptCore/bytecode/StructureStubInfo.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -487,13 +487,13 @@ static FunctionPtr<OperationPtrTag> slowOperationFromUnlinkedStructureStubInfo(c
487487
void StructureStubInfo::initializeFromUnlinkedStructureStubInfo(const BaselineUnlinkedStructureStubInfo& unlinkedStubInfo)
488488
{
489489
accessType = unlinkedStubInfo.accessType;
490-
start = unlinkedStubInfo.start;
491490
doneLocation = unlinkedStubInfo.doneLocation;
492491
slowPathStartLocation = unlinkedStubInfo.slowPathStartLocation;
493492
callSiteIndex = CallSiteIndex(BytecodeIndex(unlinkedStubInfo.bytecodeIndex.offset()));
494493
codeOrigin = CodeOrigin(unlinkedStubInfo.bytecodeIndex);
495494
m_codePtr = slowPathStartLocation;
496495
propertyIsInt32 = unlinkedStubInfo.propertyIsInt32;
496+
tookSlowPath = unlinkedStubInfo.tookSlowPath;
497497
useDataIC = true;
498498

499499
usedRegisters = RegisterSet::stubUnavailableRegisters();
@@ -654,7 +654,6 @@ void StructureStubInfo::initializeFromUnlinkedStructureStubInfo(const BaselineUn
654654
void StructureStubInfo::initializeFromDFGUnlinkedStructureStubInfo(const DFG::UnlinkedStructureStubInfo& unlinkedStubInfo)
655655
{
656656
accessType = unlinkedStubInfo.accessType;
657-
start = unlinkedStubInfo.start;
658657
doneLocation = unlinkedStubInfo.doneLocation;
659658
slowPathStartLocation = unlinkedStubInfo.slowPathStartLocation;
660659
callSiteIndex = unlinkedStubInfo.callSiteIndex;
@@ -666,6 +665,7 @@ void StructureStubInfo::initializeFromDFGUnlinkedStructureStubInfo(const DFG::Un
666665
propertyIsString = unlinkedStubInfo.propertyIsString;
667666
prototypeIsKnownObject = unlinkedStubInfo.prototypeIsKnownObject;
668667
hasConstantIdentifier = unlinkedStubInfo.hasConstantIdentifier;
668+
tookSlowPath = unlinkedStubInfo.tookSlowPath;
669669
useDataIC = true;
670670

671671
usedRegisters = unlinkedStubInfo.usedRegisters;

‎Source/JavaScriptCore/bytecode/StructureStubInfo.h

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,11 @@ class StructureStubInfo {
153153

154154
bool containsPC(void* pc) const;
155155

156-
uint32_t inlineSize() const
156+
uint32_t inlineCodeSize() const
157157
{
158-
int32_t inlineSize = MacroAssembler::differenceBetweenCodePtr(start, doneLocation);
158+
if (useDataIC)
159+
return 0;
160+
int32_t inlineSize = MacroAssembler::differenceBetweenCodePtr(startLocation, doneLocation);
159161
ASSERT(inlineSize >= 0);
160162
return inlineSize;
161163
}
@@ -382,7 +384,9 @@ class StructureStubInfo {
382384
// That's not so bad - we'll get rid of the redundant ones once we regenerate.
383385
HashSet<BufferedStructure, BufferedStructure::Hash, BufferedStructure::KeyTraits> m_bufferedStructures WTF_GUARDED_BY_LOCK(m_bufferedStructuresLock);
384386
public:
385-
CodeLocationLabel<JITStubRoutinePtrTag> start; // This is either the start of the inline IC for *byId caches. or the location of patchable jump for 'instanceof' caches.
387+
// This is either the start of the inline IC for *byId caches. or the location of patchable jump for 'instanceof' caches.
388+
// If useDataIC is true, then it is nullptr.
389+
CodeLocationLabel<JITStubRoutinePtrTag> startLocation;
386390
CodeLocationLabel<JSInternalPtrTag> doneLocation;
387391
CodeLocationLabel<JITStubRoutinePtrTag> slowPathStartLocation;
388392

@@ -480,11 +484,11 @@ struct UnlinkedStructureStubInfo {
480484
PutKind putKind { PutKind::Direct };
481485
PrivateFieldPutKind privateFieldPutKind { PrivateFieldPutKind::none() };
482486
ECMAMode ecmaMode { ECMAMode::sloppy() };
483-
bool propertyIsInt32 { false };
484-
bool propertyIsString { false };
485-
bool propertyIsSymbol { false };
486-
bool prototypeIsKnownObject { false };
487-
CodeLocationLabel<JITStubRoutinePtrTag> start; // This is either the start of the inline IC for *byId caches. or the location of patchable jump for 'instanceof' caches.
487+
bool propertyIsInt32 : 1 { false };
488+
bool propertyIsString : 1 { false };
489+
bool propertyIsSymbol : 1 { false };
490+
bool prototypeIsKnownObject : 1 { false };
491+
bool tookSlowPath : 1 { false };
488492
CodeLocationLabel<JSInternalPtrTag> doneLocation;
489493
CodeLocationLabel<JITStubRoutinePtrTag> slowPathStartLocation;
490494
};

‎Source/JavaScriptCore/dfg/DFGInlineCacheWrapperInlines.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ void InlineCacheWrapper<GeneratorType>::finalize(LinkBuffer& fastPath, LinkBuffe
3737
{
3838
m_generator.reportSlowPathCall(m_slowPath->label(), m_slowPath->call());
3939
if (m_generator.m_unlinkedStubInfo) {
40-
m_generator.m_unlinkedStubInfo->start = fastPath.locationOf<JITStubRoutinePtrTag>(m_generator.m_start);
4140
m_generator.m_unlinkedStubInfo->doneLocation = fastPath.locationOf<JSInternalPtrTag>(m_generator.m_done);
4241
m_generator.m_unlinkedStubInfo->slowPathStartLocation = fastPath.locationOf<JITStubRoutinePtrTag>(m_generator.m_slowPathBegin);
4342
} else

‎Source/JavaScriptCore/dfg/DFGJITCode.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ class JITCompiler;
5353

5454
struct UnlinkedStructureStubInfo : JSC::UnlinkedStructureStubInfo {
5555
CodeOrigin codeOrigin;
56-
CallSiteIndex callSiteIndex;
5756
RegisterSet usedRegisters;
57+
CallSiteIndex callSiteIndex;
5858
GPRReg m_baseGPR { InvalidGPRReg };
5959
GPRReg m_valueGPR { InvalidGPRReg };
6060
GPRReg m_extraGPR { InvalidGPRReg };

‎Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ static MacroAssemblerCodePtr<JSEntryPtrTag> callerReturnPC(CodeBlock* baselineCo
214214
case InlineCallFrame::GetterCall:
215215
case InlineCallFrame::SetterCall: {
216216
StructureStubInfo* stubInfo = baselineCodeBlockForCaller->findStubInfo(CodeOrigin(callBytecodeIndex));
217-
RELEASE_ASSERT(stubInfo);
217+
RELEASE_ASSERT(stubInfo, callInstruction.opcodeID());
218218
jumpTarget = stubInfo->doneLocation.retagged<JSEntryPtrTag>();
219219
break;
220220
}

‎Source/JavaScriptCore/jit/JIT.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -946,7 +946,6 @@ void JIT::link()
946946

947947
auto finalizeICs = [&] (auto& generators) {
948948
for (auto& gen : generators) {
949-
gen.m_unlinkedStubInfo->start = patchBuffer.locationOf<JITStubRoutinePtrTag>(gen.m_start);
950949
gen.m_unlinkedStubInfo->doneLocation = patchBuffer.locationOf<JSInternalPtrTag>(gen.m_done);
951950
gen.m_unlinkedStubInfo->slowPathStartLocation = patchBuffer.locationOf<JITStubRoutinePtrTag>(gen.m_slowPathBegin);
952951
}

‎Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ void JITInlineCacheGenerator::finalize(
6363
LinkBuffer& fastPath, LinkBuffer& slowPath, CodeLocationLabel<JITStubRoutinePtrTag> start)
6464
{
6565
ASSERT(m_stubInfo);
66-
m_stubInfo->start = start;
66+
m_stubInfo->startLocation = start;
6767
m_stubInfo->doneLocation = fastPath.locationOf<JSInternalPtrTag>(m_done);
6868

6969
if (!m_stubInfo->useDataIC)
@@ -534,6 +534,12 @@ void JITGetByValGenerator::generateFastPath(CCallHelpers& jit)
534534
m_done = jit.label();
535535
}
536536

537+
void JITGetByValGenerator::generateEmptyPath(CCallHelpers& jit)
538+
{
539+
m_start = jit.label();
540+
m_done = jit.label();
541+
}
542+
537543
void JITGetByValGenerator::finalize(LinkBuffer& fastPath, LinkBuffer& slowPath)
538544
{
539545
ASSERT(m_stubInfo);

‎Source/JavaScriptCore/jit/JITInlineCacheGenerator.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,8 @@ class JITGetByValGenerator final : public JITInlineCacheGenerator {
558558

559559
void generateFastPath(CCallHelpers&);
560560

561+
void generateEmptyPath(CCallHelpers&);
562+
561563
template<typename StubInfo>
562564
static void setUpStubInfo(StubInfo& stubInfo,
563565
AccessType accessType, CodeOrigin codeOrigin, CallSiteIndex callSiteIndex, const RegisterSet& usedRegisters,

0 commit comments

Comments
 (0)