This is an archive of the discontinued LLVM Phabricator instance.

[WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime
AbandonedPublic

Authored by sgraenitz on May 2 2022, 5:05 AM.

Details

Summary

Unwinding ObjC code with automatic reference counting involves calls to intrinsics like llvm.obj.retain and llvm.objc.destroyWeak. Exception handling on Windows is based on funclets and WinEHPrepare only allows calls to nounwind intrinsics. This works just fine, except for ObjC nounwind intrinsics, because these are lowered into regular function calls in an earlier stage already (i.e. PreISelIntrinsicLoweringPass). Thus, WinEHPrepare accidentally drops them as implausible instructions and silently truncates certain funclets. Eventually, this causes crashes during unwinding on Windows with the GNUstep ObjC runtime: https://github.com/gnustep/libobjc2/issues/222

This patch forces the emission of a "funclet" bundle operand for calls to ObjC ARC intrinsics during codegen and lets PreISelIntrinsicLowering carry it over onto lowered replacement calls. This way all ObjC ARC calls survive WinEHPrepare through the FuncletBundleOperand check. The latter had to be adjusted in order to allow funclet pads to get optimized away.

Diff Detail

Event Timeline

sgraenitz created this revision.May 2 2022, 5:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 5:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sgraenitz requested review of this revision.May 2 2022, 5:05 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 2 2022, 5:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I guess testing must be split in two:

rnk added a comment.May 2 2022, 11:24 AM

Thanks!

I guess testing must be split in two:

That seems like a good template for a new test. I think it probably deserves its own file. I imagine it will grow to be a more extensive test suite covering the interaction between GNUstep Objective C exceptions and WinEH.

You could go that route, but I would probably start with simplified ObjC++ code, and iteratively modify that until you get the IR that has the funclets that you want to see. This is a decent starting template:

void maythrow();
void arcOpsInFunclet() {
  try {
    maythrow();
  }
  catch (...) {
    // Do things that generate ARC intrinsics, probably set up an `id` variable.
  }
}

Maybe you could make the test plain ObjC, but I don't know it very well.

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

The test case for this should be:

struct Foo {
  inline __attribute__((always_inline)) ~Foo() { /* ARC ops to inline */ }
};
void maythrow();
void bar() {
  Foo o;
  maythrow();
}
llvm/lib/CodeGen/WinEHPrepare.cpp
966

Is this change necessary? It seems like it shouldn't be after the clang and preisel pass changes.

Thanks for your help! I will work on testing in calendar week 21.

llvm/lib/CodeGen/WinEHPrepare.cpp
966

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?

sgraenitz updated this revision to Diff 434020.Jun 3 2022, 7:26 AM

Fix unchecked nullptr compiler crash and assertion

sgraenitz updated this revision to Diff 434023.Jun 3 2022, 7:31 AM

LLVM CodeGen: check that presence of bundle operands avoids truncation

sgraenitz updated this revision to Diff 434024.Jun 3 2022, 7:32 AM

Clang frontend: check that 'funclet' bundle operands are emitted for Pre-ISel intrinsics

sgraenitz marked an inline comment as done.Jun 3 2022, 7:59 AM

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?

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?

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
1

Note: There is another test without the -seh postfix that checks very similar situations for CodeGen with the macOS runtime.

llvm/lib/CodeGen/WinEHPrepare.cpp
966

Might that be due to the inliner issue then? I'd address it in the follow-up review if it's ok?

llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
53

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.

sgraenitz added a comment.EditedJun 3 2022, 8:06 AM

For illustration, truncations look like this (left: old CodeGen, right: with this fix applied)

sgraenitz updated this revision to Diff 436730.Jun 14 2022, 3:41 AM

Update LLVM CodeGen test for mainline opaque pointers

sgraenitz updated this revision to Diff 437179.Jun 15 2022, 8:04 AM

Fix accidental functional change that failed Clang test CodeGenCXX/microsoft-abi-eh-inlineasm.cpp

sgraenitz updated this revision to Diff 437180.Jun 15 2022, 8:05 AM

Drop GNUstep assertion from getBundlesForFunclet(). Indeed a similar issue was observed before. See: D44640

sgraenitz abandoned this revision.Jul 26 2022, 3:20 AM

Close in favor of the D128190, which covers both cases at once: emit (was D124762) and inline (was D127962)