1517829 - Codegen.py argument conditional rooting confuses the hazard analysis

Status: NEW

(Core :: DOM: Bindings (WebIDL), enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

I would like the opinion of an experienced Codegen.py hacker on an issue I'm uncovering through some other work.

If I teach the hazard analysis that things like Optional<T> and Maybe<T> might contain GC pointers (if T is or contains a GC pointer), then I get several hazards of the form:

  StringObjectRecordOrLong arg0; // GC pointer
  StringObjectRecordOrLongArgument arg0_holder(arg0); // Conditionally roots, eventually, if needed
  done = (failed = !arg0_holder.TrySetToLong(cx, args[0], tryNext)) || !tryNext; // Can GC
  self->PassUnion22(cx, Constify(arg0)); // Uses arg0

This is safe, because arg0_holder roots arg0, but the analysis doesn't know this because (1) it's a separate variable, and (2) it actually only does the rooting if you store something that requires it.

One solution would be to use arg0_holder (or arg0_holder.Get()) everywhere in place of arg0, since I *Argument inherits from CustomAutoRooter. I attempted to implement that, but had trouble.

Another possibility would be to give StringObjectRecordOrLong a trace() method and then declare arg0 as Rooted<StringObjectRecordOrLong>. I don't know if the *Argument type is still needed for other reasons? It would also be very slightly slower, since it would root unconditionally (though it seems like there's already some bookkeeping to track the state, so I don't think the cost would be observable.)

Yet another option would be to leave Codegen.py mostly alone, and annotate these things as "trust me, this is safe". I don't have a great annotation mechanism for this purpose yet -- I don't store annotations on local variables, so I can't annotate the arg0 declaration.

I could do it with an annotation on the whole function ("ignore hazards in this function"), but it seems like a shame to lose the rooting checks so broadly.

Codegen.py could annotate the StringObjectRecordOrLong type as rooted, hopefully restricted to types that use a rooting holder. That's still bypassing a lot of analysis, especially if the type is used other places.

Hm... relevant *Argument types could be annotated as JS_HAZ_GC_SUPPRESSED. That would make them pretend like the GC is suppressed within their RAII scope. That's probably about the narrowest exclusion so far.

The fancy solution would be to teach the analysis that *Argument types root the variable passed into their constructor. Right now, I do things on a per-variable basis, and search the control flow graph backwards, so that's a bit of a pain. But it could be done. I guess before doing that work I'd want to check how doable the first two solutions here are. I would like to eliminate use of CustomAutoRooter anyway, so if it's about as hard to do, then it's a better way forward.

The ultimate hacky solution would be to pattern match the variable name on /^arg\d+$/ and ignore it. Or ignore it in files with "Bindings" in their name, or something. I might do that in the short term.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Boris might have thoughts to share? :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Generally speaking I've been meaning to move more stuff to Rooted instead of CustomAutoRooter in bindings, but haven't been able to justify making it a sufficient priority to spend the time it needs.  Maybe this is sufficient justification...

FooOrBarArgument also stores the actual "JS value to IDL value" conversion logic for FoorOrBar's members.  We might be able to move that logic somewhere else, somehow; it's not entirely clear.  But that's the other reason that class exists, in addition to the rooting it does.