238881 - Constant propagation for switch cases too aggressive

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

This is a regression from 1.6 that should be fixed (before 1.7a, constants
weren't propagated at all in JS and the test URL worked as expected, alerting 0
not 1).  Patch next.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

js_LookupCompileTimeConstant was overloading its return value in case of error,
leaving the error-as-exception pending even though EmitSwitch returned true.

The main fix is to stop looking at the compile-time scope chain (static link)
as soon as we find a property named atom.  If that property is directly in the
variable object, is readonly and permanent, and is not shadowed by a nearer
property, then we can propagate the constant.  Otherwise we bail with
JSVAL_VOID in *vp, which tells the caller to fall through into the case
true:/case false: handler.

There's an unrelated fix to avoid asserting on a zero-length JS_malloc call
when allocating the tableswitch compile-time jump target table.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: diff -w version of jsemit.c part of patch

Just for easier reviewing, mainly of the zero-length JS_malloc avoidance, but
please review-stamp the proposed fix.

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 144890 [details] [diff] [review]
proposed fix

Aye, cap'n.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 144890 [details] [diff] [review]
proposed fix

The old code in js_LookupCompileTimeConstant was embarrassing -- it failed to
drop the property looked up, in addition to overzealously following the
compile-time scope chain past shadowing args/vars in functions that should have
hidden the outer-scope const.  I am filled with shame.

Anyway, this is good for 1.7final.

/be

Comment 5

[chris hofmann]

Comment on attachment 144890 [details] [diff] [review]
proposed fix

a=chofmann for 1.7

Comment 6

[Brendan Eich [:brendan]]

Fix checked in.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-238881.js

thanks to be.

Comment 8

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-238881.js checked in.

Comment 9

[Bob Clary [:bc] (inactive)]

verified fixed.