[PATCH] D134866: [WinEH] Fix PreISel intrinsics in funclet catchret.dest blocks

[Reid Kleckner via Phabricator via llvm-commits]

rnk requested changes to this revision.
rnk added subscribers: majnemer, lebedev.ri.
rnk added a comment.
This revision now requires changes to proceed.

+ at majnemer

I'm still concerned about this change. If you make this change, we might want to reconsider the entire funclet bundle design, basically.

The way we designed funclets, we basically assumed the frontend was responsible for generating all the call instructions. This is obviously a problem for middle end passes creating function calls, like any instrumentation tool or this ObjC ARC stuff, and that's why this keeps coming up again.

The risk of not discarding implausible blocks is that valid IR transforms, such as simplifycfg, will create IR that we cannot compile because we can't assign unique colors and EH state numbers to all blocks. The most plausible example of this tail merging, which I think @lebedev.ri was working on, but I don't recall the status.

I think, in the end, this whole funclet bundle design was a way of coming up with an invariant that we could enforce on IR producers that would prevent valid IR transforms from creating IR that we couldn't compile. The motivation of all this is to improve reliability. It was meant to prevent users from hitting problems, filing bugs, finding us, and asking us to make difficult changes to WinEH code. It was always about shifting the problem of preserving the EH structure onto the IR producer, to make it easier to reconstruct the structured nesting in the backend. Which is a valid motivation: we got burned from trying to do heroic backend structure reconstruction the first time we tried to implement this. However, now we have this invariant about how funclet bundles should be placed, and the IR producer needs to obey the invariant, and the consequences of breaking that invariant are that the user sees random crashes.

I think we can say that this plan really hasn't worked out. Nobody knows about these WinEH funclet IR invariants, every instrumentation tool breaks them or has to carry code to preserve them, and people keep hitting bugs. Moving the problem from WinEH lowering to IR production has not ultimately made the compiler more reliable form the perspective of the user, and people root cause the problem to this WinEH code that removes implausible blocks.

---

So what should we do about this? I think it would be a better user experience if we accepted that EH state numbering was a fallible algorithm. If we have valid IR that we try to lower to use a structured Windows EH personality but we can't do it because coloring fails, we should just report a compiler error. That would be way better than the current experience of crashing at runtime because the user happened to use a feature (ARC) that wasn't tested with WinEH. If we did this, we'd be able to delete quite a lot of code and simplify things across the codebase.

For extra bonus points, it would be great if we reported errors via the LLVMContext rather than aborting the program with report_fatal_error.

What do people think about this? I can't ask @sgraenitz to accept the huge increase in scope, but if we could at least document this as the intended direction in the WinEH docs and as a github issue, that would be enough for me to say we should land this as is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134866/new/

https://reviews.llvm.org/D134866