JIT: tolerate malformed IL when counting block successors #83676
Conversation
If we run synthesis, we may now invoke `BasicBlock::NumSucc` before we've detected some cases of invalid IL, In particular an endfinally that is not within a finally (this gets detected during importation). Tolerate this by reporting that an endfinally with no handler index has no successors. Fixes dotnet#83674.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsIf we run synthesis, we may now invoke Tolerate this by reporting that an endfinally with no handler index has no successors. Fixes #83674.
|
|
@BruceForstall PTAL |
|
Failure looks like an instance of #82771 and is not related. |
| { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it be feasible to badcode on something like this earlier so we could have a nicer invariant on the data structures, making it simpler to work with this?
What about the other NumSucc variant?
There was a problem hiding this comment.
Would it be feasible to badcode on something like this earlier
I think in this case we can probably detect it earlier, when we're building the flow graph. I can look into this.
What about the other NumSucc variant?
BBJ_EHFINALLYRET is handled differently in the "simple" variant, since it takes some work to figure out where control goes when it leaves a finally; we don't represent this flow via graph edges.
It turns out that synthesis runs so early that the algorithm the complex variant uses won't work anyways (the relevant blocks are created by our friend impImportLeave which as the name implies happens in the importer), but synthesis wants the complex variant's behavior for other BBJ kinds.
There was a problem hiding this comment.
Would it be feasible to badcode on something like this earlier
I think in this case we can probably detect it earlier, when we're building the flow graph. I can look into this.
In the failing test case we have an endfinally in an inlinee. We could catch this earlier by calling fgCheckBasicBlockControlFlow before returning from fgMakeBasicBlocks for inlinees and for methods with no EH clauses.
But that seems like overkill for this case; we know immediately when we see the IL opcode that things are bad, since the method has no EH clauses. But thanks to the issue I was trying to resolve in #83622 we pollute the inlinee with the root method's EH table, so the check for "has no EH clauses" is not quite as simple as checking info.compXcptnsCount == 0.
So (assuming I don't merge #83622 because of the TP impact) the check has to be a bit more nuanced: BADCODE if we see an endfinally (or endfilter) and we're the root method and info.compXcptnsCount is zero, or always, if we're an inlinee (thus further deepening the assumption that we won't inline methods with EH).
There is also the possibility that the block with the wayward endfinally is unreachable in which case we might be able to jit the method and just ignore the problematic IL.
So I'm tempted to go with this fix.
There was a problem hiding this comment.
Ok, sounds good. Thanks for looking.
ghost
locked as resolved and limited conversation to collaborators
If we run synthesis, we may now invoke
BasicBlock::NumSuccbefore we've detected some cases of invalid IL, In particular an endfinally that is not within a finally (this gets detected during importation).Tolerate this by reporting that an endfinally with no handler index has no successors.
Fixes #83674.