This is an archive of the discontinued LLVM Phabricator instance.

[SEH]:Fix assertion when try is used inside catch(...) block with /EHa
ClosedPublic

Authored by jyu2 on May 10 2023, 7:56 PM.

Details

Summary

Current assert wiht /EHa with
A single unwind edge may only enter one EH pad

invoke void @llvm.seh.try.begin()
        to label %invoke.cont1 unwind label %catch.dispatch2

IR:
%1 = catchpad within %0 [ptr null, i32 0, ptr null]

invoke void @llvm.seh.try.begin()
        to label %invoke.cont5 unwind label %catch.dispatch2

The problem is the invoke to llvm.seh.try.begin() missing "funclet"

Accodring: https://llvm.org/docs/LangRef.html#ob-funclet
If any "funclet" EH pads have been entered but not exited (per the
description in the EH doc), it is undefined behavior to execute a
call or invoke.

To fix the problem, when emit seh_try_begin, call EmitSehScope,
instead of calling EmitRuntimeCallOrInvoke for proper "funclet"
gerenration.

Diff Detail

Event Timeline

jyu2 created this revision.May 10 2023, 7:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 7:56 PM
jyu2 requested review of this revision.May 10 2023, 7:56 PM
efriedma added inline comments.May 11 2023, 12:40 PM
clang/lib/CodeGen/CGException.cpp
650

Do we need to make the same change in EmitSEHTryStmt/ExitSEHTryStmt?

Is there some reason not to just call EmitSehTryScopeBegin here?

jyu2 added inline comments.May 11 2023, 1:53 PM
clang/lib/CodeGen/CGException.cpp
650

EmitSehTryScopeBegin: it is emit seh.scope.begin

In here we want to emit seh.try.begin. call EmitSehSCope with different function.

For EmitSEHTryStmt/ExitSEHTryStmt if is for try /except/__finally and it is for C code. I thought about that. And tried some test(BTW, try and __try can not be in same construct), I don't see the problem. So I did not add that.

efriedma added inline comments.May 11 2023, 2:27 PM
clang/lib/CodeGen/CGException.cpp
650

Not sure I understand what you're saying about EmitSehTryScopeBegin. CGException.cpp:45 refers to "llvm.seh.try.begin". CGCleanup.cpp:1370 also refers to "llvm.seh.try.begin"

For EmitSEHTryStmt/ExitSEHTryStmt, I guess __try codegen can't generate a construct quite like the given testcase, so thta's fine.

jyu2 updated this revision to Diff 521465.May 11 2023, 3:23 PM

Address Eli's comment. Thanks for review!.

:-( Sorry, You are right!! It is seems EmitSehTryScopeBegin is not called anywhere.

Changed. Thanks for the review.

efriedma accepted this revision.May 15 2023, 2:53 PM

LGTM... but I don't think the IR we're generating is really correct overall; see https://github.com/llvm/llvm-project/issues/62723

On a side-note, other open issues related to -EHa/__try:

https://github.com/llvm/llvm-project/issues/62606
D124642

This revision is now accepted and ready to land.May 15 2023, 2:53 PM
jyu2 added a comment.May 16 2023, 10:14 AM

LGTM... but I don't think the IR we're generating is really correct overall; see https://github.com/llvm/llvm-project/issues/62723

On a side-note, other open issues related to -EHa/__try:

https://github.com/llvm/llvm-project/issues/62606
D124642

Thanks Eli for the code review!! I will take look about missing seh.try.end in c++ try/catch part later. In term of D124642, I'd really want to look at IR for return inside try/finally, but for some reason, I an not build compiler with your patch: no member named 'setHasAddressTaken' in 'llvm::MachineBasicBlock'; did you mean 'hasAddressTaken'. I may missing some thing in my environment.

clang/lib/CodeGen/CGException.cpp
650

:-( Sorry, You are right!! It is seems EmitSehTryScopeBegin is not called anywhere.

Changed. Thanks for the review.

I'd really want to look at IR for return inside try/finally, but for some reason, I an not build compiler with your patch: no member named 'setHasAddressTaken' in 'llvm::MachineBasicBlock'; did you mean 'hasAddressTaken'. I may missing some thing in my environment.

The patch is old; I was having trouble getting any review, and then I forgot about it. I'll try to find time to rebase.

This revision was automatically updated to reflect the committed changes.