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
•