Bug 324708 (MONO82034) - [Flow Analysis] goto does not call the finally block to close the iterator when leaving a foreach loop
Summary: [Flow Analysis] goto does not call the finally block to close the iterator wh...
Status: RESOLVED FIXED
Alias: MONO82034
Product: Mono: Compilers
Classification: Mono
Component: C# (show other bugs)
Version: 1.2.6
Hardware: Other All
: P3 - Medium : Normal
Target Milestone: ---
Assignee: Raja Harinath
QA Contact: Mono Bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-07 20:21 UTC by Jeff Thompson
Modified: 2008-04-05 20:19 UTC (History)
0 users

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---
jeff: CCB_Review?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Wiest 2007-09-15 20:43:23 UTC


---- Reported by jeff@thefirst.org 2007-07-07 13:21:37 MST ----

Please fill in this template when reporting a bug, unless you know what you
are doing.
Description of Problem:
Usually, in an iterator, we can put yield inside a try/finally block and
when control leaves the foreach loop for the iterator (because of "break"
or an exception) it will call the iterator's finally block. But if we leave
the foreach loop using goto, it doesn't do this. But we use "goto" to
transfer control out of one or more nested loops, and it should behave the
same as "break".

Steps to reproduce the problem:
1. Compile and run this code:
using System.Collections.Generic;

class TestGoto
{
    static int x = 0;

    static void Main(string[] args)
    {
        foreach (bool b in test())
        {
        }
    }

    static IEnumerable<bool> setX()
    {
        x = 1;
        try
        {
            yield return true;
        }
        finally
        {
            x = 0;
        }
    }

    static IEnumerable<bool> test()
    {
        foreach (bool b in setX())
        {
            yield return true;
            // Change "goto label" to "break" to show the correct result.
            goto label;
        }
    label:
        if (x == 0)
            System.Console.WriteLine("Success: finally was called");
        else
            System.Console.WriteLine("Error: finally was not called");
    }
}


Actual Results:
prints "Error: finally was not called" because the setX() iterator is not
closed when leaving the foreach loop.

Expected Results:
If you change "goto label" to "break", then the iterator is closed properly
and it prints "Success: finally was called". But a "goto" should act like
"break" and also close the iterator.

How often does this happen? 
Every time

Additional Information:



---- Additional Comments From jeff@thefirst.org 2007-07-13 00:11:08 MST ----

Microsoft has confirmed the same bug in their C# compiler and will 
fix it in the next release:
https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx
?FeedbackID=286401


Unknown bug field "cf_op_sys_details" encountered while moving bug
   <cf_op_sys_details>XP</cf_op_sys_details>
Unknown bug field "cf_version_details" encountered while moving bug
   <cf_version_details>1.2.4</cf_version_details>
Unknown operating system unknown. Setting to default OS "Other".

Comment 2 Jeff Thompson 2008-02-04 20:31:39 UTC
This same bug is confirmed in Microsoft C# 2008.
Has anyone reviewed this bug for Mono?
Comment 3 Jeff Thompson 2008-02-04 20:32:59 UTC
That should have said "This same bug is confirmed fixed in Microsoft C# 2008"
Has anyone reviewed this bug for Mono?
Comment 4 Marek Safar 2008-02-20 19:23:23 UTC
The issue is that we emit hidden dispose section after closing foreach `}'. But standard goto statement does not about this section. Fixing this simple case may be easy but it's quite tricky to fix global goto jump to any instruction.

We should probably emit some helper method which gets calls when the goto leaves the scope of any disposable iterator.

Comment 5 Marek Safar 2008-02-20 19:28:20 UTC
Reassigning to Hari.
Comment 6 Raja Harinath 2008-03-30 17:40:44 UTC
The 'goto' statement does know about it, and uses the 'leave' instruction.

However, since it's inside an iterator, the "finally" clauses are emitted inline instead of inside a finally handler, and thus the 'leave' has no effect.

One solution is to keep the unwind-protect areas as-is inside iterators, and branch around any 'finally' clauses with a bool that's set by a 'yield return'.

  ExceptionStatement::DoEmitFinally():
    Label endfinally
    BeginFinallyHandler()
    if (InIterator)
       emit (ldloc, "unwind_protect")
       emit (brfalse, endfinally)
    EmitFinally ()
    MarkLabel (endfinally)
    EndExceptionBlock()

  Iterator::MarkYield():
    emit (ld.i4.0)
    emit (stloc, "unwind_protect")
    ...emit loading PC and jump to ret...

  Iterator::EmitMoveNext():
    change outer try/finally to a try/fault handler

Let me see if this works out.
Comment 7 Raja Harinath 2008-03-30 17:42:57 UTC
That last change isn't necessary: MoveNext is already emitted as a try/fault block.
Comment 8 Raja Harinath 2008-04-02 17:02:51 UTC
Unfortunately that isn't enough, since we can't jump into a try statement.  I'm working on a series of patches to clean up this area.
Comment 9 Raja Harinath 2008-04-05 20:19:46 UTC
After a long series of cleanups, this should be fixed in SVN r99929.