[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

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

sgraenitz marked an inline comment as done.
sgraenitz added a comment.

My follow-up got delayed, because I hit another bug, which appears to be a regression in release 14. This is why I wrote the tests for release/13.x and I still have to port them back to mainline, so this is *not yet ready to land*. As I don't expect it to cause big changes, however, I though it might be good to update the review anyway.

Rebased on release/13.x I am running the tests like this and both are passing:

  > ninja llvm-config llvm-readobj llc FileCheck count not
  > bin\llvm-lit.py --filter=win64-funclet-preisel-intrinsics test\CodeGen\X86
  > ninja clang
  > bin\llvm-lit.py --filter=arc-exceptions-seh tools\clang\test

@rnk What do you think about implementation, naming, etc. Is that ok?

In D124762#3486288 <https://reviews.llvm.org/D124762#3486288>, @rnk wrote:

> Lastly, I think the inliner needs to be taught that these intrinsics need to carry operand bundles, otherwise this problem will arise after optimization. The inliner has the same logic here:
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2311

Thanks for the note. Yes, I can reproduce that. I am not yet sure what's the best way to fix it though. Actually, I'd prefer to split it out into a separate review, so the two discussions can remain their individual focus. What do you think?



================
Comment at: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm:1
+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+
----------------
Note: There is another test without the `-seh` postfix that checks very similar situations for CodeGen with the macOS runtime.


================
Comment at: llvm/lib/CodeGen/WinEHPrepare.cpp:966
 
-        if (FuncletBundleOperand == FuncletPad)
+        if (!FuncletPad || FuncletBundleOperand == FuncletPad)
           continue;
----------------
sgraenitz wrote:
> rnk wrote:
> > Is this change necessary? It seems like it shouldn't be after the clang and preisel pass changes.
> Yes, apparently it is necessary. There are cases, where `CodeGenFunction::getBundlesForFunclet()` registers a "funclet" bundle operand, but the `FuncletPad` here is null. My guess was it might be due to optimizations?
Might that be due to the inliner issue then? I'd address it in the follow-up review if it's ok?


================
Comment at: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll:53
+; At this point the code used to be truncated:
+;     CHECK-NOT: int3
+;
----------------
Another side-note: Often times Clang just didn't emit anything here, so execution would run into whatever code comes next. The `int3` I sometimes got might as well have been "accidental" padding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124762