1621265 - Assertion failure: !script()->jitScript()->modifiesArguments(), at jit/IonBuilder.cpp:8803

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200310-c7766d0b4a12 (build with --enable-debug, run with --fuzzing-safe --ion-offthread-compile=off --ion-full-warmup-threshold=0 --baseline-eager):

function f73(a35) {
    arguments[gczeal(14)];
    throw new Set;
    a35 = 4;
}
f73(1);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555566174d1 in js::jit::IonBuilder::getElemTryArguments(bool*, js::jit::MDefinition*, js::jit::MDefinition*) ()
#0  0x00005555566174d1 in js::jit::IonBuilder::getElemTryArguments(bool*, js::jit::MDefinition*, js::jit::MDefinition*) ()
#1  0x00005555565f4c09 in js::jit::IonBuilder::jsop_getelem() ()
#2  0x00005555565e27e2 in js::jit::IonBuilder::inspectOpcode(JSOp, bool*) ()
#3  0x00005555565de964 in js::jit::IonBuilder::traverseBytecode() ()
#4  0x00005555565d6106 in js::jit::IonBuilder::build() ()
#5  0x00005555565ca97c in js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned int, unsigned char*, bool) ()
#6  0x00005555565c96b8 in js::jit::CanEnterIon(JSContext*, js::RunState&) ()
#7  0x0000555556666d1a in js::jit::MaybeEnterJit(JSContext*, js::RunState&) ()
#8  0x00005555558ce91b in js::RunScript(JSContext*, js::RunState&) ()
#9  0x00005555558e3d73 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#10 0x00005555563a4bd9 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) ()
#11 0x00001f26abf6ff43 in ?? ()
[...]
#37 0x0000000000000000 in ?? ()
rax	0x555556f89dea	93825019715050
rbx	0x6	6
rcx	0x555557efe850	93825035921488
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffffa7b0	140737488332720
rsp	0x7fffffffa780	140737488332672
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f9cd00	140737353731328
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x7fffffffab10	140737488333584
r13	0x7fffffffae88	140737488334472
r14	0x7fffffffa7c3	140737488332739
r15	0x7ffff4c1f828	140737299740712
rip	0x5555566174d1 <js::jit::IonBuilder::getElemTryArguments(bool*, js::jit::MDefinition*, js::jit::MDefinition*)+865>
=> 0x5555566174d1 <_ZN2js3jit10IonBuilder19getElemTryArgumentsEPbPNS0_11MDefinitionES4_+865>:	movl   $0x2263,0x0
   0x5555566174dc <_ZN2js3jit10IonBuilder19getElemTryArgumentsEPbPNS0_11MDefinitionES4_+876>:	callq  0x5555557eef2e <abort>

Marking s-s because this is a JIT assertion.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

Maybe caused by the 'arguments' changes?

Comment 3

[André Bargull [:anba]]

This is caused by bug 1478350, part 6 (https://hg.mozilla.org/mozilla-central/rev/699e15941bae4650ddba8023ca1e67a5d60c30b4). (That commit added this assertion.)

jit::AnalyzeBytecodeForIon only inspects the bytecode instructions, but otherwise ignores the control flow, which means unreachable instructions are also taken into account. Compared to that, jit::AnalyzeArgumentsUsage uses IonBuilder to build a complete MIR graph, and because IonBuilder knows how to ignore unreachable instructions, the arguments analysis never reaches a35 = 4; and therefore won't build MSetArgumentsObjectArg, which in turn means we enable lazy arguments for this function.

I think we can just remove that assertion.

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1621265: Remove a bogus assertion that assumed any JSOp::SetArg is incompatible with lazy arguments. r=jandem!

It's okay to have JSOp::SetArg and still enable the lazy arguments optimisation,
as long as the JSOp::SetArg is always unreachable.

The assertion was kept from code which was added before the arguments analysis
code rewritten to use IonBuilder. But with the current arguments analysis, it's
no longer useful to have this assertion, because any reachable (!) JSOp::SetArg
will currently always disable lazy arguments, cf. IonBuilder::jsop_setarg().

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59fb272fb31c
Remove a bogus assertion that assumed any JSOp::SetArg is incompatible with lazy arguments. r=jandem

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/59fb272fb31c

Comment 7

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200422214848-17aa41e3cb7c.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.