[llvm][SelectionDAG] Fix up chains in lowerInvokeable. rdar://113994760 #94004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-x86 Author: Jon Roelofs (jroelofs) ChangeslowerInvokeable wasn't updating the returned chain after emitting the lowerEndEH, which caused SwiftErrorVal-handling code to re-set the DAG root, and thus accidentally skip the EH_LABEL node it was supposed to have addeed. After fixing that, a few places needed to be adjusted that assume the specific shape of the returned DAG. Fixes: #64826 Full diff: https://github.com/llvm/llvm-project/pull/94004.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 85e4cc3b82e6e..7ffb53ccf4b47 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -8592,6 +8592,7 @@ SelectionDAGBuilder::lowerInvokable(TargetLowering::CallLoweringInfo &CLI,
if (EHPadBB) {
DAG.setRoot(lowerEndEH(getRoot(), cast_or_null<InvokeInst>(CLI.CB), EHPadBB,
BeginLabel));
+ Result.second = getRoot();
}
return Result;
@@ -10389,6 +10390,8 @@ void SelectionDAGBuilder::visitPatchpoint(const CallBase &CB,
std::pair<SDValue, SDValue> Result = lowerInvokable(CLI, EHPadBB);
SDNode *CallEnd = Result.second.getNode();
+ if (CallEnd->getOpcode() == ISD::EH_LABEL)
+ CallEnd = CallEnd->getOperand(0).getNode();
if (HasDef && (CallEnd->getOpcode() == ISD::CopyFromReg))
CallEnd = CallEnd->getOperand(0).getNode();
diff --git a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
index d7f4d1c893756..671ec84fb9416 100644
--- a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
@@ -340,6 +340,9 @@ static std::pair<SDValue, SDNode *> lowerCallFromStatepointLoweringInfo(
// to grab the return value from the return register(s), or it can be a LOAD
// to load a value returned by reference via a stack slot.
+ if (CallEnd->getOpcode() == ISD::EH_LABEL)
+ CallEnd = CallEnd->getOperand(0).getNode();
+
bool HasDef = !SI.CLI.RetTy->isVoidTy();
if (HasDef) {
if (CallEnd->getOpcode() == ISD::LOAD)
diff --git a/llvm/test/CodeGen/X86/issue64826-switferror-eh.ll b/llvm/test/CodeGen/X86/issue64826-switferror-eh.ll
new file mode 100644
index 0000000000000..8ea003d7a56f5
--- /dev/null
+++ b/llvm/test/CodeGen/X86/issue64826-switferror-eh.ll
@@ -0,0 +1,75 @@
+; RUN: llc %s -filetype=obj -o - | llvm-readobj -r - | FileCheck %s --check-prefix=RELOC
+; RUN: llc %s -mtriple=x86_64-apple-macosx -o - | FileCheck %s --check-prefix=ASM
+
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx"
+
+define swiftcc void @rdar113994760() personality ptr @__gcc_personality_v0 {
+entry:
+ %swifterror = alloca swifterror ptr, align 8
+ invoke swiftcc void null(i64 0, ptr null, ptr swifterror %swifterror)
+ to label %.noexc unwind label %tsan_cleanup
+
+.noexc: ; preds = %entry
+ ret void
+
+tsan_cleanup: ; preds = %entry
+ %cleanup.lpad = landingpad { ptr, i32 }
+ cleanup
+ resume { ptr, i32 } zeroinitializer
+}
+
+declare i32 @__gcc_personality_v0(...)
+
+; RELOC-LABEL: Relocations [
+; RELOC-NEXT: Section __text {
+; RELOC-NEXT: 0x18 1 2 1 X86_64_RELOC_BRANCH 0 __Unwind_Resume
+; RELOC-NEXT: }
+; RELOC-NEXT: Section __eh_frame {
+; RELOC-NEXT: 0x13 1 2 1 X86_64_RELOC_GOT 0 ___gcc_personality_v0
+; RELOC-NEXT: }
+; RELOC-NEXT: ]
+
+; ASM-LABEL: rdar113994760:
+; ASM: ## %bb.0: ## %entry
+; ASM-NEXT: pushq %r12
+; ASM-NEXT: .cfi_def_cfa_offset 16
+; ASM-NEXT: subq $16, %rsp
+; ASM-NEXT: .cfi_def_cfa_offset 32
+; ASM-NEXT: .cfi_offset %r12, -16
+; ASM-NEXT: Ltmp0:
+; ASM-NEXT: xorl %eax, %eax
+; ASM-NEXT: xorl %edi, %edi
+; ASM-NEXT: xorl %esi, %esi
+; ASM-NEXT: callq *%rax
+; ASM-NEXT: Ltmp1:
+; ASM-NEXT: ## %bb.1: ## %.noexc
+; ASM-NEXT: addq $16, %rsp
+; ASM-NEXT: popq %r12
+; ASM-NEXT: retq
+; ASM-NEXT: LBB0_2: ## %tsan_cleanup
+; ASM-NEXT: Ltmp2:
+; ASM-NEXT: xorl %edi, %edi
+; ASM-NEXT: callq __Unwind_Resume
+; ASM-NEXT: Lfunc_end0:
+; ASM-NEXT: .cfi_endproc
+; ASM-NEXT: .section __TEXT,__gcc_except_tab
+; ASM-NEXT: .p2align 2, 0x0
+; ASM-NEXT: GCC_except_table0:
+; ASM-NEXT: Lexception0:
+; ASM-NEXT: .byte 255 ## @LPStart Encoding = omit
+; ASM-NEXT: .byte 255 ## @TType Encoding = omit
+; ASM-NEXT: .byte 1 ## Call site Encoding = uleb128
+; ASM-NEXT: .uleb128 Lcst_end0-Lcst_begin0
+; ASM-NEXT: Lcst_begin0:
+; ASM-NEXT: .uleb128 Ltmp0-Lfunc_begin0 ## >> Call Site 1 <<
+; ASM-NEXT: .uleb128 Ltmp1-Ltmp0 ## Call between Ltmp0 and Ltmp1
+; ASM-NEXT: .uleb128 Ltmp2-Lfunc_begin0 ## jumps to Ltmp2
+; ASM-NEXT: .byte 0 ## On action: cleanup
+; ASM-NEXT: .uleb128 Ltmp1-Lfunc_begin0 ## >> Call Site 2 <<
+; ASM-NEXT: .uleb128 Lfunc_end0-Ltmp1 ## Call between Ltmp1 and Lfunc_end0
+; ASM-NEXT: .byte 0 ## has no landing pad
+; ASM-NEXT: .byte 0 ## On action: cleanup
+; ASM-NEXT: Lcst_end0:
+; ASM-NEXT: .p2align 2, 0x0
+; ASM-NEXT: ## -- End function
\ No newline at end of file
|
|
@llvm/pr-subscribers-llvm-selectiondag Author: Jon Roelofs (jroelofs) ChangeslowerInvokeable wasn't updating the returned chain after emitting the lowerEndEH, which caused SwiftErrorVal-handling code to re-set the DAG root, and thus accidentally skip the EH_LABEL node it was supposed to have addeed. After fixing that, a few places needed to be adjusted that assume the specific shape of the returned DAG. Fixes: #64826 Full diff: https://github.com/llvm/llvm-project/pull/94004.diff 3 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 85e4cc3b82e6e..7ffb53ccf4b47 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -8592,6 +8592,7 @@ SelectionDAGBuilder::lowerInvokable(TargetLowering::CallLoweringInfo &CLI,
if (EHPadBB) {
DAG.setRoot(lowerEndEH(getRoot(), cast_or_null<InvokeInst>(CLI.CB), EHPadBB,
BeginLabel));
+ Result.second = getRoot();
}
return Result;
@@ -10389,6 +10390,8 @@ void SelectionDAGBuilder::visitPatchpoint(const CallBase &CB,
std::pair<SDValue, SDValue> Result = lowerInvokable(CLI, EHPadBB);
SDNode *CallEnd = Result.second.getNode();
+ if (CallEnd->getOpcode() == ISD::EH_LABEL)
+ CallEnd = CallEnd->getOperand(0).getNode();
if (HasDef && (CallEnd->getOpcode() == ISD::CopyFromReg))
CallEnd = CallEnd->getOperand(0).getNode();
diff --git a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
index d7f4d1c893756..671ec84fb9416 100644
--- a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp
@@ -340,6 +340,9 @@ static std::pair<SDValue, SDNode *> lowerCallFromStatepointLoweringInfo(
// to grab the return value from the return register(s), or it can be a LOAD
// to load a value returned by reference via a stack slot.
+ if (CallEnd->getOpcode() == ISD::EH_LABEL)
+ CallEnd = CallEnd->getOperand(0).getNode();
+
bool HasDef = !SI.CLI.RetTy->isVoidTy();
if (HasDef) {
if (CallEnd->getOpcode() == ISD::LOAD)
diff --git a/llvm/test/CodeGen/X86/issue64826-switferror-eh.ll b/llvm/test/CodeGen/X86/issue64826-switferror-eh.ll
new file mode 100644
index 0000000000000..8ea003d7a56f5
--- /dev/null
+++ b/llvm/test/CodeGen/X86/issue64826-switferror-eh.ll
@@ -0,0 +1,75 @@
+; RUN: llc %s -filetype=obj -o - | llvm-readobj -r - | FileCheck %s --check-prefix=RELOC
+; RUN: llc %s -mtriple=x86_64-apple-macosx -o - | FileCheck %s --check-prefix=ASM
+
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx"
+
+define swiftcc void @rdar113994760() personality ptr @__gcc_personality_v0 {
+entry:
+ %swifterror = alloca swifterror ptr, align 8
+ invoke swiftcc void null(i64 0, ptr null, ptr swifterror %swifterror)
+ to label %.noexc unwind label %tsan_cleanup
+
+.noexc: ; preds = %entry
+ ret void
+
+tsan_cleanup: ; preds = %entry
+ %cleanup.lpad = landingpad { ptr, i32 }
+ cleanup
+ resume { ptr, i32 } zeroinitializer
+}
+
+declare i32 @__gcc_personality_v0(...)
+
+; RELOC-LABEL: Relocations [
+; RELOC-NEXT: Section __text {
+; RELOC-NEXT: 0x18 1 2 1 X86_64_RELOC_BRANCH 0 __Unwind_Resume
+; RELOC-NEXT: }
+; RELOC-NEXT: Section __eh_frame {
+; RELOC-NEXT: 0x13 1 2 1 X86_64_RELOC_GOT 0 ___gcc_personality_v0
+; RELOC-NEXT: }
+; RELOC-NEXT: ]
+
+; ASM-LABEL: rdar113994760:
+; ASM: ## %bb.0: ## %entry
+; ASM-NEXT: pushq %r12
+; ASM-NEXT: .cfi_def_cfa_offset 16
+; ASM-NEXT: subq $16, %rsp
+; ASM-NEXT: .cfi_def_cfa_offset 32
+; ASM-NEXT: .cfi_offset %r12, -16
+; ASM-NEXT: Ltmp0:
+; ASM-NEXT: xorl %eax, %eax
+; ASM-NEXT: xorl %edi, %edi
+; ASM-NEXT: xorl %esi, %esi
+; ASM-NEXT: callq *%rax
+; ASM-NEXT: Ltmp1:
+; ASM-NEXT: ## %bb.1: ## %.noexc
+; ASM-NEXT: addq $16, %rsp
+; ASM-NEXT: popq %r12
+; ASM-NEXT: retq
+; ASM-NEXT: LBB0_2: ## %tsan_cleanup
+; ASM-NEXT: Ltmp2:
+; ASM-NEXT: xorl %edi, %edi
+; ASM-NEXT: callq __Unwind_Resume
+; ASM-NEXT: Lfunc_end0:
+; ASM-NEXT: .cfi_endproc
+; ASM-NEXT: .section __TEXT,__gcc_except_tab
+; ASM-NEXT: .p2align 2, 0x0
+; ASM-NEXT: GCC_except_table0:
+; ASM-NEXT: Lexception0:
+; ASM-NEXT: .byte 255 ## @LPStart Encoding = omit
+; ASM-NEXT: .byte 255 ## @TType Encoding = omit
+; ASM-NEXT: .byte 1 ## Call site Encoding = uleb128
+; ASM-NEXT: .uleb128 Lcst_end0-Lcst_begin0
+; ASM-NEXT: Lcst_begin0:
+; ASM-NEXT: .uleb128 Ltmp0-Lfunc_begin0 ## >> Call Site 1 <<
+; ASM-NEXT: .uleb128 Ltmp1-Ltmp0 ## Call between Ltmp0 and Ltmp1
+; ASM-NEXT: .uleb128 Ltmp2-Lfunc_begin0 ## jumps to Ltmp2
+; ASM-NEXT: .byte 0 ## On action: cleanup
+; ASM-NEXT: .uleb128 Ltmp1-Lfunc_begin0 ## >> Call Site 2 <<
+; ASM-NEXT: .uleb128 Lfunc_end0-Ltmp1 ## Call between Ltmp1 and Lfunc_end0
+; ASM-NEXT: .byte 0 ## has no landing pad
+; ASM-NEXT: .byte 0 ## On action: cleanup
+; ASM-NEXT: Lcst_end0:
+; ASM-NEXT: .p2align 2, 0x0
+; ASM-NEXT: ## -- End function
\ No newline at end of file
|
arsenm
reviewed
May 31, 2024
arsenm
reviewed
May 31, 2024
lowerInvokeable wasn't updating the returned chain after emitting the lowerEndEH, which caused SwiftErrorVal-handling code to re-set the DAG root, and thus accidentally skip the EH_LABEL node it was supposed to have addeed. After fixing that, a few places needed to be adjusted that assume the specific shape of the returned DAG. Fixes: llvm#64826 Fixes: rdar://113994760
jroelofs
force-pushed
the
jroelofs/issue64826
branch
from
ada3592 to
84a43e0
Compare
jroelofs
commented
May 31, 2024
| ; ASM-NEXT: .byte 255 ## @LPStart Encoding = omit | ||
| ; ASM-NEXT: .byte 255 ## @TType Encoding = omit | ||
| ; ASM-NEXT: .byte 1 ## Call site Encoding = uleb128 | ||
| ; ASM-NEXT: .uleb128 Lcst_end0-Lcst_begin0 |
There was a problem hiding this comment.
I would have used UTC, but it drops these :(
|
ping |
arsenm
reviewed
Jun 12, 2024
| define swiftcc void @rdar113994760() personality ptr @__gcc_personality_v0 { | ||
| entry: | ||
| %swifterror = alloca swifterror ptr, align 8 | ||
| invoke swiftcc void null(i64 0, ptr null, ptr swifterror %swifterror) |
There was a problem hiding this comment.
avoiding the undefined call to null with something else would be slightly better
Comment on lines
+343
to
+344
| if (CallEnd->getOpcode() == ISD::EH_LABEL) | ||
| CallEnd = CallEnd->getOperand(0).getNode(); |
There was a problem hiding this comment.
There's no statepoint being lowered in any test?
There was a problem hiding this comment.
These cover that path via the assertion on line 355:
LLVM :: CodeGen/X86/statepoint-cmp-sunk-past-statepoint.ll
LLVM :: CodeGen/X86/statepoint-invoke.ll
LLVM :: CodeGen/X86/statepoint-ra.ll
LLVM :: CodeGen/X86/statepoint-spill-lowering.ll
LLVM :: CodeGen/X86/statepoint-stack-usage.ll
LLVM :: CodeGen/X86/statepoint-vreg-invoke.ll
I'll re-run UTC and see what changes.
arsenm
approved these changes
Jun 13, 2024
EthanLuisMcDonough
pushed a commit
to EthanLuisMcDonough/llvm-project
that referenced
this pull request
Aug 13, 2024
…60 (llvm#94004) lowerInvokeable wasn't updating the returned chain after emitting the lowerEndEH, which caused SwiftErrorVal-handling code to re-set the DAG root, and thus accidentally skip the EH_LABEL node it was supposed to have addeed. After fixing that, a few places needed to be adjusted that assume the specific shape of the returned DAG. Fixes: llvm#64826 Fixes: rdar://113994760
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
lowerInvokeable wasn't updating the returned chain after emitting the lowerEndEH, which caused SwiftErrorVal-handling code to re-set the DAG root, and thus accidentally skip the EH_LABEL node it was supposed to have addeed. After fixing that, a few places needed to be adjusted that assume the specific shape of the returned DAG.
Fixes: #64826
Fixes: rdar://113994760