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

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

sgraenitz added a comment.

Thanks for your feedback and the historic context. I am not familiar with all the details, but let me try and digest the pieces I understand. Meanwhile, I don't want to interrupt the conceptual conversation kicked-off by @rnk and @efriedma -- so @majnemer  and @lebedev.ri please don't hesitate to follow up on it.

In D134866#3824796 <https://reviews.llvm.org/D134866#3824796>, @rnk wrote:

> 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.

All such ObjC ARC calls go to the runtime and won't throw. Might that even help the overall approach? What if transformations/instrumentation could add arbitrary calls later on as long as they are `nounwind`? What prevents WinEH from considering them as regular instructions? If I understand correctly, the stack remains intact during unwinding in the WinEH model, so all regular program state should still be accessible, right?

> It was always about shifting the problem of preserving the EH structure onto the IR producer.

If we wanted to fix this properly, we'd need to emit the IR differently. Looking at the specific test case in this patch, it appears that emitting the `@llvm.objc.storeStrong` call in the catch block solves the issue -- from the view of WinEH would that be equivalent semantically?

  catch:                                            ; preds = %catch.dispatch
    %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
    %exn = load ptr, ptr %exn.slot, align 8
    %2 = call ptr @llvm.objc.retain(ptr %exn) [ "funclet"(token %1) ]
    store ptr %2, ptr %ex2, align 8
    call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ] <-----+
    catchret from %1 to label %catchret.dest                                           |
                                                                                       |
  catchret.dest:                                    ; preds = %catch                   |
    ; call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ] ----+
    br label %eh.cont



> 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.

Sounds reasonable, these silent truncations are very bad. However, people started working with the current state and it won't help to prevent them from compiling their projects with an upcoming release. Could we add a compiler flag to disregard the error during a transition period? (And instead emit a warning?)

In D134866#3825468 <https://reviews.llvm.org/D134866#3825468>, @efriedma wrote:

> If we're going to relax the funclet bundle rules, I think we need to at least ensure that coloring fails reliably with a report_fatal_error if it's presented with uncolorable code. Does that happen currently?

If you are talking about funclet coloring, then no it doesn't look like funclet bundles are validated here right now: https://github.com/llvm/llvm-project/blob/release/15.x/llvm/lib/Analysis/EHPersonalities.cpp#L85


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134866