Implement module cancellation and stack probing instrumentations

[tmat]

The goal of the stack probing instrumentation is to protect a host process executing user-specified C# code from accidental mistakes that cause unrecoverable stack overflow.

Similarly, the module-level cancellation instrumentation allows the host to avoid the compiled user code accidentally blocking for long time. The instrumentation:

• Adds CancellationToken static writable field to <PrivateImplementationDetails>. The host can set this token via Reflection before executing the compiled code.
• Inserts calls to ThrowIfCancellationRequested on the host token into each method, loop or goto.
• Replaces any tokens passed as arguments with the host token.
• Replaces calls to methods that do not take CancellationToken as the last parameter with matching overloads that do and passes it the host token.

The instrumentation may change semantics of the user specified code in rare cases, which is the trade-off the host needs to make.

Neither instrumentation provides guarantees that stack overflow will be avoided, or the code won't run indefinitely.

Supports #71268
Public API: #71961

[tmat]

@dotnet/roslyn-compiler PTAL
@dotnet/roslyn-ide FYI

[AlekseyTs]

@tmat

Replaces any tokens passed as arguments with the host token

I might be misinterpreting this, but doesn't this mean that the application will no longer be able to cancel execution through user provided tokens (since they will be ignored)? Should the tokens be "combined" instead?

Do we already have a consumer for this instrumentation? Could you provide some details if that is the case?

Are we going to document this capability and how to take advantage of it somewhere?

[AlekseyTs]

@tmat It looks like there are some analyzer errors

[tmat]

I might be misinterpreting this, but doesn't this mean that the application will no longer be able to cancel execution through user provided tokens (since they will be ignored)? Should the tokens be "combined" instead?

We don't expect the user-specified code that is being executed by the host to explicitly handle cancellations in the current scenarios (executing small scripts that have access to the APIs exposed by the host), so I went with the simple implementation.

We could definitely combine the tokens. Shouldn't add too much complexity. I can follow up on that.

The cancellation instrumentation is very specialized API tailored to very specific scenarios (similarly to EnC APIs).

[jaredpar]

@jjonescz for second review

[AlekseyTs]

Done with review pass (commit 12)

[AlekseyTs]

@tmat It looks like there are legitimate test failures

[AlekseyTs]

Done with review pass (commit 13). Besides the test failure, I am still tracking the following threads:

• Implement module cancellation and stack probing instrumentations #71896 (review)
• Implement module cancellation and stack probing instrumentations #71896 (review)

[tmat]

Done with review pass (commit 13). Besides the test failure, I am still tracking the following threads:

Fixing the test failure.

• Implement module cancellation and stack probing instrumentations #71896 (review)

This is addressed - I have added code that makes the check insensitive to whitespace.

• Implement module cancellation and stack probing instrumentations #71896 (review)

The parameter is removed.

[AlekseyTs]

Done with review pass (commit 14). Still tracking #71896 (review)

[AlekseyTs]

trumented

Typo?

[AlekseyTs]

LGTM (commit 15)

[AlekseyTs]

@jjonescz Please review the latest revision

[RikkiGibson]

I am wondering if any unexpected behavior changes could occur if:

• the original method was a readonly struct method and the replacement method was not, for example.
• Or, the other way around, the original is non-readonly and the replacement is readonly.

Since it seems like we are not checking the readonly-ness of the this parameter in these checks.

Since this PR is already merged some time ago, I will open a new issue based on this comment.