IonMonkey: Build of revision ec53c5d4c3dd crashes on Kraken/imaging-desaturate

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: djvj, Assigned: nbp)

Tracking

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
When running Kraken benchmark, js shell crashes (with SEGV) on the imaging-desaturate test, but ONLY WHEN it is NOT the first test that's run.  If imaging-desaturate is the first test, or if it is removed from the list of tests to run, then tests run to completion.

JS built with: ./configure --enable-ion --disable-debug --enable-optimize

Run on Kraken from Mercurial repository http://hg.mozilla.org/projects/kraken, revision e119421cb325, using command: ./sunspider --suite=kraken-1.1 --shell=/path/to/js --args='--ion -n' --runs=10

Machine info: Ubuntu 11.10, Linux 3.0.0-17-generic #30-Ubuntu SMP Thu Mar 8 20:45:39 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux, 64-bit Intel(R) Xeon(R) CPU E31225 @ 3.10GHz

Stack trace:
#0  MapAllocToTraceKind (thingKind=Cannot access memory at address 0x18) at js/src/jsgc.h:1020
#1  GetGCThingTraceKind (thing=0x0) at js/src/jsgcinlines.h:63
#2  js::gc::MarkThingOrValueUnbarriered (trc=0x7ff0727ac180, word=0x7ff073e17547, name=0x773e68 "imm-gc-word")
    at js/src/jsgcmark.cpp:517
#3  0x00000000006dfaba in TraceDataRelocations (reader=...,
    buffer=0x7ff073e17508 "H\203\354(H\203\304(H\203\354(I\273P\340}r\360\177", trc=0x7ff0727ac180)
    at js/src/ion/shared/Assembler-x86-shared.cpp:70
#4  js::ion::AssemblerX86Shared::TraceDataRelocations (trc=0x7ff0727ac180, code=<optimized out>, reader=...)
    at js/src/ion/shared/Assembler-x86-shared.cpp:77
#5  0x0000000000684a76 in js::ion::IonCode::trace (this=0x7ff072138330, trc=0x7ff0727ac180)
    at js/src/ion/Ion.cpp:332
#6  0x000000000047b743 in MarkChildren (code=0x7ff072138330, trc=0x7ff0727ac180)
    at js/src/jsgcmark.cpp:883
#7  processMarkStackTop (budget=..., this=0x7ff0727ac180) at js/src/jsgcmark.cpp:1099
#8  js::GCMarker::drainMarkStack (this=0x7ff0727ac180, budget=...)
    at js/src/jsgcmark.cpp:1206
#9  0x00000000004707ec in MarkAndSweep (gckind=js::GC_NORMAL, cx=0x17b7ab0)
    at js/src/jsgc.cpp:3290
#10 GCCycle (cx=0x17b7ab0, full=<optimized out>, budget=0, gckind=js::GC_NORMAL)
    at js/src/jsgc.cpp:3653
#11 0x0000000000470d80 in Collect (cx=0x17b7ab0, full=<optimized out>, budget=0, gckind=js::GC_NORMAL, reason=<optimized out>)
    at js/src/jsgc.cpp:3749
#12 0x00000000005cf463 in GC (cx=0x17b7ab0, argc=<optimized out>, vp=0x7ff07236b0a0)
    at js/src/builtin/TestingFunctions.cpp:35
#13 0x00000000004a6ee7 in CallJSNative (args=..., native=<optimized out>, cx=0x17b7ab0) at ./jscntxtinlines.h:314
#14 js::InvokeKernel (cx=0x17b7ab0, args=..., construct=js::NO_CONSTRUCT) 
    at js/src/jsinterp.cpp:527
#15 0x0000000000499a93 in js::Interpret (cx=0x17b7ab0, entryFrame=0x7ff07236b030, interpMode=js::JSINTERP_NORMAL)
    at js/src/jsinterp.cpp:2725
#16 0x00000000004a69ca in js::RunScript (cx=0x17b7ab0, script=0x7ff0721072b8, fp=0x7ff07236b030)
    at js/src/jsinterp.cpp:483
#17 0x00000000004a7aa0 in ExecuteKernel (result=0x0, thisv=..., scopeChain=<optimized out>, script=0x7ff0721072b8, cx=0x17b7ab0,
    type=<optimized out>, evalInFrame=<optimized out>) at js/src/jsinterp.cpp:681
#18 js::Execute (cx=0x17b7ab0, script=0x7ff0721072b8, scopeChainArg=<optimized out>, rval=0x0)
    at js/src/jsinterp.cpp:723 
#19 0x000000000041f8a7 in JS_ExecuteScript (cx=<optimized out>, obj=<optimized out>, script=<optimized out>, rval=<optimized out>)
    at js/src/jsapi.cpp:5237
#20 0x000000000040d90c in Process (cx=0x17b7ab0, obj=0x7ff072103060, filename=<optimized out>, forceTTY=<optimized out>)
    at js/src/shell/js.cpp:479
#21 0x0000000000410871 in ProcessArgs (op=0x7fff6c213060, obj=0x7ff072103060, cx=0x17b7ab0)
    at js/src/shell/js.cpp:4783
#22 Shell (cx=0x17b7ab0, op=0x7fff6c213060, envp=<optimized out>)
    at js/src/shell/js.cpp:4881
#23 0x0000000000403f99 in main (argc=<optimized out>, argv=<optimized out>, envp=0x7fff6c213258)
    at js/src/shell/js.cpp:5111
(Assignee)

Comment 1

5 years ago
This bug also appear with --ion-eager and with a debug build (cf 11 C++ assertions http://people.mozilla.org/~npierron/ion-eager-x64.html)

Thanks for opening the bug.
Assignee: general → nicolas.b.pierron
Blocks: 742136
Status: NEW → ASSIGNED
fwiw, it looks like there's a similar failure on ARM.
(Assignee)

Comment 3

5 years ago
Ok, I tracked the bug by using the following path:

With the following stack trace, frame 3, I can determine that there is a ImmGCPtr which is set to a data which does not represents a valid gc::Cell pointer.  At frame 3, I can look at the offset (307 on x64).

Stack trace:
#0  MapAllocToTraceKind (thingKind=Cannot access memory at address 0x18) at js/src/jsgc.h:1020
#1  GetGCThingTraceKind (thing=0x0) at js/src/jsgcinlines.h:63
#2  js::gc::MarkThingOrValueUnbarriered (trc=0x7ff0727ac180, word=0x7ff073e17547, name=0x773e68 "imm-gc-word")
    at js/src/jsgcmark.cpp:517
#3  0x00000000006dfaba in TraceDataRelocations (reader=...,
    buffer=0x7ff073e17508 "H\203\354(H\203\304(H\203\354(I\273P\340}r\360\177", trc=0x7ff0727ac180)

Then I set a breakpoint on ImmGCPtr and use the following gdb command to locate the instruction which is causing this problem:

b js::ion::ImmGCPtr
commands
frame 1
print masm.currentOffset()
end

This is printing the current instruction offset, even if this does not account for the relocation which happens at the end of the code generation, this is still useful for small script.

In this my minimal test case --ion --ion-eager -n:

gczeal(2, 1);
var a = {};

I can identify the issue as being an unknowned TypeObject pointer (set to NULL) to which we add a relocation offset.

The best fix for this bug is to avoid adding the relocation offset for NULL pointers.
Good catch - yeah, we should disallow that.
(Assignee)

Comment 5

5 years ago
Created attachment 612410 [details] [diff] [review]
Do not mark NULL pointers as relocatable.

Add isMarkable boolean to check every GCPtr or Value before registering them as markable gc::Cell.

Kannan: Could you check with kraken to see if this patch correspond to the same bug or not.
Attachment #612410 - Flags: review?(dvander)
Comment on attachment 612410 [details] [diff] [review]
Do not mark NULL pointers as relocatable.

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

I'm not a big fan of the implicit coercion from Cell* to bool in this patch. Does writeRelocation() always get passed currentOffset/nextOffset? If so, maybe we should remove the |offs| parameter, and pass in the void* instead (that is, if the goal is to avoid NULL checks at every call site).
(Assignee)

Comment 7

5 years ago
(In reply to David Anderson [:dvander] from comment #6)
> Comment on attachment 612410 [details] [diff] [review]
> Do not mark NULL pointers as relocatable.
> 
> Review of attachment 612410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a big fan of the implicit coercion from Cell* to bool in this patch.
> Does writeRelocation() always get passed currentOffset/nextOffset? If so,
> maybe we should remove the |offs| parameter, and pass in the void* instead
> (that is, if the goal is to avoid NULL checks at every call site).

I used the bool to factor with Value marking which is checking for the isMarkable flag of the Value.  Another option would be to use !! in front of all pointers or have a summy function named  bool isMarkable(gc::Cell*).  The writeReocation is indeed always using the same second argument.
You could add a maybeCell to Value. That or we can skip NULL pointers in TraceRelocations. I try to avoid boolean parameters, here it just feels a little weird (it's basically a mark function with a mark bool that tells it whether or not to mark).
(Assignee)

Comment 9

5 years ago
(In reply to David Anderson [:dvander] from comment #8)
> You could add a maybeCell to Value. That or we can skip NULL pointers in
> TraceRelocations. I try to avoid boolean parameters, here it just feels a
> little weird (it's basically a mark function with a mark bool that tells it
> whether or not to mark).

Checking for NULL pointer in TraceRelocations implies that we register a NULL pointer which will be skipped every-time, so it's better to avoid adding it in the first place, which avoid later tracing inefficiencies.
(Assignee)

Comment 10

5 years ago
Created attachment 612704 [details] [diff] [review]
Do not mark NULL pointers as relocatable.

Change the signature of writeDataRelocation to get the data as argument.  The data
is then checked to determine if it is markable/relocatable.
Attachment #612410 - Attachment is obsolete: true
Attachment #612410 - Flags: review?(dvander)
Attachment #612704 - Flags: review?(dvander)
Attachment #612704 - Flags: review?(dvander) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/5b7e87e57f28
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 743082
You need to log in before you can comment on or make changes to this bug.