Exception Handler support

[333fred]

Our current approach to rewriting await expressions in finally blocks is technically unsound: the rewrite can result in a new exit block from the original method being created. As a simple example, here is an original method, and what it gets rewritten to by AsyncExceptionHandlerRewriter (slightly simplified):

static async Task<int> M()
{
    int x = 0;

    try
    {
        x = await F();
        return x;
    }
    finally
    {
        x += await F();
    }
}

async Task<int> M()
{
    int x = 0;
    object ex = null;
    int retTaken = 0;
    int retTemp = 0;
    try
    {
        x = await F();
        goto @finally;
    }
    catch (object o)
    {
        ex = o;
        goto @finally;
    }

@finally:
    x += await F();
    if (ex != null)
    {
        if (ex is Exception e) ExceptionDispatchInfo.Capture(e).Throw();
        throw ex;
    }

    switch(retTaken)
    {
        case 2:
            return retTemp;
        default:
            break; // This is the problem
    }
}

While we can statically walk the method and prove via flow analysis that the default case of the switch isn't reachable, the generalized version of this problem is the halting problem. To date, this has never been a problem because the code that the async rewriter produces is then wrapped and rewritten again into the async state machine, and the missing return becomes completely unobservable. However, for runtime async, we're going to be emitting this general structure, essentially unchanged: that break;, then, becomes a branch to an invalid IL location and the method ends without a ret. We're taking a conservative approach to fixing this by appending a throw null; at the end of every method that has an await in a finally. Most of the time, this is provably never reachable, and the basic block reducer will eliminate it. However, sometimes a standard control flow analysis will not be able to prove that the throw null; is not reachable, and it will remain in the final IL. It is not reachable, but its presence will ensure that the method is always valid.

async Task<int> M()
{
    int x = 0;
    object ex = null;
    int retTaken = 0;
    int retTemp = 0;
    try
    {
        x = await F();
        goto @finally;
    }
    catch (object o)
    {
        ex = o;
        goto @finally;
    }

@finally:
    x += await F();
    if (ex != null)
    {
        if (ex is Exception e) ExceptionDispatchInfo.Capture(e).Throw();
        throw ex;
    }

    switch(retTaken)
    {
        case 2:
            return retTemp;
        default:
            break;
    }

    throw null; // Unreachable, but not easily provable statically
}

Relates to test plan #75960

[333fred]

@jcouv @RikkiGibson this is ready for review. I'll likely have another commit sometime tomorrow that also adds runtime async verification for await foreach tests, but I don't anticipate that to change any of the core implementation changes so I think you should be good to proceed with the initial set.

[AlekseyTs]

_F.Throw(_F.Null(_F.SpecialType(SpecialType.System_Object)))

Is this going to work for async void methods? Will there be an explicit return after all user code in the method is exhausted? #Resolved

[333fred]

I hope it will, since I expect the automatic implicit return code to not care about whether it's just void or async void, but it's a good test to add.

[333fred]

Yes, the automatic implicit return worked here. See AsyncInFinally006_AsyncVoid, newly added.

[RikkiGibson]

getting through tests going to take a little while.

[RikkiGibson]

Done review pass, had some comments on the tests.

[jcouv]

(awaitInfo.GetResult ?? awaitInfo.RuntimeAsyncAwaitMethod

Can awaitInfo.GetResult ?? awaitInfo.RuntimeAsyncAwaitMethod be null when there's no errors?
I still think it's be a good idea to add a Validate method to BoundAwaitableInfo to codify expectations.

[333fred]

It can, when dynamic is involved.

[jcouv]

FWIW, I made the following addition and ran build.cmd -testCompilerOnly -testCoreClr and go no hit:

+                if (!hasErrors)
+                {
+                    Debug.Assert((awaitInfo.GetResult ?? awaitInfo.RuntimeAsyncAwaitMethod) is not null);
+                }

[333fred]

It's possible dynamically-bound awaits do not go through here, but you absolutely can have both methods be null and not have errors.

[jcouv]

            // (3,1): hidden CS8019: Unnecessary using directive.

Test AsyncWithEH, line 150: Do we understand what API we used for async that we no longer use for runtime-async?

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncEHTests.cs:150 in 92399e0. [](commit_id = 92399e0, deletion_comment = False)

[jcouv]

                    [G]: Unexpected type on the stack. { Offset = 0x104, Found = Int32, Expected = ref 'System.Threading.Tasks.Task`1<int32>' }

Test AsyncWithEHCodeQuality, line 587: Did we file an issue on ILVerify already? It'd be good to link here and other similar places

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncEHTests.cs:587 in 92399e0. [](commit_id = 92399e0, deletion_comment = False)

[jcouv]

        verifier.VerifyDiagnostics(

nit: Test AsyncWithException1, line 690: Consider also verifying diagnostics on the async baseline above, and possibly removing the async from F
Also applies to some other tests below (AsyncWithException2 and many more)

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncEHTests.cs:690 in 503c056. [](commit_id = 503c056, deletion_comment = False)

[jcouv]

                IL_0145:  ldnull

nit: Thanks for pointing out this test. Consider leaving a comment for what to look for in the IL (this is an async test where the new "throw null" is survives and is observed). Or maybe in AsyncInFinally003 since it already had an IL baseline #Closed

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncEHTests.cs:1749 in 503c056. [](commit_id = 503c056, deletion_comment = False)

[jcouv]

        verifier.VerifyIL("Test.G(System.Threading.SemaphoreSlim)", """

Test AsyncInFinally006_AsyncVoid_01, line 2059 (last VerifyIL): Was there anything in particular to observe here? This should not have been affected by the change in this PR, right?

---

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncEHTests.cs:2059 in 503c056. [](commit_id = 503c056, deletion_comment = False)

[333fred]

Was there anything in particular to observe here? This should not have been affected by the change in this PR, right?

Correct, I'm verifying that fact.

[jcouv]

Done with review pass (commit 14)

[333fred]

Do we understand what API we used for async that we no longer use for runtime-async?

None. The non-runtime async code does not verify diagnostics, this is present on both.

[333fred]

nit: Thanks for pointing out this test. Consider leaving a comment for what to look for in the IL (this is an async test where the new "throw null" is survives and is observed). Or maybe in AsyncInFinally003 since it already had an IL baseline

I'm not sure that comment would be especially useful after this review?

[333fred]

nit: Test AsyncWithException1, line 690: Consider also verifying diagnostics on the async baseline above, and possibly removing the async from F

For the former; to be honest, I don't think the effort is worth it here. For the latter, that would potentially change the behavior of the test unless I did more extensive rewrites to change to Task.FromException, which I also don't think is worth it.

[jcouv]

LGTM Thanks (commit 14)

[jcouv]

LGTM Thanks (commit 16)