Bug 144465 – Object allocation not sinking properly through CheckStructure

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Basile Clement]

Created attachment 252079 [details]
Example of the bug

When sinking allocations through CheckStructure node, we are just ignoring them, which may cause code that should have been guarded by the CheckStructure to execute unchecked and produce wrong results.

[Basile Clement]

According to Filip, the CheckStructureImmediate node was here for that case, I am trying to put up a fix once I understand how it is supposed to work.

[Basile Clement]

Created attachment 252080 [details]
Good old NaN not being equal to itself

The test would have been failing even w/o the bug.

[Basile Clement]

Created attachment 252119 [details]
Patch

[Filip Pizlo]

Comment on attachment 252119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252119&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4974
> -        LValue structureID = lowCell(m_node->child1());
> +        LValue structure = lowCell(m_node->child1());
> +        LValue structureID = m_out.load32(structure, m_heaps.Structure_structureID);

We should do something to eliminate this load.  Can we make checkStructure() have a mode where it checks structure pointers instead of IDs?

[Basile Clement]

Created attachment 252200 [details]
Patch

[WebKit Commit Bot]

Attachment 252200 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1881:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4980:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 252200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252200&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5324
> -        LValue structureID, const FormattedValue& formattedValue, ExitKind exitKind,
> -        const StructureSet& set)
> +        LValue structureDiscriminant, const FormattedValue& formattedValue, ExitKind exitKind,
> +        const StructureSet& set, std::function<LValue(Structure*)>&& weakStructureDiscriminant)

We usually use a functor instead of std::function.  Something like:

template<typename Functor>
void checkStrcuture(..., const Functor& weakStructureDiscriminant) { ... }

In all other ways, it'll work the same as what you have.

r=me anyways, it's not a big deal.

[Basile Clement]

(In reply to comment #7)
> Comment on attachment 252200 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=252200&action=review
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5324
> > -        LValue structureID, const FormattedValue& formattedValue, ExitKind exitKind,
> > -        const StructureSet& set)
> > +        LValue structureDiscriminant, const FormattedValue& formattedValue, ExitKind exitKind,
> > +        const StructureSet& set, std::function<LValue(Structure*)>&& weakStructureDiscriminant)
> 
> We usually use a functor instead of std::function.  Something like:
> 
> template<typename Functor>
> void checkStrcuture(..., const Functor& weakStructureDiscriminant) { ... }
> 
> In all other ways, it'll work the same as what you have.
> 
> r=me anyways, it's not a big deal.

I used std::function due to the habit of writing more precise types ; I will change that to a functor before landing, if only for the sake of consistency.

[Basile Clement]

Created attachment 252321 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 252321 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1887:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5099:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 252321 [details]
Patch for landing

Clearing flags on attachment: 252321

Committed r183752: <http://trac.webkit.org/changeset/183752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.