1154556 - OdinMonkey: MIPS: Fix jit-test asm.js/testBug1046688.js failure

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[hev [:hev]]

Starting program: /home/heiher/git/loongson/gecko-dev/js/src/mips/js/src/shell/js -f /home/heiher/git/loongson/gecko-dev/js/src/jit-test/lib/prologue.js --js-cache /home/heiher/git/loongson/gecko-dev/js/src/jit-test/.js-cache -e const\ platform=\'linux2\'\;\ const\ libdir=\'/home/heiher/git/loongson/gecko-dev/js/src/jit-test/lib/\'\;\ const\ scriptdir=\'/home/heiher/git/loongson/gecko-dev/js/src/jit-test/tests/asm.js/\' -f /home/heiher/git/loongson/gecko-dev/js/src/jit-test/tests/asm.js/testBug1046688.js
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
Assertion failure: fp_ == nullptr, at /home/heiher/git/loongson/gecko-dev/js/src/vm/Stack.cpp:1645
[New Thread 0x7ffff38f5700 (LWP 27558)]
[New Thread 0x7ffff3976700 (LWP 27557)]
[New Thread 0x7ffff39f7700 (LWP 27556)]
[New Thread 0x7ffff3a78700 (LWP 27555)]
[New Thread 0x7ffff3af9700 (LWP 27554)]
[New Thread 0x7ffff3b7a700 (LWP 27553)]
[New Thread 0x7ffff3bfb700 (LWP 27552)]
[New Thread 0x7ffff3c7c700 (LWP 27551)]
[New Thread 0x7ffff3cfd700 (LWP 27550)]
[New Thread 0x7ffff3d7e700 (LWP 27549)]
[New Thread 0x7ffff3dff700 (LWP 27548)]
[New Thread 0x7ffff40d7700 (LWP 27547)]

Program received signal SIGSEGV, Segmentation fault.
0x0000000000737816 in js::AsmJSActivation::~AsmJSActivation (this=0x7fffffffa540, __in_chrg=<optimized out>)
    at /home/heiher/git/loongson/gecko-dev/js/src/vm/Stack.cpp:1645
1645        MOZ_ASSERT(fp_ == nullptr);

Comment 1

[hev [:hev]]

Attachment: OdinMonkey-MIPS-Fix-AsmJSModule-setProfilingEnabled.patch

In asm.js/testBug1046688.js, The SPSProfiling enabled by call enableSPSProfiling. At first iterate, call asmjs from interpreter and setProfilingEnabled called to patch profiling switcher, at next call asmjs from ion code, and AsmJSModule will be clone for ion, codes copied by memcpy (not modified), so profiling switcher target is right (goto begin, not entry).

In staticallyLink, every relativeLinks target will be patched to link.targetOffset (entry, not begin). The problem is link.targetOffset not update in setProfilingEnabled.

Comment 2

[Branislav Rankov [:rankov]]

Comment on attachment 8592581 [details] [diff] [review]
OdinMonkey-MIPS-Fix-AsmJSModule-setProfilingEnabled.patch

I'm not the right person to review this. I'm not that familiar with the profiler. I'm guessing Luke would be better choice.

Comment 3

[Luke Wagner [:luke]]

Nice job tracking this down!

I'm a bit worried that this makes enabling profiling cost O(num-calls * num-relative-links) which could be pretty expensive.

While thinking about this, I noticed that there are several related subtle bugs when cloning is combined with profiling: the staticallyLink after the clone doesn't take profilingEnabled_ into account and so the builtin-thunks and function-pointer-tables point to the non-profiling entries.  I think the solution is to instead fix staticallyLink to take profilingEnabled_ into account (including this callsite-relative-link MIPS case).  I can write a patch to do this.

Comment 4

[Luke Wagner [:luke]]

Attachment: change-func-ptr-tables

This is a preparatory patch to change function-pointer tables not to use RelativeLink at all, but instead their own thing.  This leads to a few simplifications and also an AsmJSModule memory reduction (before, each element would take two uint32_t's (offset and target); now each element takes one uint32_t since the offset is implicit).

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8593506 [details] [diff] [review]
change-func-ptr-tables

Actually, this patch makes solving the identified problem harder, so n/m :)

Comment 6

[Luke Wagner [:luke]]

Attachment: rm-dead-field

Random cleanup I found.

Comment 7

[Luke Wagner [:luke]]

Attachment: simplify-icache-flushing

This patch removes the terrible non-local reasoning required about the AutoFlushICache surrounding cloning.  The AutoFluchICache just serves to inhibit flushing until the final dynamic linking step (when we flush the icache for the entire module's code), so there's no issue splitting one guard into two.  Lastly, I found a place where it was missing (which is just a perf bug b/c it means we'll be doing many spurious mini-flushes).

Comment 8

[Luke Wagner [:luke]]

Attachment: fix-clone-profiling

Does this patch (applied on top of 'simplify-icache-flushing') fix the assert for you?  With this patch, we take profilingEnabled_ into account in staticallyLink to pick the correct entry.

Comment 9

[hev [:hev]]

Comment on attachment 8593604 [details] [diff] [review]
fix-clone-profiling

Review of attachment 8593604 [details] [diff] [review]:
-----------------------------------------------------------------

It works on mips, thanks!

[heiher@loongson jit-test]$ python2 ./jit_test.py -f --tbpl ../mips64-release/js/src/shell/js asm.js/testBug1046688.js
[6|0|0|0] 100% ======================================================>|   7.7s
PASSED ALL
[heiher@loongson jit-test]$ python2 ./jit_test.py -f --tbpl ../mips64-release/js/src/shell/js
[27875|    0|    0|    0] 100% ======================================>|3223.6s
PASSED ALL

Comment 10

[Luke Wagner [:luke]]

Attachment: fix-clone-profiling

Tidied up a bit for review.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8593599 [details] [diff] [review]
rm-dead-field

Review of attachment 8593599 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8593603 [details] [diff] [review]
simplify-icache-flushing

Review of attachment 8593603 [details] [diff] [review]:
-----------------------------------------------------------------

Ha, indeed simpler.

::: js/src/asmjs/AsmJSModule.cpp
@@ +930,5 @@
>      MOZ_ASSERT_IF(active(), activation()->exitReason() == AsmJSExit::Reason_JitFFI ||
>                              activation()->exitReason() == AsmJSExit::Reason_SlowFFI);
>  
> +    AutoFlushICache afc("AsmJSModule::detachHeap");
> +    setAutoFlushICacheRange();

Is this one really needed? Nor ARM neither MIPS seem to touch the code in restoreHeapToInitialState, unless i'm missing some side-effect call. Or are you putting this here in case this changes in the future?

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8594127 [details] [diff] [review]
fix-clone-profiling

Review of attachment 8594127 [details] [diff] [review]:
-----------------------------------------------------------------

Straightforward! For slow and fast FFI calls, I guess the callees add profiling information by themselves, and thus we don't need to change anything there?

::: js/src/asmjs/AsmJSModule.cpp
@@ +788,5 @@
> +            // Builtin calls are another case where, when profiling is enabled,
> +            // we must point to the profiling entry.
> +            AsmJSExit::BuiltinKind builtin;
> +            if (profilingEnabled_ && ImmKindIsBuiltin(imm, &builtin)) {
> +                const AsmJSModule::CodeRange* codeRange = lookupCodeRange(patchAt);

nit: the AsmJSModule:: namespacing can get away

::: js/src/asmjs/AsmJSModule.h
@@ +580,5 @@
>  
>          uint32_t begin() const {
>              return begin_;
>          }
> +        uint32_t profilingEntry() const {

Nice! It makes the API even easier to understand. I was even wondering whether we should just delete 'begin()' and replace all uses with 'profilingEntry', but i like the begin/end symmetry used in several functions, so we're fine like this.

::: js/src/jit-test/tests/asm.js/testProfiling.js
@@ +113,2 @@
>  }
>  for (name of ['sin', 'cos', 'tan', 'asin', 'acos', 'atan', 'ceil', 'floor', 'exp', 'log'])

pre-existing: 'abs' should be in this list too

@@ +125,2 @@
>  }
>  for (name of ['ceil', 'floor'])

pre-existing: 'abs' should be in this list too

Comment 14

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> Is this one really needed? Nor ARM neither MIPS seem to touch the code in
> restoreHeapToInitialState, unless i'm missing some side-effect call. Or are
> you putting this here in case this changes in the future?

That's a keen observation and you're right, I'm putting it here to maintain the simple invariant that all callers of restoreHeapToInitialState have ignored flushing in the entire module and in case something changes one day.

Comment 15

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> Straightforward! For slow and fast FFI calls, I guess the callees add
> profiling information by themselves, and thus we don't need to change
> anything there?

That's right: the FFI paths have a single always-profiling prologue.  (The reason is that, since we're leaving asm.js, we need AsmJSActivation::fp_ not just for profiling, but also normal sync stack-walking.)

> pre-existing: 'abs' should be in this list too

Funny thing about 'abs' is that it doesn't generate an actual builtin call so it doesn't show up in callstacks :)

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/581c5fe6d100
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d3dd3582cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2db22de94b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581c5fe6d100
https://hg.mozilla.org/mozilla-central/rev/a5d3dd3582cb
https://hg.mozilla.org/mozilla-central/rev/9d2db22de94b