approved On 9/25/07, Donald Anderson <[EMAIL PROTECTED]> wrote: > Henry - I just added you as a QA reviewer, thanks. > Tucker, you've already approved this one. > > > 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> > > Files: > M WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java > > Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20070921-dda- > l.tar > > > > -- > > Don Anderson > Java/C/C++, Berkeley DB, systems consultant > > Voice: 617-547-7881 > Email: [EMAIL PROTECTED] > WWW: http://www.ddanderson.com > >
-- Henry Minsky Software Architect [EMAIL PROTECTED]
