601395 - Crash [@ JSObject::isNative] or [@ BindLet]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

let(y = let(d = []) u, x

crashes js debug shell on TM changeset 0230a9e80c1f without -m or -j at JSObject::isNative and crashes js opt shell at BindLet

Seems to crash at null in opt shell..

Program received signal SIGSEGV, Segmentation fault.
0x080fe25f in BindLet(JSContext*, BindData*, JSAtom*, JSTreeContext*) ()
(gdb) x/i $eip
=> 0x80fe25f <_ZL7BindLetP9JSContextP8BindDataP6JSAtomP13JSTreeContext+159>:	mov    (%eax),%edx
(gdb) x/b $eax
0x0:	Cannot access memory at address 0x0

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54650:427282865362
user:        Bill McCloskey
date:        Wed Sep 29 13:21:36 2010 -0700
summary:     Bug 535912 - Eliminate blockChain from JSStackFrame (r=cdleary)

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

This patch should fix the problem. Here's a description of what happened.

Inside the parser, tc->blockChain and tc->topScopeStmt are basically two independent ways of keeping track of the current scope. Occasionally they diverge from each other, such as when parsing |e| inside |let x=e| (so that the definition of |x| can be hidden).

When I wrote the blockChain elimination patch, I didn't fully understand how this worked. In one case, inside js_PopStatement, I tried to update cg->blockChain based on tc->topScopeStmt. I now realize that this was wrong.

This patch adds an extra |parent| field so that now tc->blockChain can be updated independently in js_PopStatement, as it was before the elimination patch.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 480532 [details] [diff] [review]
fix

Makes sense to me -- my understanding is that we created |JSObjectBox| wrappers around our |JSObject *block| items in the previous patch, but we didn't add a corresponding |JSObjectBox| parent link like we had when we were dealing with |JSObject *block|s directly.

We don't want to use the |JSObject::parent| pointer to nefariously point at |JSObjectBox|es instead, so we simply add back the parent field that we lost in the transition.

It seems like when you enter multiple let-heads; i.e.

let(x=let(y=let(z=...

Your top statement pointer will get arbitrarily far away from the block statement pointer, but the bad behavior from the previous patch always made it the same as the top statement pointer on a pop.

Correct me if I'm saying something inaccurate!

r=me with the tests from the duplicates added as well (please make sure they fail without the patch and pass with it).

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

http://hg.mozilla.org/tracemonkey/rev/5642354319d6
http://hg.mozilla.org/tracemonkey/rev/0d2c82eaeab6

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5642354319d6

Comment 9

[Nicholas Nethercote [inactive]]

For future reference, tests for bugs that don't involve the JITs should probably go under tests/ instead of jit-tests/.

Comment 10

[Brendan Eich [:brendan]]

tests/js1_8_5/regress/regress-601395.js in this instance.

/be

Comment 11

[David Mandelin [:dmandelin]]

(In reply to comment #9)
> For future reference, tests for bugs that don't involve the JITs should
> probably go under tests/ instead of jit-tests/.

Actually, the name jit-tests is historical. I think it's a good place for any tests, as it's easier to create a test case for that test set than for tests/.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to comment #11)
> 
> Actually, the name jit-tests is historical. I think it's a good place for any
> tests, as it's easier to create a test case for that test set than for tests/.

jit-tests are run under all combinations of JITs.  jstests are only tested under the default -j -m -p configuration.  So putting a JIT-specific test in jstests means you're not testing it properly, and putting a non-JIT-related test in jit-tests means that you're running it eight times for no extra benefit.

Comment 13

[David Mandelin [:dmandelin]]

(In reply to comment #12)
> (In reply to comment #11)
> > 
> > Actually, the name jit-tests is historical. I think it's a good place for any
> > tests, as it's easier to create a test case for that test set than for tests/.
> 
> jit-tests are run under all combinations of JITs.  jstests are only tested
> under the default -j -m -p configuration.  So putting a JIT-specific test in
> jstests means you're not testing it properly, and putting a non-JIT-related
> test in jit-tests means that you're running it eight times for no extra
> benefit.

I would consider that a bug in the test harnesses. We should create a way for non-jit-related tests to be run only once using the jit-tests harness. It's just way easier to create a test for jit-tests.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> 
> I would consider that a bug in the test harnesses.

My understanding is that the original intent of jit-tests was for JIT-related tests.  (It was trace-tests originally, things were simpler then.)  So if there's a bug in the harnesses it's because you're repurposing jit-tests.

Anyway, how can we make it easier to add a jstest?

Comment 15

[Brendan Eich [:brendan]]

I don't think it's that hard to add a jstest. The manifest update and the reportCompare call at the bottom are the overhead. The latter actually fixes a but in jit-tests, IIRC -- knowing that you reached the end without a silent and wrong exit(0). Right?

/be

Comment 16

[David Mandelin [:dmandelin]]

(In reply to comment #15)
> I don't think it's that hard to add a jstest. The manifest update and the
> reportCompare call at the bottom are the overhead. The latter actually fixes a
> but in jit-tests, IIRC -- knowing that you reached the end without a silent and
> wrong exit(0). Right?

It's not that hard, but adding a jit-test is as easy as can be: in the basic use case, just add a JS file, and an error means a failure. I think having that ease was really helpful in promoting people writing tests, because there was no entry barrier at all. IMO boilerplate is not needed for tests and so we should not require it.

Looking for silent fails can be fixed, and in a better way than tests/, avoiding the possibility of forgetting to add epilogue boilerplate to a test. We could append a final print and check for that output. 

The way I see it, we currently have 2 test formats, and it's accidental that one of them is considered to be for "jit tests". And the jit-test/ harness is not only easier to write tests for, but is more powerful: you can check for specific errors, run valgrind, control which jit flags are used, etc. The tests/ harness can't do that stuff and it would be harder to make it do that, because its manifest file format is shared with reftests.

Comment 17

[Paul Biggar]

(In reply to comment #13)
> > So putting a JIT-specific test in
> > jstests means you're not testing it properly, and putting a non-JIT-related
> > test in jit-tests means that you're running it eight times for no extra
> > benefit.


> I would consider that a bug in the test harnesses. We should create a way for
> non-jit-related tests to be run only once using the jit-tests harness. It's
> just way easier to create a test for jit-tests.

I don't believe we should be manually distinguishing between JIT-tests and non-JIT-tests. This is a job for gcov and other code coverage tools. That's bug 449532, which I'm currently trying to revive.

That said, I like the suggestion to add a sentinel at the end to ensure the test completed.

Comment 18

[Brendan Eich [:brendan]]

Ok, let's catch the silent exit(0) problem and we can unify. I'm all in favor of an implciit manifest based on directory traversal (however, ISTM we will need an exception mechanism for shell-only tests).

/be

Comment 19

[Nicholas Nethercote [inactive]]

The |jit-test| line seems to be a reasonable exception mechanism.

(In reply to comment #17)
> 
> I don't believe we should be manually distinguishing between JIT-tests and
> non-JIT-tests. This is a job for gcov and other code coverage tools.

Sounds difficult to get right.

Comment 20

[Paul Biggar]

> (In reply to comment #17)
> > 
> > I don't believe we should be manually distinguishing between JIT-tests and
> > non-JIT-tests. This is a job for gcov and other code coverage tools.
> 
> Sounds difficult to get right.

Automatically at run-time, yes, would be hard. I think the steps to automatically generate a manifest of slow/fast that made an appropriate coverage tradeoff would be straightforward.

However, the point I really want to make is that we're flying in the dark here. We shouldn't be reducing the permutations of tests we run until we get a bit more light on the coverage situation.


> putting a non-JIT-related
> test in jit-tests means that you're running it eight times for no extra
> benefit.

I disagree strongly. We should be taking our jstests and running them with all jit permutations, to see if it flushes out any bugs, or prevents any being added.

Comment 21

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

(In reply to comment #20)
> > putting a non-JIT-related
> > test in jit-tests means that you're running it eight times for no extra
> > benefit.
> 
> I disagree strongly. We should be taking our jstests and running them with all
> jit permutations, to see if it flushes out any bugs, or prevents any being
> added.

I'm with Paul here. Sometimes different parameters for the same testcase can expose issues, though the probability here may be small and largely random, I wouldn't count against it.

But perhaps another place for discussion, rather than in this bug?

Comment 22

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug601395.js.