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.
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.
Created attachment 252080 [details] Good old NaN not being equal to itself The test would have been failing even w/o the bug.
Created attachment 252119 [details] Patch
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?
Created attachment 252200 [details] Patch
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.
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.
(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.
Created attachment 252321 [details] Patch for landing
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.
Comment on attachment 252321 [details] Patch for landing Clearing flags on attachment: 252321 Committed r183752: <http://trac.webkit.org/changeset/183752>
All reviewed patches have been landed. Closing bug.