Constant propagation for switch cases too aggressive

VERIFIED FIXED in mozilla1.7final

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
14 years ago
12 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.7final
js1.5
Points:
---
Bug Flags:
blocking1.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

14 years ago
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

14 years ago
Created attachment 144890 [details] [diff] [review]
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
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Flags: blocking1.7+
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
(Assignee)

Updated

14 years ago
Attachment #144890 - Flags: review?(shaver)
(Assignee)

Comment 2

14 years ago
Created attachment 144891 [details] [diff] [review]
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 on attachment 144890 [details] [diff] [review]
proposed fix

Aye, cap'n.
Attachment #144890 - Flags: review?(shaver) → review+
(Assignee)

Comment 4

14 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

14 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

14 years ago
Fix checked in.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 7

13 years ago
Created attachment 174971 [details]
js1_5/Regress/regress-238881.js

thanks to be.

Comment 8

13 years ago
js1_5/Regress/regress-238881.js checked in.

Updated

13 years ago
Flags: testcase+

Comment 9

12 years ago
verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.