Author: dda
Date: 2007-09-27 15:53:32 -0700 (Thu, 27 Sep 2007)
New Revision: 6652
Modified:
openlaszlo/trunk/WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java
Log:
Change 20070921-dda-l by [EMAIL PROTECTED] on 2007-09-21 17:52:38 EDT
in /Users/dda/laszlo/src/svn/openlaszlo/trunk
for http://svn.openlaszlo.org/openlaszlo/trunk
Summary: Fix script compiler switch stack mixup in default label
New Features:
Bugs Fixed: LPP-3023
Technical Reviewer: ptw
QA Reviewer: henry
Doc Reviewer: (pending)
Documentation:
Release Notes:
Details:
The problem is in the script compiler, specifically for the SW7/8 code
generator.
When creating a jump table (a series of comparisons to jump to the case
labels),
each jump action leaves a value at the top of the stack that must be popped
as the
first action of the case label. However, default was handled differently.
The extra value was popped first and the jump to the default label is made,
so the default label did not pop as its first action.
Here's the problem. When case labels are joined with the default label like
this:
case 10:
case 11:
default:
// do something...
the code generator attempts to share the code, but the stack levels are
mismatched.
One solution is to force the default label to be the last and insert the
extra pop
for the case labels only:
case 10:
case 11:
pop stack
// fall through
default:
I opted for a simpler solution, to guarantee that stack levels are always
the same.
During the jump table, we avoid popping the stack before the default jump.
We must
take care that if there is no default label, the stack is popped before the
jump table
completes and we jump to the end of the switch code block.
With this approach, we now always emit a pop as the first action of a default
label, like every other case label. This allows case labels to be easily
joined whether or not they represent the default case.
I don't believe there is any performance impact: we are doing the same
number of pops
at runtime (well, one extra that is needed to fix the buggy situation), and
the
overall code size should be the same.
Tests:
Ran test listed in LPP-3023 and made sure it is now correct, both with debug
and no debug.
Enhanced the app to test a number of other corner cases for default labels
and verified
the output, again with/without debug. App included here:
<canvas>
<class name="switcher" extends="text">
<method name="testSwitch" args="val">
var result = "none";
switch (val) {
case 1:
result = "case 1";
break;
case 2:
case 3:
// if you uncomment the true below both 2 and 3 work
// true;
default:
// if you remove the default: above 2 and 3 also work
result = "default";
break;
}
this.setText(this.name + ": " + result);
</method>
<method name="testSwitchDefaultBreak" args="val">
var result = "none";
switch (val) {
case 1:
result = "case 1";
break;
case 2:
case 3:
// if you uncomment the true below both 2 and 3 work
// true;
default:
break;
}
this.setText(this.name + ": " + result);
</method>
<method name="testSwitchDefaultNoBreak" args="val">
var result = "none";
switch (val) {
case 1:
result = "case 1";
break;
case 2:
case 3:
// if you uncomment the true below both 2 and 3 work
// true;
default:
}
this.setText(this.name + ": " + result);
</method>
<method name="testSwitchNoDefault" args="val">
var result = "none";
switch (val) {
case 1:
result = "case 1";
break;
case 2:
case 3:
// if you uncomment the true below both 2 and 3 work
// true;
}
this.setText(this.name + ": " + result);
</method>
</class>
<simplelayout />
<switcher name="switch1" />
<switcher name="switch2" />
<switcher name="switch3" />
<switcher name="switch4" />
<switcher name="switch11" />
<switcher name="switch21" />
<switcher name="switch31" />
<switcher name="switch41" />
<switcher name="switch12" />
<switcher name="switch22" />
<switcher name="switch32" />
<switcher name="switch42" />
<switcher name="switch13" />
<switcher name="switch23" />
<switcher name="switch33" />
<switcher name="switch43" />
<text name="switch5">
<method name="otherSwitch" args="val">
var result = "none";
switch (val) {
case 1:
result = "case 1";
break;
case 2:
case 3:
// if you uncomment the true below both 2 and 3 work
// true;
default:
// if you remove the default: above 2 and 3 also work
result = "default";
break;
}
this.setText(this.name + ": " + result);
</method>
</text>
<text name="switch6">
<method name="nonSwitch" args="val">
var result = "none";
if (val == 1) {
result = "case 1";
} else {
result = "default";
}
this.setText(this.name + ": " + result);
</method>
</text>
<text name="switch7">
</text>
<script>
// the first case works
switch1.testSwitch(1);
// the default case works;
switch2.testSwitch(5);
// using 2 or 3 appears to work but breaks all method calls
switch3.testSwitch(2);
// none of these locally defined calls work
switch4.testSwitch(1);
switch11.testSwitchDefaultBreak(1);
switch21.testSwitchDefaultBreak(5);
switch31.testSwitchDefaultBreak(2);
switch41.testSwitchDefaultBreak(1);
switch12.testSwitchDefaultNoBreak(1);
switch22.testSwitchDefaultNoBreak(5);
switch32.testSwitchDefaultNoBreak(2);
switch42.testSwitchDefaultNoBreak(1);
switch13.testSwitchNoDefault(1);
switch23.testSwitchNoDefault(5);
switch33.testSwitchNoDefault(2);
switch43.testSwitchNoDefault(1);
switch5.otherSwitch(1);
switch6.nonSwitch(1);
switch7.setText(this.name + ": " + "test");
</script>
</canvas>
Modified:
openlaszlo/trunk/WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java
===================================================================
---
openlaszlo/trunk/WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java
2007-09-27 22:52:59 UTC (rev 6651)
+++
openlaszlo/trunk/WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java
2007-09-27 22:53:32 UTC (rev 6652)
@@ -839,16 +839,20 @@
collector.emit(Instructions.EQUALS);
collector.emit(Instructions.BranchIfTrue.make(l));
}
- collector.emit(Instructions.POP);
- collector.emit(Instructions.BRANCH.make((defaultLabel != null) ?
defaultLabel : finalLabel));
+ if (defaultLabel != null) {
+ collector.emit(Instructions.BRANCH.make(defaultLabel));
+ }
+ else {
+ collector.emit(Instructions.POP);
+ collector.emit(Instructions.BRANCH.make(finalLabel));
+ }
String nextLabel = null;
for (Iterator i = targets.keySet().iterator(); i.hasNext(); ) {
String l = (String)i.next();
SimpleNode stmt = (SimpleNode)targets.get(l);
collector.emit(Instructions.LABEL.make(l));
- if (! l.equals(defaultLabel)) {
- collector.emit(Instructions.POP);
- } else {
+ collector.emit(Instructions.POP);
+ if (l.equals(defaultLabel)) {
defaultLabel = null;
}
if (nextLabel != null) {
@@ -862,14 +866,15 @@
collector.emit(Instructions.BRANCH.make(nextLabel));
}
}
+ // Handle empty default as last clause
+ if (defaultLabel != null) {
+ collector.emit(Instructions.LABEL.make(defaultLabel));
+ collector.emit(Instructions.POP);
+ }
// Handle fall-though in last clause
if (nextLabel != null) {
collector.emit(Instructions.LABEL.make(nextLabel));
}
- // Handle empty default as last clause
- if (defaultLabel != null) {
- collector.emit(Instructions.LABEL.make(defaultLabel));
- }
collector.emit(Instructions.LABEL.make(finalLabel));
}
finally {
_______________________________________________
Laszlo-checkins mailing list
[email protected]
http://www.openlaszlo.org/mailman/listinfo/laszlo-checkins
