Closed Bug 1028529 Opened 10 years ago Closed 10 years ago

IONFILTER: Assertion failure: !this->graph at js/src/jit/IonSpewer.cpp:146

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: guillaume.turri, Assigned: guillaume.turri)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140608211622

Steps to reproduce:

* built SpiderMonkey on commit dd65a59b3d09f437a7fa0f4ff8834b534e26c184
* from js/src, launched

    IONFILTER=jit-test/tests/ion/dce-with-rinstructions.js:16 IONFLAGS=logs ./build_DBG.OBJ/dist/bin/js jit-test/tests/ion/dce-with-rinstructions.js 

nb: adding the flag --ion-offthread-compile=off , it doesn't crash


Actual results:

Assertion failure: !this->graph, at /documents/mozilla/gecko-dev/js/src/jit/IonSpewer.cpp:146
Segmentation fault (core dumped)



Expected results:

no core dump
Component: Canvas: 2D → JavaScript Engine: JIT
OS: Linux → All
Hardware: x86_64 → All
Assignee: nobody → guillaume.turri
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug1028529.patch (obsolete) — Splinter Review
Here is a first draft to fix it. With this patch, running

    IONFILTER=jit-test/tests/ion/dce-with-rinstructions.js:16 IONFLAGS=logs ./build_DBG.OBJ/dist/bin/js jit-test/tests/ion/dce-with-rinstructions.js

doesn't crash anymore.

My concerns are in particular:

* Is it actually the wanted behavior?
In particular, in the case where `OffThreadCompilationAvailable(cx)` is true, it seems we can't log anyway.  Hence, wouldn't it make more sense e.g. to call `IonSpewNewFunction` after this `if` block?

* What would be the best way to add a non regression unit test?
I feel like I could extract the end of `IonCompile` in another function. I would then just have to setup the test so that `OffThreadCompilationAvailable(cx)` returns true, and call this new function twice. Without the patch it should crash. With it, it shouldn't anymore.

Do you concur?
Attachment #8445116 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8445116 [details] [diff] [review]
bug1028529.patch

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

(In reply to guillaume.turri from comment #1)
> * Is it actually the wanted behavior?
> In particular, in the case where `OffThreadCompilationAvailable(cx)` is
> true, it seems we can't log anyway.  Hence, wouldn't it make more sense e.g.
> to call `IonSpewNewFunction` after this `if` block?

I think that on the contrary it might be more helpful to have it this way.  In the current patch, modulo the warning, you should be able to print the MIR Graph returned by IonBuilder for every script which is compiled out-side the main thread, even if this is not ideal this might be a good way to get an idea of the code that we produce during off-thread compilation and if it differ much.

One example where it might be interesting to test with that is on noisy octane benchmarks, where the noise is not coming from the GC.

> * What would be the best way to add a non regression unit test?

Sometimes, we just do not bother testing such things, as they are only used in debug builds, and only serve developer purposes.
Also making test might be more painful to maintain than fixing the feature, so don't bother for this one ;)

::: js/src/jit/Ion.cpp
@@ +1905,5 @@
>          JS_ASSERT(executionMode == SequentialExecution);
>          builderScript->ionScript()->setRecompiling();
>      }
>  
> +    IonSpewRaii ionSpewRaii(graph, builderScript);

nit: s/IonSpewRaii/IonSpewFunction/

::: js/src/jit/IonSpewRaii.cpp
@@ +2,5 @@
> +
> +#include "jit/IonSpewer.h"
> +#include "jit/MIRGraph.h"
> +
> +IonSpewRaii::IonSpewRaii(MIRGraph *graph, JS::HandleScript function)

No need to create a new header, just add it to IonSpewer.{h,cpp}

::: js/src/jit/IonSpewRaii.h
@@ +5,5 @@
> +namespace jit {
> +
> +class MIRGraph;
> +
> +class IonSpewRaii 

style-nit: remove trailing whitespace.
Attachment #8445116 - Flags: feedback?(nicolas.b.pierron) → feedback+
Attached patch bug1028529_2.patch (obsolete) — Splinter Review
Attachment #8445116 - Attachment is obsolete: true
Attachment #8445153 - Flags: review?(nicolas.b.pierron)
Attachment #8445153 - Attachment is obsolete: true
Attachment #8445153 - Flags: review?(nicolas.b.pierron)
Attachment #8445161 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8445161 [details] [diff] [review]
bug1028529_3.patch

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

This is great :)
Attachment #8445161 - Flags: review?(nicolas.b.pierron) → review+
Sending the modification to the Try server, only checking for compilation (dbg/opt), as this patch is unlikely to modify the way we execute JavaScript.

https://tbpl.mozilla.org/?tree=Try&rev=8e9afc52de7e
I've just landed this patch ;)
Do you already found a bug where you might want to continue?

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fceaf8f7904
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> Do you already found a bug where you might want to continue?


I had a quick look at https://bugzilla.mozilla.org/show_bug.cgi?id=1019843 and started to quickly look at the related code.

However, can't guarantee I'll be able to find the time to produce a patch any time soon.
(In reply to guillaume.turri from comment #8)
> (In reply to Nicolas B. Pierron [:nbp] from comment #7)
> > Do you already found a bug where you might want to continue?
> 
> 
> I had a quick look at https://bugzilla.mozilla.org/show_bug.cgi?id=1019843
> and started to quickly look at the related code.

Cool :)

> However, can't guarantee I'll be able to find the time to produce a patch
> any time soon.

Feel free to comment on the bug when you are ready to start working on it ;)
https://hg.mozilla.org/mozilla-central/rev/1fceaf8f7904
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: