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

RESOLVED FIXED in mozilla32

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: djvj)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
mozilla32
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:testComment=21,origRev=d7c07694f339])

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
Posted file 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?
Flags: needinfo?(kvijayan)
Assignee

Comment 1

5 years ago
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.
Flags: needinfo?(kvijayan)
Assignee

Updated

5 years ago
Assignee: nobody → kvijayan
Assignee

Comment 2

5 years ago
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.
Assignee

Comment 3

5 years ago
Posted patch fix-bug-994957.patch (obsolete) — Splinter Review
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.
Attachment #8406244 - Flags: review?(jdemooij)
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?
Assignee

Comment 6

5 years ago
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.
Assignee

Comment 7

5 years ago
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.
Attachment #8406244 - Attachment is obsolete: true
Attachment #8406244 - Flags: review?(jdemooij)
Attachment #8407798 - Flags: review?(luke)
Assignee

Comment 8

5 years ago
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.
Attachment #8407809 - Flags: review?(luke)
Assignee

Comment 9

5 years ago
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?
Flags: needinfo?(jdemooij)
Assignee

Comment 10

5 years ago
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
Attachment #8408420 - Flags: review?(jdemooij)
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?
Attachment #8407798 - Flags: review?(luke) → review+
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.
Attachment #8407809 - Flags: review?(luke) → review+
Assignee

Comment 13

5 years ago
(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.
Assignee

Comment 14

5 years ago
(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.
(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?
Flags: needinfo?(jdemooij)
Assignee

Comment 16

5 years ago
(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.
Assignee

Comment 17

5 years ago
(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.
(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.
Assignee

Comment 19

5 years ago
(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.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision b7062df8c7c3).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
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.
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:update,testComment=21,origRev=d7c07694f339]
Whiteboard: [jsbugmon:update,testComment=21,origRev=d7c07694f339] → [jsbugmon:testComment=21,origRev=d7c07694f339]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Assignee

Comment 23

5 years ago
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
Attachment #8408420 - Attachment is obsolete: true
Attachment #8408420 - Flags: review?(jdemooij)
Attachment #8414685 - Flags: review?(jdemooij)
Assignee

Comment 24

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/9dff0b9f8294
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee

Comment 26

5 years ago
Still have a patch to land on this.  Forgot to put [leave open] on Whiteboard.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8414685 - Flags: review?(jdemooij) → review+
Assignee

Comment 30

5 years ago
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
Attachment #8416547 - Flags: review?(jdemooij)
Flags: needinfo?(kvijayan)
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?
Attachment #8416547 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/8bd37328de87
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Assignee

Updated

5 years ago
Duplicate of this bug: 992377
You need to log in before you can comment on or make changes to this bug.