[PATCH] D154294: [Windows SEH]: Do not fold branches for MSVC TableSEH function

Thu Aug 31 19:30:02 PDT 2023

pengfei added a comment.

In D154294#4631804 <https://reviews.llvm.org/D154294#4631804>, @rnk wrote:

> In D154294#4630056 <https://reviews.llvm.org/D154294#4630056>, @pengfei wrote:
>
>> `bb.5.invoke.cont4` to landing pad (I assume it's `bb.7.ehcleanup`) doesn't matter to me here. I analyzed the problem was the edge from `bb.3.invoke.cont2` to `bb.4.__except` was removed in BranchFolding. This is the direct problem I wanted to solve here. It seems we are talking about independent problems. I didn't research the other but I think it doesn't solve my problem.
>
> I agree, that is what is happening. bb3 is a simple JMP to bb5. BranchFolding sees it as an empty block and nobody else jumps to bb5, so it wants to delete it and RAUW all uses of bb3 with bb5. BranchFolding doesn't transfer the unwind edges from bb3 to bb5, so they are all dropped and the EH pads become dead. If transferred the EH edges to bb5, it would keep the EH pads alive in the CFG. So, maybe transferring those EH edges is an OK solution?

I think so, this can solve the specific problem here.

> Still, having them present in the first place as Eli says would solve issues in other passes. From reading the state numbering code in winehprepare, it seems like we already assign all these blocks to the try state number, which means any exception in them will unwind to the catchpad. Can we use the state numbers during machine CFG construction to add the missing edges? To say it another way, state numbering is the dataflow algorithm we need to apply the missing edges. It's a matter of transferring that from IR to MIR.

That would be feasible. However, if we built the edges from all BBs in try region to catchpad, the CFG may turn into a too complex graph to understand for developers who don't know much about SEH. Instead, I think it would be much clear if we taking state numbers into account just when doing CFG optimization.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154294