Mike Hommey [:glandium] Reporter
	Description • 7 years ago

      No description provided.

	Mike Hommey [:glandium] 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?

	Mike Hommey [:glandium] Reporter
	Comment 2 • 7 years ago

Gah, the plugin is actually still built with 4.9 after bug 1444543

	Mike Hommey [:glandium] 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

	Mike Hommey [:glandium] 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

	Nathan Froyd [:froydnj]
	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!

	Steve Fink [:sfink] [:s:] 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.

	Steve Fink [:sfink] [:s:] 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.

	Steve Fink [:sfink] [:s:] 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.

	Steve Fink [:sfink] [:s:] 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?

	Steve Fink [:sfink] [:s:] Assignee
	Comment 11 • 7 years ago

Requesting review on the annotations so far, though they're pretty simple.

	Steve Fink [:sfink] [:s:] 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.

	Pulsebot
	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

	Steve Fink [:sfink] [:s:] 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. :-)

	Natalia Csoregi [:nataliaCs]
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/870f0aeb6ade
https://hg.mozilla.org/mozilla-central/rev/3f8778d19c40