994957 - Assertion failure: offset < length(), at jsscript.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

enableSPSProfilingAssertions(true)
for (var j = 0; j < 9; ++j) {
    (function() {
        (function() {
            Proxy.createFunction((function() {
                return {
                    get: function() {},
                };
            })
            (), (function() {})).f
        })()
    })()
}

asserts js debug shell on m-c changeset 83ae54e18689 with --ion-eager --ion-parallel-compile=off at Assertion failure: offset < length(), at jsscript.h

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140318150004" and the hash "c0e333b0f7dc".
The "bad" changeset has the timestamp "20140318150802" and the hash "c8275c5686d5".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0e333b0f7dc&tochange=c8275c5686d5

Kannan, is bug 948229 a likely regressor?

Comment 1

[Kannan Vijayan [:djvj]]

Probably.  There is a known issue here, and it's affecting some live pages as well.  It's not straightforward to fix, so still thinking about it.  Lemme take this bug.

Comment 2

[Kannan Vijayan [:djvj]]

Found the issue.  IC code is pushing the PC associated with the inlined function, not the call-site of the inlined function in the top-level function.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-994957.patch

This patch fixes the issue, but at a cost which I'm not happy with.. however I don't know of a better way to go about this.

The problem is this: the profiler instrumentation in ICs assumes that the top pseudostack frame is for the innermost inlined script currently active.  It updates the pseudostack |pc| field with the |pc| corresponding to the IC within its inlined script.  This breaks when inline-frame-profiling is turned off, because the top pseudostack frame is for the outermost script that was compiled for the frame, and the pcs do not correspond.

The only real fix I can think of for this is for Ion ICs to carry around the top-level script and bytecode pointer that needs to be used when updating the profiler pseudostack.

This adds two machine words to each Ion IC, and forces the threading down of IonBuilder top-level-function-and-call-pc through the IC MIR instructions, into codegen and eventually the ICs themselves.

I must add: every time I fix a bug in the profiler these days it feels like I'm adding more cruft.

Comment 4

[Kannan Vijayan [:djvj]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=0fe57871d0be

Comment 5

[Jan de Mooij [:jandem]]

I agree it's a bit unfortunate, also from a memory usage POV.

If we remove the profileInlineFrames option, we can get the (outer) JSScript from GetTopIonJSScript (it gets it from the callee token). We're calling it already to get the IonScript.

We still need the pc though, if we don't want to use script->code... Only GetPropertyIC and SetPropertyIC use the MacroAssembler constructor that takes the script/pc. Could we only store the (SPS) pc for these caches?

Comment 6

[Kannan Vijayan [:djvj]]

For related purposes, I'm already planning on adding a small tree representation of the inlinings done when compiling a particular script.  This is to help construct jitcode => bytecode tables that will be able to retrieve the full series of [JSScript, jsbytecode] inlinings that are active at a particular point in the code.

When I land that, I can change this to use that structure instead of manually passing down jsbytecode pointers.

Good point about removing the words on the ICs except for GetProp and SetProp ICs.  I'll do that.  It does mean we'll have to be careful in the future if we add any stubs that emit vm-calls, since we'll have to go back and thread in the appropriate info to not introduce a bug into the profiler.

That's why I added the word to all ICs: the expectation that we may generate stubs-with-calls for any of them at some point, and we're leaving opportunity for a bug to silently creep in if we expect the writer of that stub to go back and ensure that |profilerLeavePc_| and |profilerLeaveScript_| fields are added and properly used during MacroAssembler construction.

I do want to rip out all of this code eventually.  When we get full native=>bytecode mapping tables, we should be able to eliminate this entire mess.

Comment 7

[Kannan Vijayan [:djvj]]

Attachment: add-inline-script-tree.patch

I'm taking a different tack to fixing this bug, anticipating future work we'll need to do in order to support native => bytecode mappings for the profiler.

To start, we have no easy way of "walking up" the chain of inlined functions during any part of the compilation.  Part of what's needed for fixing this bug is for certain IC stub compilers to easily get at the top-level function it was compiled for, and the pcOffset within the top-level function for the topmost inlined callee.

This patch adds InlineScriptTree, a small tree using a linked-sibling-lists for children.  All scripts which participate in a compile (including the top-level script), have an InlineScriptTree associated with them.  For every IonBuilder instance, the associated CompileInfo points to the InlineScriptTree entry corresponding to its script.

All InlineScriptTree entries have a caller pointer to the InlineScriptTree instance corresponding to their caller.  Using this, a single pointer to an InlineScriptTree can be used to derive the entire stack of inlines (and pc-offsets for the inlines) for a given inner script.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: add-site-tracking-to-mir.patch

This patch uses the InlineScriptTree introduced in the previous patch.

Currently, MBasicBlocks and MDefinitions track their current bytecode pointer, and don't directly track the inlining-stack above them.

This patch adds a new structure, BytecodeSite, that represents the pair of a pointer to an InlineScriptTree and a bytecode pointer.  A BytecodeSite is then able to represent the chain of (script, callPC) pairs that leads to the innermost script, as well as the bytecode within the innermost script.

MBasicBlocks and MDefinitions are modified to use BytecodeSites instead of pointers-to-bytecode to track their location.  This adds an extra 2 words to every MDefinition, and an extra word to every MBasicBlock.

Comment 9

[Kannan Vijayan [:djvj]]

Jan: about your comments earlier about only adding the |profilerLeavePc| and |profilerLeaveScript| to the stubs that require them.

I don't think this is feasible.  Profiler pseudo-stack updates will happen for every IonCache that generates a stub that does a callVM or a callWithABI, which is most of them.  Furthermore, if we leave this out for a particular IonCache, and later on someone adds a stub to this cache that does a callWithABI (or modifies an existing stub to do a callWithABI), then they will be implicitly introducing the bug back into the system.

I don't think we can get around having these two words added to every IonCache instance.  We can eventually remove them once we get rid of the pseudostack entirely, but until then it seems to me like this is the only option.

Thoughts?

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: add-profiler-info-to-ioncache.patch

This patch uses the BytecodeSite tracking added by the previous two patches, to propogate the appropriate profiler pseudo-stack update information to the relevant ICs.

try run with all 3 patches looks good, and the bug is reproducible on my linux box as well as fixed by the patch:

https://tbpl.mozilla.org/?tree=Try&rev=f0422170d0d0

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8407798 [details] [diff] [review]
add-inline-script-tree.patch

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

Sounds good.

::: js/src/jit/CompileInfo.h
@@ +49,5 @@
> +// current function being inlined).
> +//
> +// To support this, the top-level IonBuilder keeps a tree that records the
> +// inlinings done during compilation.
> +class InlineScriptTree {

Since this is tiny, seems like you could make this a TempObject and avoid the null-checks on allocation.

@@ +51,5 @@
> +// To support this, the top-level IonBuilder keeps a tree that records the
> +// inlinings done during compilation.
> +class InlineScriptTree {
> +    // InlineScriptTree for the caller
> +    InlineScriptTree *callerTree_;

The 'Tree' in this name is a bit confusing, since the pointer is to a node within the same tree.  How about just 'caller_'?

@@ +61,5 @@
> +    JSScript *script_;
> +
> +    // Child entries (linked together by nextCallee pointer)
> +    InlineScriptTree *children_;
> +    InlineScriptTree *nextCallee_;

I can't find any use of these two fields in this or the other two patches.  Are you foreseeing a need to traverse the call tree from parent to children?

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8407809 [details] [diff] [review]
add-site-tracking-to-mir.patch

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

Makes sense.

::: js/src/jit/CompileInfo.h
@@ +108,5 @@
> +      : tree_(nullptr), pc_(nullptr)
> +    {}
> +
> +    BytecodeSite(const BytecodeSite &other)
> +      : tree_(other.tree_), pc_(other.pc_)

This should be automatically generated for you.

::: js/src/jit/MIRGraph.h
@@ +509,5 @@
>      Vector<MBasicBlock *, 1, IonAllocPolicy> immediatelyDominated_;
>      MBasicBlock *immediateDominator_;
>      size_t numDominated_;
>  
> +    BytecodeSite trackedSite_;

I'm a bit confused by the two BytecodeSites here.  If I understand what these mean, it seems like you could have:
  InlineScriptTree * const tree_;
  jsbytecode * const entryPC_;
  jsbytecode * currentPC_;
instead.

Comment 13

[Kannan Vijayan [:djvj]]

(In reply to Luke Wagner [:luke] from comment #11)
> ::: js/src/jit/CompileInfo.h
> @@ +49,5 @@
> > +// current function being inlined).
> > +//
> > +// To support this, the top-level IonBuilder keeps a tree that records the
> > +// inlinings done during compilation.
> > +class InlineScriptTree {
> 
> Since this is tiny, seems like you could make this a TempObject and avoid
> the null-checks on allocation.

This introduces an include loop (IonAllocPolicy.h => Ion.h => CompileInfo.h), and I'd rather
not move around a bunch of code as part of this patch since the number of places InlineScriptTree is allocated is rather small and unlikely to grow.

> The 'Tree' in this name is a bit confusing, since the pointer is to a node
> within the same tree.  How about just 'caller_'?

Done.

> I can't find any use of these two fields in this or the other two patches. 
> Are you foreseeing a need to traverse the call tree from parent to children?

Not strictly useful yet, but they will be necessary later.  When we generate the maps to bytecode, we'll need to have each script-specific region in the native code be able to describe the stack-of-inlinings above them.

To efficiently encode those, I'm planning on having a small array all scripts involved in a given compilation.. and have each script-specific region encode a series of varint indexes into the array.

Constructing that array will require traversing the InlineScriptTree from the top.

Comment 14

[Kannan Vijayan [:djvj]]

(In reply to Luke Wagner [:luke] from comment #12)
> I'm a bit confused by the two BytecodeSites here.  If I understand what
> these mean, it seems like you could have:
>   InlineScriptTree * const tree_;
>   jsbytecode * const entryPC_;
>   jsbytecode * currentPC_;
> instead.

Good point.  I'll change the |BytecodeSite site_| field back to |jsbytecode *pc_|, and have |BytecodeSite trackedSite_| keep the InlineScriptTree pointer.  I suppose that a single block cannot span more than one JSScript.  I'll add an assert to verify that.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #9)
> Jan: about your comments earlier about only adding the |profilerLeavePc| and
> |profilerLeaveScript| to the stubs that require them.
> 
> I don't think this is feasible.  Profiler pseudo-stack updates will happen
> for every IonCache that generates a stub that does a callVM or a
> callWithABI, which is most of them.  Furthermore, if we leave this out for a
> particular IonCache, and later on someone adds a stub to this cache that
> does a callWithABI (or modifies an existing stub to do a callWithABI), then
> they will be implicitly introducing the bug back into the system.

The only places that pass the script + pc to the MacroAssembler constructor are GetPropertyIC, SetPropertyIC and NameIC, so the others don't use them atm. I think either every IC should do that or we shouldn't store the profiler script/pc for those.

> I don't think we can get around having these two words added to every
> IonCache instance.  We can eventually remove them once we get rid of the
> pseudostack entirely, but until then it seems to me like this is the only
> option.

We don't need the script if we remove the profileInlineFrames option; the ICs already get the outermost JSScript to get the IonScript. We discussed removing the profileInlineFrames option for that SPS bailout bug, or do you think we should keep it?

Comment 16

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> The only places that pass the script + pc to the MacroAssembler constructor
> are GetPropertyIC, SetPropertyIC and NameIC, so the others don't use them
> atm. I think either every IC should do that or we shouldn't store the
> profiler script/pc for those.

I'm OK with every IC doing this.  The loss (an extra 2 words per IC chain) seems reasonable compared to the benefit of avoiding any future bugs with ICs using the PC from the wrong script.

Are you fine with that or do you really want to avoid those 2 words per chain?

 
> We don't need the script if we remove the profileInlineFrames option; the
> ICs already get the outermost JSScript to get the IonScript. We discussed
> removing the profileInlineFrames option for that SPS bailout bug, or do you
> think we should keep it?

Good point about the top-level script already being available.  I'll re-work the patch to use that.

Comment 17

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #15)
> We don't need the script if we remove the profileInlineFrames option; the
> ICs already get the outermost JSScript to get the IonScript. We discussed
> removing the profileInlineFrames option for that SPS bailout bug, or do you
> think we should keep it?

Forgot to answer the question.  I'm OK with removing the profileInlineFrames option.  We're on track for replacing it with a better solution that handles inlined frames anyway, and the option is disabled by default currently.

Removing the profileInlineFrames option won't help us here, though.  The behaviour we want to keep is one where inline frame profiling is disabled, and the code still needs to be fixed so that ICs don't use the wrong PC in that circumstance.

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Kannan Vijayan [:djvj] from comment #16)
> Are you fine with that or do you really want to avoid those 2 words per
> chain?

That's fine, and if we use the top level JSScript from GetTopIonJSScript it's only 1 word for the pc.

> Good point about the top-level script already being available.  I'll re-work
> the patch to use that.

Great, thanks.

(In reply to Kannan Vijayan [:djvj] from comment #17)
> Removing the profileInlineFrames option won't help us here, though.  The
> behaviour we want to keep is one where inline frame profiling is disabled,
> and the code still needs to be fixed so that ICs don't use the wrong PC in
> that circumstance.

Yeah it still has to be fixed, but with profileInlineFrames gone we can just grab the JSScript from GetTopIonJSScript + the extra pc we'll store in the cache and it should work. If profileInlineFrames == true it's just a bit more complicated because you need the inner JSScript and can no longer use GetTopIonJSScript.

Comment 19

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> Yeah it still has to be fixed, but with profileInlineFrames gone we can just
> grab the JSScript from GetTopIonJSScript + the extra pc we'll store in the
> cache and it should work. If profileInlineFrames == true it's just a bit
> more complicated because you need the inner JSScript and can no longer use
> GetTopIonJSScript.

This is a convincing argument.  I wrote up a patch to remove the profileInlineFrames option, going through try right now.  I'll set it up as a blocker for this and the other bug.

Comment 20

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision b7062df8c7c3).

Comment 21

[Christian Holler (:decoder)]

JSBugMon: --enable-threadsafe


enableSPSProfilingWithSlowAssertions();
for (var j = 0; j < 9; ++j) {
    (function() {
        (function() {
            Proxy.createFunction((function() {
                return {
                    get: function() {},
                };
            })
            (), (function() {})).f
        })()
    })()
}



Test probably broke due to djvj's changes to the SPSProfiling functions.

Comment 22

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 23

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-994957.patch

This is a rebased patch that only adds 1 word to Ion ICs: the profilerLeavePc_ bytecode pointer.  The top-level script to use for the pseud-stack update is obtained from the C stack.  The patch also changes all MacroAssembler instantiations by SequentialExecution ICs to use the correct script and pc.

This still builds off of the InlineScriptTree and BytecodeSite structures.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2d79e5e6e15c

Comment 24

[Kannan Vijayan [:djvj]]

Pushed the first two patches that Luke r+ed (patches were folded into one before pushing):

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dff0b9f8294

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dff0b9f8294

Comment 26

[Kannan Vijayan [:djvj]]

Still have a patch to land on this.  Forgot to put [leave open] on Whiteboard.

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8414685 [details] [diff] [review]
fix-bug-994957.patch

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

Looks good, sorry for the delay.

::: js/src/jit/IonCaches.cpp
@@ +1199,5 @@
>                                 HandlePropertyName name, void *returnAddr, bool *emitted)
>  {
>      JS_ASSERT(canAttachStub());
>      JS_ASSERT(!*emitted);
> +    JS_ASSERT(topScript->hasIonScript() && topScript->ionScript() == ion);

Nit: ionScript() will assert hasIonScript().

::: js/src/jit/MIR.h
@@ +386,5 @@
>      }
>  
> +    JSScript *profilerLeaveScript() const {
> +        if (js_JitOptions.profileInlineFrames)
> +            return trackedTree()->script();

This branch can be removed now.

@@ +394,5 @@
> +    jsbytecode *profilerLeavePc() const {
> +        // If we're profiling inline frames, use the pc directly.
> +        // If this is in a top-level function, use the pc directly.
> +        if (js_JitOptions.profileInlineFrames || trackedTree()->isOutermostCaller())
> +            return trackedPc();

Same for the profileInlineFrames check here.

Comment 28

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af1cebf017fc

Ok now we can close :)

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7d352709614b for introducing two hazards: https://tbpl.mozilla.org/php/getParsedLog.php?id=38894238&tree=Mozilla-Inbound

Comment 30

[Kannan Vijayan [:djvj]]

Attachment: fix-rooting-hazards.patch

Previous patch introduced rooting hazards.  This patch is a delta from the previous patch, which resolves the rooting hazards, and additionally converts all of the |attachSomeStub| signatures to use handles instead of pointers.

Hazard try run: https://tbpl.mozilla.org/?tree=Try&rev=7fbff11ad047
Full try run: https://tbpl.mozilla.org/?tree=Try&rev=a9f8284a3654

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 8416547 [details] [diff] [review]
fix-rooting-hazards.patch

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

::: js/src/jit/IonCaches.cpp
@@ +1196,5 @@
>      return true;
>  }
>  
>  bool
> +GetPropertyIC::tryAttachNative(JSContext *cx, HandleScript topScript, IonScript *ion,

Sorry I didn't notice this earlier, but maybe rename topScript to outerScript to make it clear it's not the innermost, inlined script?

Comment 32

[Kannan Vijayan [:djvj]]

Fixed, re-pushed as one big patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd37328de87

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bd37328de87