Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix release build infinite loop #49943

Merged
merged 1 commit into from
Mar 21, 2021
Merged

Conversation

BruceForstall
Copy link
Member

The problem manifested as an infinite loop during the StackLevelSetter phase
in the release build SuperPMI replay of the tests, but also occurs as a
normal release build test run of the varargsupport.il test.

The issue is we had corrupt LIR gtPrev links, with a cycle. The problem had
nothing to do with StackLevelSetter -- it just happened to be the first phase
that iterated in reverse over the gtPrev links.

The corruption was introduced in the importer, in
verConvertBBToThrowVerificationException. It required a verification failure
in a filter (possibly also catch) clause where the JIT would throw away the
currently imported code and convert the block to a call to the verification
failure helper.

This was a classic case of important, functional code being under #ifdef DEBUG
that is needed in non-DEBUG as well.

The result was we would end up adding an ASG(LCL_VAR, CATCH_ARG) to the statement
list twice, with the same CATCH_ARG node.

Fixes #45580

The problem manifested as an infinite loop during the StackLevelSetter phase
in the release build SuperPMI replay of the tests, but also occurs as a
normal release build test run of the varargsupport.il test.

The issue is we had corrupt LIR gtPrev links, with a cycle. The problem had
nothing to do with StackLevelSetter -- it just happened to be the first phase
that iterated in reverse over the gtPrev links.

The corruption was introduced in the importer, in
`verConvertBBToThrowVerificationException`. It required a verification failure
in a filter (possibly also catch) clause where the JIT would throw away the
currently imported code and convert the block to a call to the verification
failure helper.

This was a classic case of important, functional code being under `#ifdef DEBUG`
that is needed in non-DEBUG as well.

The result was we would end up adding an `ASG(LCL_VAR, CATCH_ARG)` to the statement
list twice, with the same `CATCH_ARG` node.

Fixes dotnet#45580
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 20, 2021
@@ -5321,22 +5321,20 @@ bool Compiler::verMergeEntryStates(BasicBlock* block, bool* changed)
/*****************************************************************************
* 'logMsg' is true if a log message needs to be logged. false if the caller has
* already logged it (presumably in a more detailed fashion than done here)
* 'bVerificationException' is true for a verification exception, false for a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument no longer exists

@BruceForstall
Copy link
Member Author

@dotnet/jit-contrib PTAL

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr outerloop

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member

I thought all this was now dead code. How is this getting called?

@BruceForstall
Copy link
Member Author

There is this:

    // Set to TRUE if verification cannot be skipped for this method
    // CoreCLR does not ever run IL verification. Compile out the verifier from the JIT by making this a constant.
    // TODO: Delete the verifier from the JIT? (https://github.com/dotnet/runtime/issues/32648)
    // BOOL tiVerificationNeeded;
    static const BOOL tiVerificationNeeded = FALSE;

The caller is

verRaiseVerifyException(INDEBUG("stack must be 1 on end of filter") DEBUGARG(__FILE__)

@BruceForstall
Copy link
Member Author

jitstress failure is #49954

@AndyAyersMS
Copy link
Member

I'm still confused as to how verConvertBBToThrowVerificationException gets called.

What I'm wondering is whether we should BADCODE at the point we decide calling the above makes sense, instead of trying to defer things to runtime.

@BruceForstall
Copy link
Member Author

When verRaiseVerifyException on the bad IL (a filter expression that leaves the IL stack with 2 slots when reaching an endfilter), it raises an exception via RaiseException. It's caught here:

verHandleVerificationFailure(block DEBUGARG(false));

which calls verHandleVerificationFailure which calls verConvertBBToThrowVerificationException.

You're right that perhaps BADCODE is the right behavior; I don't understand why it was originally a verification error and deferred to runtime. I guess we allowed unverifiable code to run and only lazily fail?

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

We can sort out the rest when we delete all the verifier support.

@BruceForstall BruceForstall merged commit d65f54a into dotnet:main Mar 21, 2021
@BruceForstall BruceForstall deleted the Fix45580 branch March 21, 2021 21:56
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite loop in stacklevelsetter
3 participants