Make the analysis consider whether a virtual call can GC when all targets are known

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Hazard:

Function 'js::ScriptSourceObject* js::ScriptSourceObject::create(js::ExclusiveContext*, js::ScriptSource*, JS::ReadOnlyCompileOptions*)' has unrooted '__temp_11' of type 'js::ScriptSourceObject*' live across GC call JS::ReadOnlyCompileOptions.element at js/src/jsscript.cpp:1009
    js/src/jsscript.cpp:1009: Call(21,22, __temp_13 := options*.element*())
    js/src/jsscript.cpp:1009: Call(22,23, __temp_12 := ObjectOrNullValue(__temp_13*))
    js/src/jsscript.cpp:1009: Call(23,24, __temp_11*.field:0.field:0.initSlot(1,__temp_12))

The problem is that ReadOnlyCompileOptions::element() is a virtual function call. The implementations are trivial and certainly cannot GC, and the analysis knows this when constructing the callgraph. But when looking for hazards, the callgraph is not involved for FieldCalls, so it assumes that any non-annotated FieldCall can gc.
(Assignee)

Comment 1

5 years ago
Created attachment 8343940 [details] [diff] [review]
Make the analysis consider whether a virtual call can GC when all targets are known

When determining all targets of a virtual function call, record those targets in callgraph.txt. Then when computing gcFunctions, check whether any of them can GC and if not, add the FieldCall into suppressedFunctions. It turns out that suppressedFunctions *already* contained field calls, so this turns out to be straightforward. Note that any virtual function call that cannot be resolved to a known set of target callees will remain the same as it is now, and so will be assumed to GC.

This brings the current shell hazard count down to zero (!).
Attachment #8343940 - Flags: review?(bhackett1024)
Comment on attachment 8343940 [details] [diff] [review]
Make the analysis consider whether a virtual call can GC when all targets are known

Review of attachment 8343940 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/devtools/rootAnalysis/analyzeRoots.js
@@ +237,5 @@
>          var csuName = field.FieldCSU.Type.Name;
>          var fullFieldName = csuName + "." + field.Name[0];
>          if (fieldCallCannotGC(csuName, fullFieldName))
>              return null;
> +        //printErr(fullFieldName + ": " + ((fullFieldName in suppressedFunctions) ? "suppressed" : "cangc"));

rm debugging code

::: js/src/devtools/rootAnalysis/computeCallgraph.js
@@ +151,5 @@
> +                // calls to all possible resolved types, but also record edges
> +                // from this field call to each final callee. When the analysis
> +                // is checking whether an edge can GC and it sees an unrooted
> +                // pointer held live across this field call, it will know
> +                // whether any of the direct callers can GC or not.

s/callers/callees

::: js/src/vm/StructuredClone.cpp
@@ +395,5 @@
> +        if (tag == SCTAG_TRANSFER_MAP_ARRAY_BUFFER) {
> +            if (ownership == JS::SCTAG_TMO_ALLOC_DATA)
> +                js_free(content);
> +        } else if (cb && cb->freeTransfer) {
> +            cb->freeTransfer(buffer, tag, JS::TransferableOwnership(ownership), content, cbClosure);

This change looks unrelated?
Attachment #8343940 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/5c34885094e4
https://hg.mozilla.org/mozilla-central/rev/a55439a67e05
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.