1205463 - Remove IONMASM and replace with real rooting usage

Status: NEW

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: remove_IONMASM-v0.diff

I ran the hazard analysis locally to pinpoint what needs rooting and found... nothing. I think we're probably protected by other primitives an this is legacy. Running a try run to see if that is likely true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=229902264ffa

Steve, can you think of anything that we could do to increase our confidence that the analysis is not just missing something?

Comment 1

[Terrence Cole [:terrence]]

At Jan's suggestion, I added an AutoCheckCannotGC to MacroAssember. This causes all asm.js tests to fail because validation does a Parse, which allocates, however, the rt->mainThread.suppressGC is not set at this time. I'm not sure if or where we should be suppressing in asm.js.

Comment 2

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

Hrm. LinkCodeGen is not recognized as a GC Function, which is why that wasn't reported as a hazard. Let's see...

(Cmd) caller LinkCodeGen
2 callers of #774540 = Ion.cpp:uint8 LinkCodeGen(JSContext*, js::jit::IonBuilder*, js::jit::CodeGenerator*, class JS::AutoVectorRooter<JSScript*>*, OnIonCompilationInfo*)
  #774544 = Ion.cpp:uint8 LinkBackgroundCodeGen(JSContext*, js::jit::IonBuilder*, class JS::AutoVectorRooter<JSScript*>*, OnIonCompilationInfo*)
  #774736 = (SUPPRESSED) Ion.cpp:uint32 js::jit::IonCompile(JSContext*, JSScript*, js::jit::BaselineFrame*, uint8*, uint8, uint8, uint32)
(Cmd) caller #774544
2 callers of #774544 = Ion.cpp:uint8 LinkBackgroundCodeGen(JSContext*, js::jit::IonBuilder*, class JS::AutoVectorRooter<JSScript*>*, OnIonCompilationInfo*)
  #774548 = (SUPPRESSED) uint8* js::jit::LazyLinkTopActivation(JSContext*)
  #754773 = (SUPPRESSED) void js::jit::AttachFinishedCompilations(JSContext*)

Ok, so all routes to it go through a suppression. My graph traversal tool doesn't know *why* things are suppressed, so let me look...  IonCompile's call is suppressed because it's within an AutoEnterAnalysis. LazyLink doesn't call LinkBackgroundCodeGen in my current checkout, but that's probably because I'm looking at an old callgraph, and in my source LazyLinkTopActivation calls LazyLink calls LinkBackgroundCodeGen within an AEA. AttachedFinishedCompilations has an AEA around almost its entire body.

So I think jandem already said this on irc, but it's not a hazard because of AutoEnterAnalysis, which contains an AutoSuppressGC field. But you said "...the rt->mainThread.suppressGC is not set at this time." I can't explain that.

Comment 3

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

Comment on attachment 8662092 [details] [diff] [review]
remove_IONMASM-v0.diff

f+ for the reasons given in the last comment, though note that I didn't fully figure out what was going on. But I do agree with the hazard analysis wrt what it's seeing here.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Terrence Cole [:terrence] from comment #1)
> At Jan's suggestion, I added an AutoCheckCannotGC to MacroAssember. This
> causes all asm.js tests to fail because validation does a Parse, which
> allocates, however, the rt->mainThread.suppressGC is not set at this time.
> I'm not sure if or where we should be suppressing in asm.js.

What's probably saving us here is that asm.js should not embed any GC pointers in the generated code, so ideally we'd assert this and suppress the AutoCheckCannotGC in this case.