Bug 144465 - Object allocation not sinking properly through CheckStructure
Summary: Object allocation not sinking properly through CheckStructure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basile Clement
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-30 12:23 PDT by Basile Clement
Modified: 2015-05-04 11:39 PDT (History)
5 users (show)

See Also:


Attachments
Example of the bug (333 bytes, application/x-javascript)
2015-04-30 12:23 PDT, Basile Clement
no flags Details
Good old NaN not being equal to itself (342 bytes, application/x-javascript)
2015-04-30 12:44 PDT, Basile Clement
no flags Details
Patch (5.46 KB, patch)
2015-04-30 17:24 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Patch (7.23 KB, patch)
2015-05-01 16:58 PDT, Basile Clement
no flags Details | Formatted Diff | Diff
Patch for landing (7.28 KB, patch)
2015-05-04 11:05 PDT, Basile Clement
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basile Clement 2015-04-30 12:23:48 PDT
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.
Comment 1 Basile Clement 2015-04-30 12:28:15 PDT
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.
Comment 2 Basile Clement 2015-04-30 12:44:36 PDT
Created attachment 252080 [details]
Good old NaN not being equal to itself

The test would have been failing even w/o the bug.
Comment 3 Basile Clement 2015-04-30 17:24:08 PDT
Created attachment 252119 [details]
Patch
Comment 4 Filip Pizlo 2015-05-01 15:25:30 PDT
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?
Comment 5 Basile Clement 2015-05-01 16:58:37 PDT
Created attachment 252200 [details]
Patch
Comment 6 WebKit Commit Bot 2015-05-01 17:00:44 PDT
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 7 Filip Pizlo 2015-05-01 19:04:33 PDT
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.
Comment 8 Basile Clement 2015-05-04 10:38:52 PDT
(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.
Comment 9 Basile Clement 2015-05-04 11:05:43 PDT
Created attachment 252321 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2015-05-04 11:09:40 PDT
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 11 WebKit Commit Bot 2015-05-04 11:39:18 PDT
Comment on attachment 252321 [details]
Patch for landing

Clearing flags on attachment: 252321

Committed r183752: <http://trac.webkit.org/changeset/183752>
Comment 12 WebKit Commit Bot 2015-05-04 11:39:22 PDT
All reviewed patches have been landed.  Closing bug.