Closed Bug 238881 Opened 20 years ago Closed 20 years ago

Constant propagation for switch cases too aggressive

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.7final

People

(Reporter: brendan, Assigned: brendan)

References

()

Details

(Keywords: js1.5)

Attachments

(3 files)

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
Attached patch proposed fixSplinter Review
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
Status: NEW → ASSIGNED
Flags: blocking1.7+
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
Attachment #144890 - Flags: review?(shaver)
Just for easier reviewing, mainly of the zero-length JS_malloc avoidance, but
please review-stamp the proposed fix.

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

Aye, cap'n.
Attachment #144890 - Flags: review?(shaver) → review+
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
Attachment #144890 - Flags: approval1.7?
Comment on attachment 144890 [details] [diff] [review]
proposed fix

a=chofmann for 1.7
Attachment #144890 - Flags: approval1.7? → approval1.7+
Fix checked in.

/be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
thanks to be.
js1_5/Regress/regress-238881.js checked in.
Flags: testcase+
verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: