Skip to content

Commit aaff132

Browse files
saambaratisaambarati
committed
Continue to consult InlineAccess's Structure even after switching to a stub IC
https://bugs.webkit.org/show_bug.cgi?id=227785 Reviewed by Yusuke Suzuki. This patch fixes a crash in: stress/class-subclassing-function.js The bug is this: 1. We initialize a StructureStubInfo to be an inline self access doing a load based on structure S. 2. We transition to being a PolymorphicAccess based StructureStubInfo. But, we haven't generated code yet. We're in the buffered state. So we are still running the inline access from (1). But the StructureStubInfo thinks it's a "Stub". 3. S is collected 4. We continue to run code from (1), because when we finalize the IC during GC, it doesn't think it's an inline access. The fix is to always track the structure S that we used when generating the inline access, and to only stop tracking it once we've generated code for the Stub. * bytecode/AccessCase.cpp: (JSC::AccessCase::fromStructureStubInfo): (JSC::AccessCase::propagateTransitions const): * bytecode/AccessCase.h: * bytecode/GetByStatus.cpp: (JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback): * bytecode/InByStatus.cpp: (JSC::InByStatus::computeForStubInfoWithoutExitSiteFeedback): * bytecode/PolymorphicAccess.cpp: (JSC::PolymorphicAccess::propagateTransitions const): * bytecode/PolymorphicAccess.h: * bytecode/PutByIdStatus.cpp: (JSC::PutByIdStatus::computeForStubInfo): * bytecode/StructureStubInfo.cpp: (JSC::StructureStubInfo::initGetByIdSelf): (JSC::StructureStubInfo::initPutByIdReplace): (JSC::StructureStubInfo::initInByIdSelf): (JSC::StructureStubInfo::addAccessCase): (JSC::StructureStubInfo::reset): (JSC::StructureStubInfo::visitWeakReferences): (JSC::StructureStubInfo::propagateTransitions): * bytecode/StructureStubInfo.h: Canonical link: https://commits.webkit.org/239574@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@279813 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 95c47ac commit aaff132

10 files changed

Lines changed: 98 additions & 71 deletions

File tree

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,47 @@
1+
2021-07-10 Saam Barati <sbarati@apple.com>
2+
3+
Continue to consult InlineAccess's Structure even after switching to a stub IC
4+
https://bugs.webkit.org/show_bug.cgi?id=227785
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
This patch fixes a crash in: stress/class-subclassing-function.js
9+
10+
The bug is this:
11+
1. We initialize a StructureStubInfo to be an inline self access doing a load based on structure S.
12+
2. We transition to being a PolymorphicAccess based StructureStubInfo. But, we haven't
13+
generated code yet. We're in the buffered state. So we are still running the inline access
14+
from (1). But the StructureStubInfo thinks it's a "Stub".
15+
3. S is collected
16+
4. We continue to run code from (1), because when we finalize the IC during GC, it
17+
doesn't think it's an inline access.
18+
19+
The fix is to always track the structure S that we used when generating the inline
20+
access, and to only stop tracking it once we've generated code for the Stub.
21+
22+
* bytecode/AccessCase.cpp:
23+
(JSC::AccessCase::fromStructureStubInfo):
24+
(JSC::AccessCase::propagateTransitions const):
25+
* bytecode/AccessCase.h:
26+
* bytecode/GetByStatus.cpp:
27+
(JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
28+
* bytecode/InByStatus.cpp:
29+
(JSC::InByStatus::computeForStubInfoWithoutExitSiteFeedback):
30+
* bytecode/PolymorphicAccess.cpp:
31+
(JSC::PolymorphicAccess::propagateTransitions const):
32+
* bytecode/PolymorphicAccess.h:
33+
* bytecode/PutByIdStatus.cpp:
34+
(JSC::PutByIdStatus::computeForStubInfo):
35+
* bytecode/StructureStubInfo.cpp:
36+
(JSC::StructureStubInfo::initGetByIdSelf):
37+
(JSC::StructureStubInfo::initPutByIdReplace):
38+
(JSC::StructureStubInfo::initInByIdSelf):
39+
(JSC::StructureStubInfo::addAccessCase):
40+
(JSC::StructureStubInfo::reset):
41+
(JSC::StructureStubInfo::visitWeakReferences):
42+
(JSC::StructureStubInfo::propagateTransitions):
43+
* bytecode/StructureStubInfo.h:
44+
145
2021-07-10 Yusuke Suzuki <ysuzuki@apple.com>
246

347
[JSC] Workaround test262.report bug by making $ properties enumerable

‎Source/JavaScriptCore/bytecode/AccessCase.cpp‎

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -155,15 +155,15 @@ RefPtr<AccessCase> AccessCase::fromStructureStubInfo(
155155
switch (stubInfo.cacheType()) {
156156
case CacheType::GetByIdSelf:
157157
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
158-
return ProxyableAccessCase::create(vm, owner, Load, identifier, stubInfo.u.byIdSelf.offset, stubInfo.u.byIdSelf.baseObjectStructure.get());
158+
return ProxyableAccessCase::create(vm, owner, Load, identifier, stubInfo.u.byIdSelf.offset, stubInfo.inlineAccessBaseStructure.get());
159159

160160
case CacheType::PutByIdReplace:
161161
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
162-
return AccessCase::create(vm, owner, Replace, identifier, stubInfo.u.byIdSelf.offset, stubInfo.u.byIdSelf.baseObjectStructure.get());
162+
return AccessCase::create(vm, owner, Replace, identifier, stubInfo.u.byIdSelf.offset, stubInfo.inlineAccessBaseStructure.get());
163163

164164
case CacheType::InByIdSelf:
165165
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
166-
return AccessCase::create(vm, owner, InHit, identifier, stubInfo.u.byIdSelf.offset, stubInfo.u.byIdSelf.baseObjectStructure.get());
166+
return AccessCase::create(vm, owner, InHit, identifier, stubInfo.u.byIdSelf.offset, stubInfo.inlineAccessBaseStructure.get());
167167

168168
case CacheType::ArrayLength:
169169
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
@@ -758,35 +758,29 @@ bool AccessCase::visitWeak(VM& vm) const
758758
}
759759

760760
template<typename Visitor>
761-
bool AccessCase::propagateTransitions(Visitor& visitor) const
761+
void AccessCase::propagateTransitions(Visitor& visitor) const
762762
{
763-
bool result = true;
764-
765763
if (m_structure)
766-
result &= m_structure->markIfCheap(visitor);
764+
m_structure->markIfCheap(visitor);
767765

768766
if (m_polyProtoAccessChain) {
769767
for (StructureID structureID : m_polyProtoAccessChain->chain())
770-
result &= visitor.vm().getStructure(structureID)->markIfCheap(visitor);
768+
visitor.vm().getStructure(structureID)->markIfCheap(visitor);
771769
}
772770

773771
switch (m_type) {
774772
case Transition:
775773
case Delete:
776774
if (visitor.isMarked(m_structure->previousID()))
777775
visitor.appendUnbarriered(m_structure.get());
778-
else
779-
result = false;
780776
break;
781777
default:
782778
break;
783779
}
784-
785-
return result;
786780
}
787781

788-
template bool AccessCase::propagateTransitions(AbstractSlotVisitor&) const;
789-
template bool AccessCase::propagateTransitions(SlotVisitor&) const;
782+
template void AccessCase::propagateTransitions(AbstractSlotVisitor&) const;
783+
template void AccessCase::propagateTransitions(SlotVisitor&) const;
790784

791785

792786
template<typename Visitor>

‎Source/JavaScriptCore/bytecode/AccessCase.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ class AccessCase : public ThreadSafeRefCounted<AccessCase> {
300300

301301
DECLARE_VISIT_AGGREGATE_WITH_MODIFIER(const);
302302
bool visitWeak(VM&) const;
303-
template<typename Visitor> bool propagateTransitions(Visitor&) const;
303+
template<typename Visitor> void propagateTransitions(Visitor&) const;
304304

305305
// FIXME: This only exists because of how AccessCase puts post-generation things into itself.
306306
// https://bugs.webkit.org/show_bug.cgi?id=156456

‎Source/JavaScriptCore/bytecode/GetByStatus.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(
213213
return GetByStatus(NoInformation);
214214

215215
case CacheType::GetByIdSelf: {
216-
Structure* structure = stubInfo->u.byIdSelf.baseObjectStructure.get();
216+
Structure* structure = stubInfo->inlineAccessBaseStructure.get();
217217
if (structure->takesSlowPathInDFGForImpureProperty())
218218
return GetByStatus(JSC::slowVersion(summary), *stubInfo);
219219
CacheableIdentifier identifier = stubInfo->identifier();

‎Source/JavaScriptCore/bytecode/InByStatus.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ InByStatus InByStatus::computeForStubInfoWithoutExitSiteFeedback(const Concurren
139139
return InByStatus(NoInformation);
140140

141141
case CacheType::InByIdSelf: {
142-
Structure* structure = stubInfo->u.byIdSelf.baseObjectStructure.get();
142+
Structure* structure = stubInfo->inlineAccessBaseStructure.get();
143143
if (structure->takesSlowPathInDFGForImpureProperty())
144144
return InByStatus(TakesSlowPath);
145145
CacheableIdentifier identifier = stubInfo->identifier();

‎Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -371,16 +371,14 @@ bool PolymorphicAccess::visitWeak(VM& vm) const
371371
}
372372

373373
template<typename Visitor>
374-
bool PolymorphicAccess::propagateTransitions(Visitor& visitor) const
374+
void PolymorphicAccess::propagateTransitions(Visitor& visitor) const
375375
{
376-
bool result = true;
377376
for (unsigned i = 0; i < size(); ++i)
378-
result &= at(i).propagateTransitions(visitor);
379-
return result;
377+
at(i).propagateTransitions(visitor);
380378
}
381379

382-
template bool PolymorphicAccess::propagateTransitions(AbstractSlotVisitor&) const;
383-
template bool PolymorphicAccess::propagateTransitions(SlotVisitor&) const;
380+
template void PolymorphicAccess::propagateTransitions(AbstractSlotVisitor&) const;
381+
template void PolymorphicAccess::propagateTransitions(SlotVisitor&) const;
384382

385383
template<typename Visitor>
386384
void PolymorphicAccess::visitAggregateImpl(Visitor& visitor)

‎Source/JavaScriptCore/bytecode/PolymorphicAccess.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ class PolymorphicAccess {
160160

161161
// This returns true if it has marked everything it will ever marked. This can be used as an
162162
// optimization to then avoid calling this method again during the fixpoint.
163-
template<typename Visitor> bool propagateTransitions(Visitor&) const;
163+
template<typename Visitor> void propagateTransitions(Visitor&) const;
164164

165165
void aboutToDie();
166166

‎Source/JavaScriptCore/bytecode/PutByIdStatus.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,10 @@ PutByIdStatus PutByIdStatus::computeForStubInfo(
147147

148148
case CacheType::PutByIdReplace: {
149149
PropertyOffset offset =
150-
stubInfo->u.byIdSelf.baseObjectStructure->getConcurrently(uid);
150+
stubInfo->inlineAccessBaseStructure->getConcurrently(uid);
151151
if (isValidOffset(offset)) {
152152
return PutByIdVariant::replace(
153-
stubInfo->u.byIdSelf.baseObjectStructure.get(), offset);
153+
stubInfo->inlineAccessBaseStructure.get(), offset);
154154
}
155155
return PutByIdStatus(JSC::slowVersion(summary));
156156
}

‎Source/JavaScriptCore/bytecode/StructureStubInfo.cpp‎

Lines changed: 31 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,14 @@ StructureStubInfo::~StructureStubInfo()
5959
{
6060
}
6161

62-
void StructureStubInfo::initGetByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
62+
void StructureStubInfo::initGetByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* inlineAccessBaseStructure, PropertyOffset offset, CacheableIdentifier identifier)
6363
{
6464
ASSERT(hasConstantIdentifier);
6565
setCacheType(locker, CacheType::GetByIdSelf);
6666
m_identifier = identifier;
6767
codeBlock->vm().heap.writeBarrier(codeBlock);
6868

69-
u.byIdSelf.baseObjectStructure.set(
70-
codeBlock->vm(), codeBlock, baseObjectStructure);
69+
this->inlineAccessBaseStructure.set(codeBlock->vm(), codeBlock, inlineAccessBaseStructure);
7170
u.byIdSelf.offset = offset;
7271
}
7372

@@ -81,25 +80,23 @@ void StructureStubInfo::initStringLength(const ConcurrentJSLockerBase& locker)
8180
setCacheType(locker, CacheType::StringLength);
8281
}
8382

84-
void StructureStubInfo::initPutByIdReplace(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
83+
void StructureStubInfo::initPutByIdReplace(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* inlineAccessBaseStructure, PropertyOffset offset, CacheableIdentifier identifier)
8584
{
8685
setCacheType(locker, CacheType::PutByIdReplace);
8786
m_identifier = identifier;
8887
codeBlock->vm().heap.writeBarrier(codeBlock);
8988

90-
u.byIdSelf.baseObjectStructure.set(
91-
codeBlock->vm(), codeBlock, baseObjectStructure);
89+
this->inlineAccessBaseStructure.set(codeBlock->vm(), codeBlock, inlineAccessBaseStructure);
9290
u.byIdSelf.offset = offset;
9391
}
9492

95-
void StructureStubInfo::initInByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset, CacheableIdentifier identifier)
93+
void StructureStubInfo::initInByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* inlineAccessBaseStructure, PropertyOffset offset, CacheableIdentifier identifier)
9694
{
9795
setCacheType(locker, CacheType::InByIdSelf);
9896
m_identifier = identifier;
9997
codeBlock->vm().heap.writeBarrier(codeBlock);
10098

101-
u.byIdSelf.baseObjectStructure.set(
102-
codeBlock->vm(), codeBlock, baseObjectStructure);
99+
this->inlineAccessBaseStructure.set(codeBlock->vm(), codeBlock, inlineAccessBaseStructure);
103100
u.byIdSelf.offset = offset;
104101
}
105102

@@ -228,6 +225,15 @@ AccessGenerationResult StructureStubInfo::addAccessCase(
228225

229226
if (!result.generatedSomeCode())
230227
return result;
228+
229+
// When we first transition to becoming a Stub, we might still be running the inline
230+
// access code. That's because when we first transition to becoming a Stub, we may
231+
// be buffered, and we have not yet generated any code. Once the Stub finally generates
232+
// code, we're no longer running the inline access code, so we can then clear out
233+
// inlineAccessBaseStructure. The reason we don't clear inlineAccessBaseStructure while
234+
// we're buffered is because we rely on it to reset during GC if inlineAccessBaseStructure
235+
// is collected.
236+
inlineAccessBaseStructure.clear();
231237

232238
// If we generated some code then we don't want to attempt to repatch in the future until we
233239
// gather enough cases.
@@ -241,6 +247,7 @@ AccessGenerationResult StructureStubInfo::addAccessCase(
241247
void StructureStubInfo::reset(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock)
242248
{
243249
clearBufferedStructures();
250+
inlineAccessBaseStructure.clear();
244251

245252
if (m_cacheType == CacheType::Unset)
246253
return;
@@ -346,47 +353,31 @@ void StructureStubInfo::visitWeakReferences(const ConcurrentJSLockerBase& locker
346353
});
347354
}
348355

349-
switch (m_cacheType) {
350-
case CacheType::GetByIdSelf:
351-
case CacheType::PutByIdReplace:
352-
case CacheType::InByIdSelf:
353-
if (vm.heap.isMarked(u.byIdSelf.baseObjectStructure.get()))
354-
return;
355-
break;
356-
case CacheType::Stub:
357-
if (u.stub->visitWeak(vm))
358-
return;
359-
break;
360-
default:
356+
bool isValid = true;
357+
if (inlineAccessBaseStructure)
358+
isValid &= vm.heap.isMarked(inlineAccessBaseStructure.get());
359+
if (m_cacheType == CacheType::Stub)
360+
isValid &= u.stub->visitWeak(vm);
361+
362+
if (isValid)
361363
return;
362-
}
363364

364365
reset(locker, codeBlock);
365366
resetByGC = true;
366367
}
367368

368369
template<typename Visitor>
369-
bool StructureStubInfo::propagateTransitions(Visitor& visitor)
370+
void StructureStubInfo::propagateTransitions(Visitor& visitor)
370371
{
371-
switch (m_cacheType) {
372-
case CacheType::Unset:
373-
case CacheType::ArrayLength:
374-
case CacheType::StringLength:
375-
return true;
376-
case CacheType::GetByIdSelf:
377-
case CacheType::PutByIdReplace:
378-
case CacheType::InByIdSelf:
379-
return u.byIdSelf.baseObjectStructure->markIfCheap(visitor);
380-
case CacheType::Stub:
381-
return u.stub->propagateTransitions(visitor);
382-
}
383-
384-
RELEASE_ASSERT_NOT_REACHED();
385-
return true;
372+
if (inlineAccessBaseStructure)
373+
inlineAccessBaseStructure->markIfCheap(visitor);
374+
375+
if (m_cacheType == CacheType::Stub)
376+
u.stub->propagateTransitions(visitor);
386377
}
387378

388-
template bool StructureStubInfo::propagateTransitions(AbstractSlotVisitor&);
389-
template bool StructureStubInfo::propagateTransitions(SlotVisitor&);
379+
template void StructureStubInfo::propagateTransitions(AbstractSlotVisitor&);
380+
template void StructureStubInfo::propagateTransitions(SlotVisitor&);
390381

391382
StubInfoSummary StructureStubInfo::summary(VM& vm) const
392383
{

‎Source/JavaScriptCore/bytecode/StructureStubInfo.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -84,11 +84,11 @@ class StructureStubInfo {
8484
StructureStubInfo(AccessType, CodeOrigin);
8585
~StructureStubInfo();
8686

87-
void initGetByIdSelf(const ConcurrentJSLockerBase&, CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
87+
void initGetByIdSelf(const ConcurrentJSLockerBase&, CodeBlock*, Structure* inlineAccessBaseStructure, PropertyOffset, CacheableIdentifier);
8888
void initArrayLength(const ConcurrentJSLockerBase&);
8989
void initStringLength(const ConcurrentJSLockerBase&);
90-
void initPutByIdReplace(const ConcurrentJSLockerBase&, CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
91-
void initInByIdSelf(const ConcurrentJSLockerBase&, CodeBlock*, Structure* baseObjectStructure, PropertyOffset, CacheableIdentifier);
90+
void initPutByIdReplace(const ConcurrentJSLockerBase&, CodeBlock*, Structure* inlineAccessBaseStructure, PropertyOffset, CacheableIdentifier);
91+
void initInByIdSelf(const ConcurrentJSLockerBase&, CodeBlock*, Structure* inlineAccessBaseStructure, PropertyOffset, CacheableIdentifier);
9292

9393
AccessGenerationResult addAccessCase(const GCSafeConcurrentJSLocker&, JSGlobalObject*, CodeBlock*, ECMAMode, CacheableIdentifier, RefPtr<AccessCase>);
9494

@@ -104,7 +104,7 @@ class StructureStubInfo {
104104
void visitWeakReferences(const ConcurrentJSLockerBase&, CodeBlock*);
105105

106106
// This returns true if it has marked everything that it will ever mark.
107-
template<typename Visitor> bool propagateTransitions(Visitor&);
107+
template<typename Visitor> void propagateTransitions(Visitor&);
108108

109109
StubInfoSummary summary(VM&) const;
110110

@@ -326,11 +326,11 @@ class StructureStubInfo {
326326
CodeOrigin codeOrigin;
327327
union {
328328
struct {
329-
WriteBarrierBase<Structure> baseObjectStructure;
330329
PropertyOffset offset;
331330
} byIdSelf;
332331
PolymorphicAccess* stub;
333332
} u;
333+
WriteBarrier<Structure> inlineAccessBaseStructure;
334334
private:
335335
CacheableIdentifier m_identifier;
336336
// Represents those structures that already have buffered AccessCases in the PolymorphicAccess.

0 commit comments

Comments
 (0)