Last Comment Bug 742519 - IonMonkey: Build of revision ec53c5d4c3dd crashes on Kraken/imaging-desaturate
: IonMonkey: Build of revision ec53c5d4c3dd crashes on Kraken/imaging-desaturate
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 743082 (view as bug list)
Depends on:
Blocks: IonEager
  Show dependency treegraph
 
Reported: 2012-04-04 13:40 PDT by Kannan Vijayan [:djvj]
Modified: 2012-04-17 13:09 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Do not mark NULL pointers as relocatable. (9.01 KB, patch)
2012-04-04 17:20 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Do not mark NULL pointers as relocatable. (9.06 KB, patch)
2012-04-05 15:17 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Kannan Vijayan [:djvj] 2012-04-04 13:40:11 PDT
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
Comment 1 Nicolas B. Pierron [:nbp] 2012-04-04 14:12:48 PDT
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.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-04-04 14:25:30 PDT
fwiw, it looks like there's a similar failure on ARM.
Comment 3 Nicolas B. Pierron [:nbp] 2012-04-04 16:20:15 PDT
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.
Comment 4 David Anderson [:dvander] 2012-04-04 16:33:01 PDT
Good catch - yeah, we should disallow that.
Comment 5 Nicolas B. Pierron [:nbp] 2012-04-04 17:20:48 PDT
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.
Comment 6 David Anderson [:dvander] 2012-04-04 18:02:34 PDT
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).
Comment 7 Nicolas B. Pierron [:nbp] 2012-04-05 12:19:37 PDT
(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.
Comment 8 David Anderson [:dvander] 2012-04-05 14:30:07 PDT
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).
Comment 9 Nicolas B. Pierron [:nbp] 2012-04-05 14:38:27 PDT
(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.
Comment 10 Nicolas B. Pierron [:nbp] 2012-04-05 15:17:54 PDT
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.
Comment 11 Nicolas B. Pierron [:nbp] 2012-04-05 17:30:19 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/5b7e87e57f28
Comment 12 Christian Holler (:decoder) 2012-04-17 13:09:44 PDT
*** Bug 743082 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.