1146836 - emitSwitch duplicate case detection is buggy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

For bug 1143704 I was looking at emitSwitch and noticed a minor, ancient bug. To detect duplicate cases, a stack-allocated bitmap is used if the case values < sizeof(size_t) * 256. Else, we malloc some memory and use that.

However, when we switch to malloc storage, we don't copy the values in our stack-allocated bitmap to our heap-allocated one, so we can fail to detect duplicates. The testcase below fails on 32-bit and 64-bit:

$ js test.js
test.js:9:4 Error: Assertion failed: got -1, expected 8005

This is obviously a silly case (no pun intended), but we should rewrite this code to use a Vector instead of this manual memory management to avoid these problems.

function f() {
    var s = "switch (x) {";
    for (var i=8000; i<16400; i++) {
	s += "case " + i + ": return " + i + "; break;";
    }
    s += "case 8005: return -1; break;";
    s += "}";
    var g = Function("x", s);
    assertEq(g(8005), 8005);
}
f();

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Random cleanup

Move variable declarations to the point where they are first used, use helpful names instead of pn2/pn3/pn4 etc.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use Vector; fix the bug

Replace both malloc calls in emitSwitch with a Vector.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8586021 [details] [diff] [review]
Part 1 - Random cleanup

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

Much nicer.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2602,5 @@
>   */
>  MOZ_NEVER_INLINE bool
>  BytecodeEmitter::emitSwitch(ParseNode* pn)
>  {
> +    ParseNode* cases = pn->pn_right;

But how will I know it's fast if it's not named 'pn2'?

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8586023 [details] [diff] [review]
Part 2 - Use Vector; fix the bug

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

Nice find and cleanup.

Comment 5

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1ebf0ad20b
https://hg.mozilla.org/integration/mozilla-inbound/rev/98ea146e6f51

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/fe1ebf0ad20b
https://hg.mozilla.org/mozilla-central/rev/98ea146e6f51