Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address deref issue #1530

Merged
merged 1 commit into from
Sep 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions lib/Backend/GlobOptFields.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2170,7 +2170,7 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
isObjTypeSpecialized = ProcessPropOpInTypeCheckSeq<true>(instr, opnd, block, updateExistingValue, emitsTypeCheckOut, changesTypeValueOut, &isObjTypeChecked);
}

if (opnd == instr->GetDst() && this->objectTypeSyms && !isObjTypeChecked)
if (opnd == instr->GetDst() && this->objectTypeSyms)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the change you're making here is that we kill the current type sym even if its type has been checked upstream? But if the type has been checked, and if the store causes us to transition to a type with a different layout, then we can update the type sym's value in the value table, and that should cause downstream dereferences to be correct. Is that not happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I am porting the change you have made. Was there a problem in the porting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty funny. I obviously have vacation brain. I'll look again later.

-- Paul


From: Akrosh Gandhimailto:notifications@github.com
Sent: ‎9/‎2/‎2016 10:04 AM
To: Microsoft/ChakraCoremailto:ChakraCore@noreply.github.com
Cc: Paul Leathersmailto:pleath@microsoft.com; Mentionmailto:mention@noreply.github.com
Subject: Re: [Microsoft/ChakraCore] Address deref issue (#1530)

In lib/Backend/GlobOptFields.cpphttps://github.com//pull/1530#discussion_r77379371:

@@ -2170,7 +2170,7 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
isObjTypeSpecialized = ProcessPropOpInTypeCheckSeq(instr, opnd, block, updateExistingValue, emitsTypeCheckOut, changesTypeValueOut, &isObjTypeChecked);
}

  • if (opnd == instr->GetDst() && this->objectTypeSyms && !isObjTypeChecked)
  • if (opnd == instr->GetDst() && this->objectTypeSyms)

Wait, I am porting the change you have made. Was there a problem in the porting?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1530/files/5ec2d8f6dd3e67e8aa85002dbad152a614f92eeb#r77379371, or mute the threadhttps://github.com/notifications/unsubscribe-auth/APF8RCyVA8Ey0Rwuy34S_lZV5yLHz5mDks5qmFcvgaJpZM4JzSqF.

{
if (block == nullptr)
{
Expand All @@ -2180,26 +2180,29 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
// This is a property store that may change the layout of the object that it stores to. This means that
// it may change any aliased object. Do two things to address this:
// - Add all object types in this function to the set that may have had a property added. This will prevent
// final type optimization across this instruction.
// final type optimization across this instruction. (Only needed here for non-specialized stores.)
// - Kill all type symbols that currently hold object-header-inlined types. Any of them may have their layout
// changed by the addition of a property.

SymID opndId = opnd->HasObjectTypeSym() ? opnd->GetObjectTypeSym()->m_id : -1;
if (block->globOptData.maybeWrittenTypeSyms == nullptr)
{
block->globOptData.maybeWrittenTypeSyms = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
}
if (isObjTypeSpecialized)
if (!isObjTypeChecked)
{
// The current object will be protected by a type check, unless no further accesses to it are
// protected by this access.
Assert(this->objectTypeSyms->Test(opndId));
this->objectTypeSyms->Clear(opndId);
}
block->globOptData.maybeWrittenTypeSyms->Or(this->objectTypeSyms);
if (isObjTypeSpecialized)
{
this->objectTypeSyms->Set(opndId);
if (block->globOptData.maybeWrittenTypeSyms == nullptr)
{
block->globOptData.maybeWrittenTypeSyms = JitAnew(this->alloc, BVSparse<JitArenaAllocator>, this->alloc);
}
if (isObjTypeSpecialized)
{
// The current object will be protected by a type check, unless no further accesses to it are
// protected by this access.
Assert(this->objectTypeSyms->Test(opndId));
this->objectTypeSyms->Clear(opndId);
}
block->globOptData.maybeWrittenTypeSyms->Or(this->objectTypeSyms);
if (isObjTypeSpecialized)
{
this->objectTypeSyms->Set(opndId);
}
}

if (!isObjTypeSpecialized || opnd->ChangesObjectLayout())
Expand Down
37 changes: 30 additions & 7 deletions lib/Backend/Opnd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -846,20 +846,43 @@ PropertySymOpnd::IsObjectHeaderInlined() const
bool
PropertySymOpnd::ChangesObjectLayout() const
{
Js::Type *cachedType = this->IsMono() ? this->GetType() : this->GetFirstEquivalentType();

Js::Type *finalType = this->GetFinalType();
if (finalType == nullptr || !Js::DynamicType::Is(finalType->GetTypeId()))
if (finalType && Js::DynamicType::Is(finalType->GetTypeId()))
{
// This is the case where final type opt may cause pro-active type transition to take place.

Assert(cachedType && Js::DynamicType::Is(cachedType->GetTypeId()));

Js::DynamicTypeHandler * cachedTypeHandler = (static_cast<Js::DynamicType*>(cachedType))->GetTypeHandler();
Js::DynamicTypeHandler * finalTypeHandler = (static_cast<Js::DynamicType*>(finalType))->GetTypeHandler();

return cachedTypeHandler->GetInlineSlotCapacity() != finalTypeHandler->GetInlineSlotCapacity() ||
cachedTypeHandler->GetOffsetOfInlineSlots() != finalTypeHandler->GetOffsetOfInlineSlots();
}

if (!this->HasInitialType())
{
return false;
}

Js::Type *cachedType = this->IsMono() ? this->GetType() : this->GetFirstEquivalentType();
Assert(cachedType && Js::DynamicType::Is(cachedType->GetTypeId()));
Js::Type *initialType = this->GetInitialType();
if (initialType && Js::DynamicType::Is(initialType->GetTypeId()))
{
// This is the case where the type transition actually occurs. (This is the only case that's detectable
// during the loop pre-pass, since final types are not in place yet.)

Js::DynamicTypeHandler * cachedTypeHandler = (static_cast<Js::DynamicType*>(cachedType))->GetTypeHandler();
Js::DynamicTypeHandler * finalTypeHandler = (static_cast<Js::DynamicType*>(finalType))->GetTypeHandler();
Assert(cachedType && Js::DynamicType::Is(cachedType->GetTypeId()));

return cachedTypeHandler->GetInlineSlotCapacity() != finalTypeHandler->GetInlineSlotCapacity() ||
cachedTypeHandler->GetOffsetOfInlineSlots() != finalTypeHandler->GetOffsetOfInlineSlots();
Js::DynamicTypeHandler * cachedTypeHandler = (static_cast<Js::DynamicType*>(cachedType))->GetTypeHandler();
Js::DynamicTypeHandler * initialTypeHandler = (static_cast<Js::DynamicType*>(initialType))->GetTypeHandler();

return cachedTypeHandler->GetInlineSlotCapacity() != initialTypeHandler->GetInlineSlotCapacity() ||
cachedTypeHandler->GetOffsetOfInlineSlots() != initialTypeHandler->GetOffsetOfInlineSlots();
}

return false;
}

void
Expand Down