Bug 189628 – Don't dump OSRAvailabilityData in Graph::dump because a stale Availability may point to a Node that is already freed

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

[Saam Barati]

Otherwise, an Availability may point to a node that was removed from the graph.

[Filip Pizlo]

(In reply to Saam Barati from comment #0)
> Otherwise, an Availability may point to a node that was removed from the
> graph.

Or just not report availability in dumping.

I don't think it's a good idea to have dumping cause us to run an analysis.  We want to be able to dump when debugging.  If there's a bug then computing an analysis may trigger another bug.

[Saam Barati]

<rdar://problem/39481690>

[Saam Barati]

(In reply to Filip Pizlo from comment #1)
> (In reply to Saam Barati from comment #0)
> > Otherwise, an Availability may point to a node that was removed from the
> > graph.
> 
> Or just not report availability in dumping.
> 
> I don't think it's a good idea to have dumping cause us to run an analysis. 
> We want to be able to dump when debugging.  If there's a bug then computing
> an analysis may trigger another bug.

Yeah I'm torn on this. I personally find dumping availability helpful. But I also thought through an issue you're describing here. Making an analysis run in dumping could make other analyses break in weird ways when they may be using the availability data structures.

I think we should just remove the dumping, and if someone needs that data dumped, they can manually hack the dumping code.

[Saam Barati]

Created attachment 349801 [details]
patch

[Mark Lam]

Comment on attachment 349801 [details]
patch

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

r=me

> Source/JavaScriptCore/dfg/DFGGraph.cpp:572
> +            if (false)

Can you make this a static const bool at the top of this file?  It makes it easy to enable by just changing the bool.

[Saam Barati]

Created attachment 349802 [details]
patch for landing

Thanks for the review

[WebKit Commit Bot]

Comment on attachment 349802 [details]
patch for landing

Clearing flags on attachment: 349802

Committed r236022: <https://trac.webkit.org/changeset/236022>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.