Skip to content

JIT: tolerate malformed IL when counting block successors #83676

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 22, 2023
Merged

JIT: tolerate malformed IL when counting block successors #83676

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1151,6 +1151,13 @@ unsigned BasicBlock::NumSucc(Compiler* comp)

case BBJ_EHFINALLYRET:
{
// We may call this method before we realize we have invalid IL. Tolerate.
//
if (!hasHndIndex())
{
return 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds good. Thanks for looking.

// The first block of the handler is labelled with the catch type.
BasicBlock* hndBeg = comp->fgFirstBlockOfHandler(this);
if (hndBeg->bbCatchTyp == BBCT_FINALLY)
Expand Down