Rooting hazards in asm.js

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The static analysis complains about 11 hazards from js::AsmJSModule::Global, which contains a HeapPtrPropertyName as well as a bare Value. I am uncertain whether these hazards are real.
Posted patch Rooting hazards in asm.js (obsolete) — Splinter Review
This would seem to be safer. First, it traces through the Value in addition to the HeapPtrPropertyName because I couldn't see any reason why it wasn't needed. Unless Value is never a gcthing? I'm not sure what constant_ is for.

Second, it stops passing Global around on the stack, because it seems like the Globals are already rooted and firing the HeapPtrPropertyName destructor (and therefore post barrier) seems like a bad idea.

There are other generational GC hazards here, and this patch won't stop the analysis from complaining, but I'll need to talk to Luke before I can make further headway.
Attachment #735487 - Flags: review?(terrence)
Attachment #735487 - Flags: review?(luke)
Comment on attachment 735487 [details] [diff] [review]
Rooting hazards in asm.js

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

Yeah, I think this should be fine for exact rooting. GGC is going to be much harder to support between the union and the Vector.

Is the list of static analysis workarounds in the tree somewhere?

::: js/src/ion/AsmJSModule.h
@@ +79,5 @@
>          void trace(JSTracer *trc) {
>              if (name_)
>                  MarkString(trc, &name_, "asm.js global name");
> +            if (which_ == Variable && u.var.initKind_ == InitConstant)
> +                gc::MarkValueRoot(trc, &u.var.init.constant_, "asm.js global init constant");

MarkValueUnbarriered
Attachment #735487 - Flags: review?(terrence) → review+
Updating marking code after hallway conversation w/terrence.
Attachment #735539 - Flags: review?(terrence)
Attachment #735539 - Flags: review?(luke)
Attachment #735487 - Attachment is obsolete: true
Attachment #735487 - Flags: review?(luke)
Comment on attachment 735539 [details] [diff] [review]
Rooting hazards in asm.js

Oh, sorry, posted an updated patch without checking for changes. Carrying over r+. I still want Luke to weigh in before I land this, though.
Attachment #735539 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #2)
> Is the list of static analysis workarounds in the tree somewhere?

I think what you mean is what's in js/src/devtools/rootAnalysis/annotations.js. It has a couple of different ways of ignoring certain things from the perspective of the analysis.
Comment on attachment 735539 [details] [diff] [review]
Rooting hazards in asm.js

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

Nice, thanks!

::: js/src/ion/AsmJSModule.h
@@ +79,5 @@
>          void trace(JSTracer *trc) {
>              if (name_)
>                  MarkString(trc, &name_, "asm.js global name");
> +            if (which_ == Variable && u.var.initKind_ == InitConstant)
> +                gc::MarkValueUnbarriered(trc, &u.var.init.constant_, "asm.js global init constant");

I guess you could do this, but, for very many reasons, this Value will only ever hold an int32/double.  I'd kinda rather not.
Attachment #735539 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #6)
> ::: js/src/ion/AsmJSModule.h
> @@ +79,5 @@
> >          void trace(JSTracer *trc) {
> >              if (name_)
> >                  MarkString(trc, &name_, "asm.js global name");
> > +            if (which_ == Variable && u.var.initKind_ == InitConstant)
> > +                gc::MarkValueUnbarriered(trc, &u.var.init.constant_, "asm.js global init constant");
> 
> I guess you could do this, but, for very many reasons, this Value will only
> ever hold an int32/double.  I'd kinda rather not.

I was speculating that this might be the case. If so, then I'd rather switch the mark to an assert, comment on the field, and whitelist the whole type for the static analysis.

What about switching the parameters to references? I was doing that because constructing and destructing HeapPtrPropertyName on the stack all the time seemed bad. But I guess that sort of means that I want Global to be a heap-only class, so maybe its copy constructor should be removed or something?
(In reply to Steve Fink [:sfink] from comment #7)
> (In reply to Luke Wagner [:luke] from comment #6)
> > ::: js/src/ion/AsmJSModule.h
> > @@ +79,5 @@
> > >          void trace(JSTracer *trc) {
> > >              if (name_)
> > >                  MarkString(trc, &name_, "asm.js global name");
> > > +            if (which_ == Variable && u.var.initKind_ == InitConstant)
> > > +                gc::MarkValueUnbarriered(trc, &u.var.init.constant_, "asm.js global init constant");
> > 
> > I guess you could do this, but, for very many reasons, this Value will only
> > ever hold an int32/double.  I'd kinda rather not.
> 
> I was speculating that this might be the case. If so, then I'd rather switch
> the mark to an assert, comment on the field, and whitelist the whole type
> for the static analysis.

Sounds good

> What about switching the parameters to references?

That seems like the right fix.

> But I guess that sort of means that I want Global to be a
> heap-only class, so maybe its copy constructor should be removed or
> something?

I was considering asking you to do this, but I think they are stored in a Vector so that'd be annoying.
> > But I guess that sort of means that I want Global to be a
> > heap-only class, so maybe its copy constructor should be removed or
> > something?
> 
> I was considering asking you to do this, but I think they are stored in a
> Vector so that'd be annoying.

Ok. Though we're actually covering up a problem for terrence, because when the Vector moves the HeapPtrs around, it messes up his storebuffer links. But he's aware of the problem.
(In reply to Steve Fink [:sfink] from comment #9)
> > > But I guess that sort of means that I want Global to be a
> > > heap-only class, so maybe its copy constructor should be removed or
> > > something?
> > 
> > I was considering asking you to do this, but I think they are stored in a
> > Vector so that'd be annoying.
> 
> Ok. Though we're actually covering up a problem for terrence, because when
> the Vector moves the HeapPtrs around, it messes up his storebuffer links.
> But he's aware of the problem.

Actually, I did a little happy dance when I saw that constant_ is int32/double only because it means this isn't a problem anymore.
Attachment #735539 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/350baac4bbdb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.