[Laszlo-dev] For Review: Change 20070921-dda-l Summary: Fix script compiler switch stack mixup in default label

[Donald Anderson]

Change 20070921-dda-l by dda at freddie.local 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: (pending)
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:  dda at ddanderson.com
WWW:    http://www.ddanderson.com