sq / JSIL

CIL to Javascript Compiler
http://jsil.org/
Other
1.73k stars 242 forks source link

Labelgroups get stuck in an infinite loop from heavily nested swtich statements. #950

Open otac0n opened 8 years ago

otac0n commented 8 years ago

With compiler optimizations on, heavily nested switch statements can turn into a pretty tangled jumble of goto's and switch statements.

Imagine a large structure like this, containing all of the tokens of a grammar:

int result;
switch (input[index++])
{
    case 'f':
    case 'F':
        switch (input[index++])
        {
            case 'o':
            case 'O':
                switch (input[index++])
                {
                    case 'o':
                    case 'O':
                        switch (input[index++])
                        {
                            case 'b':
                            case 'B':
                                switch (input[index++])
                                {
                                    case 'a':
                                    case 'A':
                                        switch (input[index++])
                                        {
                                            case 'r':
                                            case 'R':
                                                result = FOOBAR;
                                                break;
                                        }
                                        break;
                                }
                                break;
                        }
                        break;
                }
                break;
        }
        break;
}
return result;

The resulting code has these break; statements, but they only break out of the labelgroup's switch statement, not it's while(true). They don't change the state, so it just loops until it hit's the end of the string and breaks internally.

otac0n commented 8 years ago
      case "S": 
      case "s": 

        var $label13 = 0;
      $labelgroup13: 
        while (true) {
          switch ($label13) {

...

            case 5: /* IL_XXXX */ 
              num = (index | 0);
              index = ((num + 1) | 0);
              c = (input[num]);
              if (((c.charCodeAt(0) | 0) !== (("E").charCodeAt(0) | 0)) && ((c.charCodeAt(0) | 0) !== (("e").charCodeAt(0) | 0))) {
                break;
              }

This is an example of the resulting code. You can see the break here which just jumps back to the same spot.

otac0n commented 8 years ago

If the resulting IL produces this pattern in ILSPY, I think it will always reproduce this error:

switch (...) {
    case ...:
        goto label;
        break;
    label:
        ...
}

That is, when the label is an immediate child of a switch statement. Hope this helps.

kg commented 8 years ago

Thanks, that makes sense.

otac0n commented 8 years ago

So, I was able to fix this locally, but I'm still in the process of getting approval to contribute it back.

Here's my approach:

  1. Create a JSAstVisitor in the pipeline that searches for all breaks and continues, checks the Stack to see if there is an interposing JSLabelGroupStatement, and then labels their parent switch/loop if necessary.
  2. In the JavascriptAstEmitter, modify the continue and break statements to make the same check, but then read the lablel from the parent switch and emit it as part of the instruction.
kg commented 8 years ago

The existing pass that messes with labels probably could just be tweaked to not strip labels in this case, and then astemitter just needs the changes you describe. Thanks for doing the work to come up with a fix, even if you can't contribute it back!

otac0n commented 8 years ago

If I may, I'd like to suggest a slightly larger change that I think will make the internals a bit cleaner:

For break and continue nodes, instead of int? TargetLoop, I would suggest string Target. A loop (or switch!) would just be given a label, like "$loop0", and would no longer need the Index property.

This is more in line with how the Javascript language works, and would make implementing this fix very easy.

kg commented 8 years ago

Yeah, I think you're right. I forget whether I had a good reason for numbering loops instead of assigning them names.