1182133 - Shell test failures due to arguments weirdness when treating the tests as runOnce

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

To reproduce, apply the patch in bug 1181908 and run js/src/jit-test/tests/asm.js/testModuleFunctions.js with --args="--ion-eager --ion-offthread-compile=off"

We end up in ICTypeUpdate_PrimitiveSet::Compiler::getStub with our flags including TypeToFlag(JSVAL_TYPE_MAGIC) and fail the assert there.

The problem is that in ICUpdatedStub::addUpdateStubForValue we have:

(gdb) p val.ptr->data.debugView.tag
$14 = JSVAL_TAG_MAGIC
(gdb) p val.ptr->data.s.payload.why
$15 = JS_OPTIMIZED_ARGUMENTS

and this apparently tests true for isPrimitive() but the IC code then asserts that we don't have a JSVAL_TYPE_MAGIC.

Talking to npb, jandem, shu on IRC today, we tracked this down to the hasStaticScopeObject() call in IonBuilder::jsop_setaliasedvar returning true but the call being null; the rval is a constant optimized arguments, and we end up setting it on the callobject... and then later things blow up.  I don't know offhand why this causes problems in _baseline_ ICs later.

The claim was made that in this case we should not have setaliasedvar with an optimized arguments at all.  It comes from the lambda prologue.

It seems that we have an optimized arguments because in jit::AnalyzeArgumentsUsage we have script->bindingsAccessedDynamically() set to true, so we setNeedsArgsObj(false).

Also, in Parser<FullParseHandler>::checkFunctionArguments we had maybeArgDef null, so even though pc->sc->bindingsAccessedDynamically() is true we never call funbox->setDefinitelyNeedsArgsObj().

This is a kind of weird situation in that it's hard to reproduce without --ion-eager: we apparently need to be compiling and running in ion the prologue of a run-once lambda, which is hard to do normally.

Anyway, what's not clear to me at this point is what exactly the invariants are here.  Clearly we're forcing argumentsHasBinding and argumentsHasLocalBinding in checkFunctionArguments, because bindingsAccessedDynamically().  That's what outputs the setaliasedvar in the prologue, based on BytecodeEmitter::emitFunctionScript.  But is it ok that our (totally unused!) arguments is JS_OPTIMIZED_ARGUMENTS in this case?  And if so, do we just need to fix baseline to deal?  And if not, who's responsible for deoptimizing it and why do we need it deoptimized?

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #0)
> But is it ok that
> our (totally unused!) arguments is JS_OPTIMIZED_ARGUMENTS in this case?

I wanted to say "No.", but it looks like this case is possible since bug 842522. Eval itself does not force non-lazy arguments, only if the eval actually uses the arguments binding.

> And if so, do we just need to fix baseline to deal?

Yes it looks like it. Do you want to fix this?

Comment 2

[Jan de Mooij [:jandem]]

Also, I don't understand how your runOnce patches affect this. inEval should definitely not be treated as a run-once lambda; I'll take a look.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Do you want to fix this?

I would love it if someone else did, actually; I have a ton of stuff on my plate right now.  :(

> Also, I don't understand how your runOnce patches affect this. 

Mainly in terms of making hasStaticScopeObject() return true, but also it changes which exact MIR op we have for the callobject...

You're right that inEval shouldn't be ending up as a run-once lambda.  I'm not sure what the story with it is and why it ends up having a runoncecallobject afaict in some cases.....

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Also, I don't understand how your runOnce patches affect this. inEval should
> definitely not be treated as a run-once lambda; I'll take a look.

OK, I think it's this line in JSScript::Create:

    script->treatAsRunOnce_ = options.isRunOnce;

options is indeed a ReadOnlyCompileOptions. This looks wrong, I think we should only do this if the script is not a function script (but global or eval).

treatAsRunOnce can also be set for run-once lambdas, but that case will be handled in the emitter AFAIK.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Aha!  Yes, that is in fact the problem.

Without the patch for bug 1181908, the CompileOptions we create in BytecodeEmitter::emitFunction doesn't copy the isRunOnce flag, so we never end up with options.isRunOnce being true for a function script.  Going to mark this invalid and take this up in bug 1181908.