Closed
Bug 1449066
Opened 7 years ago
Closed 7 years ago
Switch hazard builds to GCC 6
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: glandium, Assigned: sfink)
References
Details
Attachments
(2 files)
No description provided.
Reporter | ||
Comment 1•7 years ago
|
||
https://taskcluster-artifacts.net/ACtQvW9VTCS0laZmDuz_sQ/0/public/logs/live_backing.log > ERROR: xgill compiled with 6.4.0 but running inside 4.9.4! is /builds/worker/workspace/sixgill/usr/libexec/sixgill/scripts/wrap_gcc/gcc not using the gcc from the toolchain package?
Flags: needinfo?(sphink)
Reporter | ||
Comment 2•7 years ago
|
||
Gah, the plugin is actually still built with 4.9 after bug 1444543
Depends on: 1444543
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(sphink)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•7 years ago
|
||
Steve, can you check why this is failing with hazards detected? https://queue.taskcluster.net/v1/task/E902sOxZSXCGOYHhf5kLsg/runs/0/artifacts/public/logs/live_backing.log TEST-UNEXPECTED-FAIL 160 rooting hazards detected
Flags: needinfo?(sphink)
Reporter | ||
Comment 5•7 years ago
|
||
And https://queue.taskcluster.net/v1/task/E8yB9qy1QrSO4iABiQzf6g/runs/0/artifacts/public/logs/live_backing.log TEST-UNEXPECTED-FAIL 162 rooting hazards detected TEST-UNEXPECTED-FAIL 47 heap write hazards detected out of 0 allowed
![]() |
||
Updated•7 years ago
|
Attachment #8962615 -
Flags: review?(core-build-config-reviews) → review?(nfroyd)
![]() |
||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8962615 [details] Bug 1449066 - Switch hazard builds to GCC 6. https://reviewboard.mozilla.org/r/231464/#review237064 Yaaaaay!
Attachment #8962615 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 7•7 years ago
|
||
That's odd. When I (just now, as a result of this bug) looked at what was going on, it looked to me like sixgill was being built with gcc 6 but tested with gcc 4. When I switched it to pull in gcc 6 from the toolchain (as you did in this patch), I got weird assembler errors: /tmp//ccUK6FnU.s: Assembler messages: /tmp//ccUK6FnU.s:45: Error: expecting string instruction after `rep' From googling, it seems those are associated with older binutils, but I got the same thing after upgrading to 2.27. Oh well, if it's working for you, I won't complain. Looking at the hazards now.
Assignee | ||
Comment 8•7 years ago
|
||
All of the hazards I looked at are from an invalid call chain -- it thinks tryNewTenuredThing<allowGC = 0>() can call GCRuntime::gc(), which it can't because that's the whole point of AllowGC. But somehow with gcc 6 it thinks it can. Rather strange, given that GCC itself is generating the call; this isn't some weirdness of the analysis code. I will import the changes here and do a try push with --upload-xdbs to see what's going on.
Assignee | ||
Comment 9•7 years ago
|
||
Ugh. Optimization doesn't seem to be happening in the same way. The code says if (MOZ_UNLIKELY(!t && allowGC && otherstuff)) { ... } (allowGC is the constant zero in this case, so the whole thing should be discarded.) GCC 6 is translating that, in this state of optimization at least, to t = refillFreeListFromAnyThread(cx, kind); if (t) { __temp_4 = 0; __temp_3 = MOZ_UNLIKELY(__temp_4); if (__temp_3) { gc(); } } else { ...exact same thing... } checkIncrementalZoneState(cx, t); return t; For comparison, gcc 4.9 produced t = refillFreeListFromAnyThread(cx, kind, thingSize); checkIncrementalZoneState(cx, t); return t; Ugh. I'll work around this with an annotation. I hope the optimizer *does* handle this at some stage. Hm... disassembly seems to say that it does. This whole function is inlined into js::Allocate, and in the case here it does a tail call to refillFreeListFromAnyThread (which is fair because checkIncrementalZoneState is empty if non-DEBUG.) So this appears to be an artifact of DEBUG + different optimizations kicking in before the plugin sees the code.
Assignee | ||
Comment 10•7 years ago
|
||
The annotation appears to mostly work, though I think there's still a single remaining hazard that is unrelated. I'm going to have to run off to bed before I can check the latest results, though: https://treeherder.mozilla.org/#/jobs?repo=try&revision=829ddcd570009ef8d82d7c52715919e1b81b708e (that may need a retry on the decision task if it times out again, as it has been doing recently.) Remaining hazard is in Function 'js::jit::AbortReasonOr<js::jit::MCall*> js::jit::IonBuilder::makeCallHelper(const mozilla::Maybe<mozilla::Vector<JSFunction*, 6ul, js::jit::JitAllocPolicy> >&, js::jit::CallInfo&)' has unrooted 'target' of type 'JSFunction*' live across GC call 'js::jit::AbortReasonOr<bool> js::jit::IonBuilder::testShouldDOMCall(js::TypeSet*, JSFunction*, JSJitInfo::OpType)' at js/src/jit/IonBuilder.cpp:5557 where that GC call invokes an "instanceChecker" function pointer. I'll look into it tomorrow. I thought this stuff generally runs within a GC suppression?
Assignee | ||
Comment 11•7 years ago
|
||
Requesting review on the annotations so far, though they're pretty simple.
Attachment #8963464 -
Flags: review?(jcoppeard)
Assignee | ||
Updated•7 years ago
|
Assignee: mh+mozilla → sphink
Status: NEW → ASSIGNED
Updated•7 years ago
|
Attachment #8963464 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 12•7 years ago
|
||
Oh, bleh. This looks like a real issue; the internal CFG must have added something that sixgill doesn't know how to handle. The hazard is because of a missing call edge from IonBuilder::setPropTryCommonSetter to IonBuilder::makeCallHelper from the line MOZ_TRY_VAR(call, makeCallHelper(targets, callInfo)); sixgill's translation of that line is Assign(119,120, mozTryVarTempResult_:1 := error*) 'error' is not a variable; it's a sixgill fallback for stuff it doesn't understand but so far hasn't been enough of a problem to matter. MOZ_TRY_VAR is #define MOZ_TRY_VAR(target, expr) \ do { \ auto mozTryVarTempResult_ = (expr); \ if (mozTryVarTempResult_.isErr()) { \ return ::mozilla::Err( \ mozTryVarTempResult_.unwrapErr()); \ } \ (target) = mozTryVarTempResult_.unwrap(); \ } while (0) so really it's the translation of auto mozTryVarTempResult_ = makeCallHelper(targets, callInfo); For the record, makeCallHelper returns js::jit::AbortReasonOr<js::jit::MCall*> which is mozilla::Result<MCall*, AbortReason> which ends up boiling down to roughly struct { uint8 rawData[8]; tag uint8; }. (Which is a little sad, because there are only 6 AbortReasons and they would fit in the 3 low-order bits of an aligned pointer.) I'll have to step through sixgill's processing of this line.
Flags: needinfo?(sphink)
Comment 13•7 years ago
|
||
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/870f0aeb6ade Switch hazard builds to GCC 6, r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8778d19c40 Annotate more func<AllowGC::NoGC> as not being able to GC, r=jonco
Assignee | ||
Comment 14•7 years ago
|
||
Er, oops. I kinda pushed my local import of the reviewboard patch along with my patches. (And it is incorrectly listed as being written by me, because the reviewboard link I used only gave me the diff, and I didn't bother to look further because I only wanted it for testing.) On the bright side, it landed and it's green. :-)
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/870f0aeb6ade https://hg.mozilla.org/mozilla-central/rev/3f8778d19c40
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Version: Version 3 → 3 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•