Open
Bug 1205463
Opened 9 years ago
Updated 1 year ago
Remove IONMASM and replace with real rooting usage
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox43 | --- | affected |
People
(Reporter: terrence, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.07 KB,
patch
|
sfink
:
feedback+
|
Details | Diff | Splinter Review |
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?
Attachment #8662092 -
Flags: feedback?(sphink)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8662092 -
Flags: feedback?(sphink) → feedback+
Comment 4•9 years ago
|
||
(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.
Updated•3 years ago
|
Assignee: terrence.d.cole → nobody
Status: ASSIGNED → NEW
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•