Bug 195721 - DFG::AbstractValue::validateOSREntry is wrong when isHeapTop and the incoming value is Empty
Summary: DFG::AbstractValue::validateOSREntry is wrong when isHeapTop and the incoming...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-13 18:12 PDT by Saam Barati
Modified: 2019-03-20 22:44 PDT (History)
15 users (show)

See Also:


Attachments
WIP (22.27 KB, patch)
2019-03-20 16:10 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.99 KB, patch)
2019-03-20 16:48 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.99 KB, patch)
2019-03-20 16:51 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-03-13 18:12:54 PDT
```
    bool validate(JSValue value) const
    {
        if (isHeapTop())
            return true;
```

However, JSValue() is not part of heap top, but would return true if asked to validate on an AbstractValue that isHeapTop().
Comment 1 Filip Pizlo 2019-03-13 18:24:39 PDT
(In reply to Saam Barati from comment #0)
> ```
>     bool validate(JSValue value) const
>     {
>         if (isHeapTop())
>             return true;
> ```
> 
> However, JSValue() is not part of heap top, but would return true if asked
> to validate on an AbstractValue that isHeapTop().

JSValue() in AbstractValue means that the value is “any JSValue”. There is no way to reflect folding to JSValue() in abstract value.
Comment 2 Saam Barati 2019-03-13 18:48:58 PDT
(In reply to Filip Pizlo from comment #1)
> (In reply to Saam Barati from comment #0)
> > ```
> >     bool validate(JSValue value) const
> >     {
> >         if (isHeapTop())
> >             return true;
> > ```
> > 
> > However, JSValue() is not part of heap top, but would return true if asked
> > to validate on an AbstractValue that isHeapTop().
> 
> JSValue() in AbstractValue means that the value is “any JSValue”. There is
> no way to reflect folding to JSValue() in abstract value.

The scenario I'm worried about is OSR entry.

Let's say that this AbstractValue has type SpecHeapTop (and top for all the other things too). Let's say we deleted a CheckTDZ because we saw the type is SpecHeapTop. Let's also say that we're now OSR entering with the TDZ value (JSValue()). I think that would lead to a bug.
Comment 3 Filip Pizlo 2019-03-20 15:00:48 PDT
(In reply to Saam Barati from comment #2)
> (In reply to Filip Pizlo from comment #1)
> > (In reply to Saam Barati from comment #0)
> > > ```
> > >     bool validate(JSValue value) const
> > >     {
> > >         if (isHeapTop())
> > >             return true;
> > > ```
> > > 
> > > However, JSValue() is not part of heap top, but would return true if asked
> > > to validate on an AbstractValue that isHeapTop().
> > 
> > JSValue() in AbstractValue means that the value is “any JSValue”. There is
> > no way to reflect folding to JSValue() in abstract value.
> 
> The scenario I'm worried about is OSR entry.
> 
> Let's say that this AbstractValue has type SpecHeapTop (and top for all the
> other things too). Let's say we deleted a CheckTDZ because we saw the type
> is SpecHeapTop. Let's also say that we're now OSR entering with the TDZ
> value (JSValue()). I think that would lead to a bug.

I'm not saying that it wouldn't.  I'm just saying that AbstractValue::m_value being JSValue() means "top JSValue".

So if AbstractValue::m_value is empty, it doesn't mean JSValue().
Comment 4 Saam Barati 2019-03-20 16:10:47 PDT
Created attachment 365426 [details]
WIP

Currently crashing in a lot of tests. I need to figure out why.
Comment 5 Saam Barati 2019-03-20 16:48:52 PDT
Created attachment 365437 [details]
patch
Comment 6 EWS Watchlist 2019-03-20 16:51:33 PDT
Attachment 365437 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h:235:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/testdfg.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Saam Barati 2019-03-20 16:51:35 PDT
Created attachment 365439 [details]
patch
Comment 8 EWS Watchlist 2019-03-20 16:55:16 PDT
Attachment 365439 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGStructureAbstractValue.h:235:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/testdfg.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Yusuke Suzuki 2019-03-20 16:58:39 PDT
Comment on attachment 365439 [details]
patch

r=me too.
Comment 10 WebKit Commit Bot 2019-03-20 22:43:14 PDT
Comment on attachment 365439 [details]
patch

Clearing flags on attachment: 365439

Committed r243278: <https://trac.webkit.org/changeset/243278>
Comment 11 WebKit Commit Bot 2019-03-20 22:43:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-03-20 22:44:49 PDT
<rdar://problem/49095372>