[PATCH] D138123: [Verifier][WinEH] Check funclet tokens on intrinsic calls that may lower to function calls

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

Thanks for your feedback @efriedma

In D138123#4067201 <https://reviews.llvm.org/D138123#4067201>, @efriedma wrote:

> Would it make sense to check calls to non-intrinsic functions here?  I don't see any meaningful distinction between an intrinsic that will be lowered to a non-intrinsic call, vs. an actual call.

Front-ends that target WinEH are responsible for setting up funclet tokens for function calls. In Clang this happens in generic code paths and it works reliably. While we could check it here for all cases, it doesn't seem necessary and it would add overhead. The compromise to check funclet tokens for certain intrinsic calls arised from the fact that middle-end passes can add/replace intrinsics and sometimes miss to set up funclet tokens. One current example is here: https://reviews.llvm.org/D137944#C3801853OL1776

It would be handy to catch these cases early in the verifier. Reid explained some more background here: https://reviews.llvm.org/D134866#3824796

> Would it make sense to allow dangling funclet tokens in unreachable code?  In particular, I'm concerned about cases where a bit of code is logically inside a funclet, but control flow simplification cuts it off from the funclet entry.  (WinEHPrepare will throw away unreachable code anyway, so it shouldn't care.)

Good point. I am not an expert for details in simplifycfg. If it produces such cases legally and doesn't clean them up (and instead rely on WinEHPrepare), that's odd but it needs to be considered. Dangling tokens were the symptom I observed in https://reviews.llvm.org/D134866 (which turned out to be a front-end codegen issue) and catching them here would be great, but they used to be less frequent than missing token cases and I guess we could live without them for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138123