Skip to content

[CodeGen] Avoid generating trap instructions after exception restore intrinsics #109560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+48 −2
Open

[CodeGen] Avoid generating trap instructions after exception restore intrinsics #109560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/include/llvm/IR/Intrinsics.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,8 +1405,8 @@ let IntrProperties = [IntrNoMem, IntrSpeculatable, IntrWillReturn] in {
// given function it is 'const' and may be CSE'd etc.
def int_eh_typeid_for : Intrinsic<[llvm_i32_ty], [llvm_anyptr_ty], [IntrNoMem]>;

def int_eh_return_i32 : Intrinsic<[], [llvm_i32_ty, llvm_ptr_ty]>;
def int_eh_return_i64 : Intrinsic<[], [llvm_i64_ty, llvm_ptr_ty]>;
def int_eh_return_i32 : Intrinsic<[], [llvm_i32_ty, llvm_ptr_ty], [IntrNoReturn]>;
def int_eh_return_i64 : Intrinsic<[], [llvm_i64_ty, llvm_ptr_ty], [IntrNoReturn]>;
Comment on lines +1408 to +1409
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you still need to special case this in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused as to what you mean. The Clang intrinsic, __builtin_eh_return is marked as NoReturn, everything essentially treats it as noreturn due to unreachable being emitted immediately after them, and the verifier would complain if you emitted code after them (which is exactly what this PR is trying to fix).

Marking these as noreturn accurately represents their behavior and makes it so that the patch can be a bit smaller since I don't need to reorganize logic in the instruction selectors.

Copy link
Contributor

@arsenm arsenm Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IR intrinsic noreturn attribute is separate from the machine verifier issue you are seeing. You can do this as a pre-commit (which is independently observable in IR optimizations).

You're also not making use of this attribute below?


// eh.exceptionpointer returns the pointer to the exception caught by
// the given `catchpad`.
Expand Down
8 changes: 8 additions & 0 deletions llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3125,6 +3125,14 @@ bool IRTranslator::translateUnreachable(const User &U, MachineIRBuilder &MIRBuil
// Do not emit an additional trap instruction.
if (Call->isNonContinuableTrap())
return true;
// Do not emit trap instructions after EH_RETURN intrinsics.
// This can cause problems later down the line when other machine passes
// attempt to use the last instruction in a BB to determine terminator behavior.
if (const auto *II = dyn_cast<IntrinsicInst>(Call)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be handled in isNonContinuableTrap?

const auto IID = II->getIntrinsicID();
if (IID == Intrinsic::eh_return_i32 || IID == Intrinsic::eh_return_i64)
return true;
}
Comment on lines +3131 to +3135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These intrinsics are missing from the langref and I don't know what's expected of them. Should these really be terminators in the first place? Can the insert point just be adjusted?

use the last instruction in a BB to determine terminator behavior.

This comment doesn't exactly make sense

Copy link
Contributor Author

@duk-37 duk-37 Sep 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these really be terminators in the first place? Can the insert point just be adjusted?

Yes and no, respectively. They're old exception-handling intrinsics that lower down to ISD::EH_RETURN through the SelectionDAG, which is then lowered to target-specific EH_RETURN opcodes across different architectures. These opcodes are all marked as "return instructions" in various .td files.

The problem becomes that these are not actually return instructions, they're intrinsics. When they're lowered, we visit an unreachable instruction after the EH_RETURN opcode is emitted through the intrinsic, meaning that if trap-unreachable is enabled we emit a TRAP instruction after a terminator. You can see this behavior here: https://godbolt.org/z/5br41sWqf

We therefore also get blocks where MachineBasicBlock::isReturnBlock() returns false when it would have returned true before, as the function checks the last instruction in a block to figure out whether it's a "return block" or not. This is used, for example, in PrologEpilogInsertion to determine where to restore registers.

I'm not quite sure if this is a miscompile, but returning false in isReturnBlock is definitely not what we want to happen here. Also, the restore behavior is explicitly checked in tests, so I'm not going to bank on that. My goal here is essentially to just preserve the current behavior (and not crash in the machine verifier) when trap-unreachable is set to true.

Looking at this again, I'm not sure these intrinsics are even supported in GlobalISel on any architecture, but it would be good to keep this consistent between instruction selection backends.

}

MIRBuilder.buildTrap();
Expand Down
8 changes: 8 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3553,6 +3553,14 @@ void SelectionDAGBuilder::visitUnreachable(const UnreachableInst &I) {
// Do not emit an additional trap instruction.
if (Call->isNonContinuableTrap())
return;
// Do not emit trap instructions after EH_RETURN intrinsics.
// This can cause problems later down the line when other machine passes
// attempt to use the last instruction in a BB to determine terminator behavior.
if (const auto *II = dyn_cast<IntrinsicInst>(Call)) {
const auto IID = II->getIntrinsicID();
if (IID == Intrinsic::eh_return_i32 || IID == Intrinsic::eh_return_i64)
return;
}
}

DAG.setRoot(DAG.getNode(ISD::TRAP, getCurSDLoc(), MVT::Other, DAG.getRoot()));
Expand Down
15 changes: 15 additions & 0 deletions llvm/test/CodeGen/X86/eh-trap-unreachable-x86.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
; RUN: llc < %s -mtriple=i386-unknown-unknown -trap-unreachable -no-trap-after-noreturn=false -verify-machineinstrs | FileCheck %s

;; Ensure we restore caller-saved registers before EH_RETURN, even with trap-unreachable enabled.
;; For now, we deliberately avoid generating traps altogether.

; CHECK-LABEL: test32
; CHECK: pushl
; CHECK: popl
; CHECK: eh_return
; CHECK-NOT: ud2
define void @test32(i32 %offset, ptr %handler) {
call void @llvm.eh.unwind.init()
call void @llvm.eh.return.i32(i32 %offset, ptr %handler)
unreachable
}
15 changes: 15 additions & 0 deletions llvm/test/CodeGen/X86/eh-trap-unreachable-x86_64.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
; RUN: llc < %s -mtriple=x86_64-unknown-unknown -trap-unreachable -no-trap-after-noreturn=false -verify-machineinstrs | FileCheck %s

;; Ensure we restore caller-saved registers before EH_RETURN, even with trap-unreachable enabled.
;; For now, we deliberately avoid generating traps altogether.

; CHECK-LABEL: test64
; CHECK: pushq
; CHECK: popq
; CHECK: eh_return
; CHECK-NOT: ud2
define void @test64(i64 %offset, ptr %handler) {
call void @llvm.eh.unwind.init()
call void @llvm.eh.return.i64(i64 %offset, ptr %handler)
unreachable
}
Loading