Closed
Bug 238881
Opened 20 years ago
Closed 20 years ago
Constant propagation for switch cases too aggressive
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.7final
People
(Reporter: brendan, Assigned: brendan)
References
()
Details
(Keywords: js1.5)
Attachments
(3 files)
9.58 KB,
patch
|
shaver
:
review+
chofmann
:
approval1.7+
|
Details | Diff | Splinter Review |
7.24 KB,
patch
|
Details | Diff | Splinter Review | |
2.22 KB,
text/plain
|
Details |
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
Assignee | ||
Comment 1•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Flags: blocking1.7+
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
Assignee | ||
Updated•20 years ago
|
Attachment #144890 -
Flags: review?(shaver)
Assignee | ||
Comment 2•20 years ago
|
||
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+
Assignee | ||
Comment 4•20 years ago
|
||
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 5•20 years ago
|
||
Comment on attachment 144890 [details] [diff] [review] proposed fix a=chofmann for 1.7
Attachment #144890 -
Flags: approval1.7? → approval1.7+
Assignee | ||
Comment 6•20 years ago
|
||
Fix checked in. /be
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 7•19 years ago
|
||
thanks to be.
Comment 8•19 years ago
|
||
js1_5/Regress/regress-238881.js checked in.
Updated•19 years ago
|
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•