jyknight created this revision.
jyknight added reviewers: rjmccall, nikic.
Herald added a project: All.
jyknight requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Pointers will now always be caught by-value.
The workaround was originally added in commit
5add20cefac7d1b06e7892ce0d97767a23db4e1a. (Note, that commit also made
the codegen for catching a pointer by reference work _at all_. I am
not reverting that part of it.)
The intent was to enable `catch (int*&ptr) { ptr = 0; throw; }`
to correctly propagate the new value of ptr to a higher catch block.
Unfortunately, the implementation was always rather suspect, as it
hardcoded byte offsets of EH structs, and accessed data directly,
assuming it knew the layout. Unfortunately, as recently reported, this
is now known to break std::rethrow_exception, as that doesn't use the
expected layout.
Because it does not appear possible to solve the issue in a principled
way without modifying the Itanium ABI, this change simply removes the
hack.
While we therefore fix the bug referenced, this also introduces the
bug whereby modifying a pointer caught by reference is no longer
visible to other catch blocks.
Introduces FIXME file new bug and link here FIXME
Fixes https://github.com/llvm/llvm-project/issues/55340
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D127545
Files:
clang/lib/CodeGen/ItaniumCXXABI.cpp
clang/lib/CodeGen/TargetInfo.cpp
clang/lib/CodeGen/TargetInfo.h
clang/test/CodeGenCXX/eh.cpp
clang/test/CodeGenCXX/sizeof-unwind-exception.cpp
Index: clang/test/CodeGenCXX/sizeof-unwind-exception.cpp
===================================================================
--- clang/test/CodeGenCXX/sizeof-unwind-exception.cpp
+++ /dev/null
@@ -1,33 +0,0 @@
-// RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions %s -O2 -o - | FileCheck %s --check-prefix=X86-64
-// RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions %s -O2 -o - | FileCheck %s --check-prefix=X86-32
-// RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-apple-darwin10 -emit-llvm -fcxx-exceptions -fexceptions %s -O2 -o - | FileCheck %s --check-prefix=ARM-DARWIN
-// RUN: %clang_cc1 -no-opaque-pointers -triple arm-unknown-gnueabi -emit-llvm -fcxx-exceptions -fexceptions %s -O2 -o - | FileCheck %s --check-prefix=ARM-EABI
-// RUN: %clang_cc1 -no-opaque-pointers -triple mipsel-unknown-unknown -emit-llvm -fcxx-exceptions -fexceptions %s -O2 -o - | FileCheck %s --check-prefix=MIPS
-
-void foo();
-void test() {
- try {
- foo();
- } catch (int *&i) {
- *i = 5;
- }
-}
-
-// PR10789: different platforms have different sizes for struct UnwindException.
-
-// X86-64: [[T0:%.*]] = tail call i8* @__cxa_begin_catch(i8* [[EXN:%.*]]) [[NUW:#[0-9]+]]
-// X86-64-NEXT: [[T1:%.*]] = getelementptr i8, i8* [[EXN]], i64 32
-// X86-32: [[T0:%.*]] = tail call i8* @__cxa_begin_catch(i8* [[EXN:%.*]]) [[NUW:#[0-9]+]]
-// X86-32-NEXT: [[T1:%.*]] = getelementptr i8, i8* [[EXN]], i64 32
-// ARM-DARWIN: [[T0:%.*]] = tail call i8* @__cxa_begin_catch(i8* [[EXN:%.*]]) [[NUW:#[0-9]+]]
-// ARM-DARWIN-NEXT: [[T1:%.*]] = getelementptr i8, i8* [[EXN]], i64 32
-// ARM-EABI: [[T0:%.*]] = tail call i8* @__cxa_begin_catch(i8* [[EXN:%.*]]) [[NUW:#[0-9]+]]
-// ARM-EABI-NEXT: [[T1:%.*]] = getelementptr i8, i8* [[EXN]], i32 88
-// MIPS: [[T0:%.*]] = tail call i8* @__cxa_begin_catch(i8* [[EXN:%.*]]) [[NUW:#[0-9]+]]
-// MIPS-NEXT: [[T1:%.*]] = getelementptr i8, i8* [[EXN]], i32 24
-
-// X86-64: attributes [[NUW]] = { nounwind }
-// X86-32: attributes [[NUW]] = { nounwind }
-// ARM-DARWIN: attributes [[NUW]] = { nounwind }
-// ARM-EABI: attributes [[NUW]] = { nounwind }
-// MIPS: attributes [[NUW]] = { nounwind }
Index: clang/test/CodeGenCXX/eh.cpp
===================================================================
--- clang/test/CodeGenCXX/eh.cpp
+++ clang/test/CodeGenCXX/eh.cpp
@@ -243,26 +243,11 @@
}
}
-// __cxa_begin_catch returns pointers by value, even when catching by reference
-// <rdar://problem/8212123>
+// __cxa_begin_catch returns pointers by value, even when catching by reference.
+// This violates the langauge spec, but is not solveable without Itanium EH ABI
+// changes. See https://github.com/llvm/llvm-project/issues/55340
namespace test11 {
void opaque();
-
- // CHECK-LABEL: define{{.*}} void @_ZN6test113fooEv()
- void foo() {
- try {
- // CHECK: invoke void @_ZN6test116opaqueEv()
- opaque();
- } catch (int**&p) {
- // CHECK: [[EXN:%.*]] = load i8*, i8**
- // CHECK-NEXT: call i8* @__cxa_begin_catch(i8* [[EXN]]) [[NUW]]
- // CHECK-NEXT: [[ADJ1:%.*]] = getelementptr i8, i8* [[EXN]], i32 32
- // CHECK-NEXT: [[ADJ2:%.*]] = bitcast i8* [[ADJ1]] to i32***
- // CHECK-NEXT: store i32*** [[ADJ2]], i32**** [[P:%.*]]
- // CHECK-NEXT: call void @__cxa_end_catch() [[NUW]]
- }
- }
-
struct A {};
// CHECK-LABEL: define{{.*}} void @_ZN6test113barEv()
Index: clang/lib/CodeGen/TargetInfo.h
===================================================================
--- clang/lib/CodeGen/TargetInfo.h
+++ clang/lib/CodeGen/TargetInfo.h
@@ -70,16 +70,6 @@
const FunctionDecl *Callee,
const CallArgList &Args) const {}
- /// Determines the size of struct _Unwind_Exception on this platform,
- /// in 8-bit units. The Itanium ABI defines this as:
- /// struct _Unwind_Exception {
- /// uint64 exception_class;
- /// _Unwind_Exception_Cleanup_Fn exception_cleanup;
- /// uint64 private_1;
- /// uint64 private_2;
- /// };
- virtual unsigned getSizeOfUnwindException() const;
-
/// Controls whether __builtin_extend_pointer should sign-extend
/// pointers to uint64_t or zero-extend them (the default). Has
/// no effect for targets:
Index: clang/lib/CodeGen/TargetInfo.cpp
===================================================================
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -440,18 +440,6 @@
TargetCodeGenInfo::~TargetCodeGenInfo() = default;
-// If someone can figure out a general rule for this, that would be great.
-// It's probably just doomed to be platform-dependent, though.
-unsigned TargetCodeGenInfo::getSizeOfUnwindException() const {
- // Verified for:
- // x86-64 FreeBSD, Linux, Darwin
- // x86-32 FreeBSD, Linux, Darwin
- // PowerPC Linux, Darwin
- // ARM Darwin (*not* EABI)
- // AArch64 Linux
- return 32;
-}
-
bool TargetCodeGenInfo::isNoProtoCallVariadic(const CallArgList &args,
const FunctionNoProtoType *fnType) const {
// The following conventions are known to require this to be false:
@@ -6388,11 +6376,6 @@
return false;
}
- unsigned getSizeOfUnwindException() const override {
- if (getABIInfo().isEABI()) return 88;
- return TargetCodeGenInfo::getSizeOfUnwindException();
- }
-
void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
CodeGen::CodeGenModule &CGM) const override {
if (GV->isDeclaration())
@@ -7859,11 +7842,9 @@
};
class MIPSTargetCodeGenInfo : public TargetCodeGenInfo {
- unsigned SizeOfUnwindException;
public:
MIPSTargetCodeGenInfo(CodeGenTypes &CGT, bool IsO32)
- : TargetCodeGenInfo(std::make_unique<MipsABIInfo>(CGT, IsO32)),
- SizeOfUnwindException(IsO32 ? 24 : 32) {}
+ : TargetCodeGenInfo(std::make_unique<MipsABIInfo>(CGT, IsO32)) {}
int getDwarfEHStackPointer(CodeGen::CodeGenModule &CGM) const override {
return 29;
@@ -7920,9 +7901,6 @@
bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
llvm::Value *Address) const override;
- unsigned getSizeOfUnwindException() const override {
- return SizeOfUnwindException;
- }
};
}
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===================================================================
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -4442,47 +4442,34 @@
// We have no way to tell the personality function that we're
// catching by reference, so if we're catching a pointer,
// __cxa_begin_catch will actually return that pointer by value.
+ //
+ // This violates the language semantics in two ways:
+ //
+ // 1. If you have `struct A : public B {};`, and throw a B*, that should not
+ // be caught by `catch (A*& ptrref)` (though it would via `catch (A*
+ // ptr)`). But, it will be.
+ //
+ // 2. Assigning to the pointer through the reference ought to be visible to
+ // other catch blocks if we rethrow the exception after the mutation. But it
+ // won't be.
+ //
+ // Unfortunately, there's nothing we can actually do about that within the
+ // current (long-standing) Itanium EH ABI. GCC has the same issue.
+ // So, we'll just dump the by-value adjusted pointer into an
+ // alloca, and have the reference point to that. _shrug_.
if (const PointerType *PT = dyn_cast<PointerType>(CaughtType)) {
QualType PointeeType = PT->getPointeeType();
+ // Pull the pointer for the reference type off.
+ llvm::Type *PtrTy = CGF.ConvertTypeForMem(CaughtType);
- // When catching by reference, generally we should just ignore
- // this by-value pointer and use the exception object instead.
- if (!PointeeType->isRecordType()) {
-
- // Exn points to the struct _Unwind_Exception header, which
- // we have to skip past in order to reach the exception data.
- unsigned HeaderSize =
- CGF.CGM.getTargetCodeGenInfo().getSizeOfUnwindException();
- AdjustedExn =
- CGF.Builder.CreateConstGEP1_32(CGF.Int8Ty, Exn, HeaderSize);
-
- // However, if we're catching a pointer-to-record type that won't
- // work, because the personality function might have adjusted
- // the pointer. There's actually no way for us to fully satisfy
- // the language/ABI contract here: we can't use Exn because it
- // might have the wrong adjustment, but we can't use the by-value
- // pointer because it's off by a level of abstraction.
- //
- // The current solution is to dump the adjusted pointer into an
- // alloca, which breaks language semantics (because changing the
- // pointer doesn't change the exception) but at least works.
- // The better solution would be to filter out non-exact matches
- // and rethrow them, but this is tricky because the rethrow
- // really needs to be catchable by other sites at this landing
- // pad. The best solution is to fix the personality function.
- } else {
- // Pull the pointer for the reference type off.
- llvm::Type *PtrTy = CGF.ConvertTypeForMem(CaughtType);
-
- // Create the temporary and write the adjusted pointer into it.
- Address ExnPtrTmp =
+ // Create the temporary and write the adjusted pointer into it.
+ Address ExnPtrTmp =
CGF.CreateTempAlloca(PtrTy, CGF.getPointerAlign(), "exn.byref.tmp");
- llvm::Value *Casted = CGF.Builder.CreateBitCast(AdjustedExn, PtrTy);
- CGF.Builder.CreateStore(Casted, ExnPtrTmp);
+ llvm::Value *Casted = CGF.Builder.CreateBitCast(AdjustedExn, PtrTy);
+ CGF.Builder.CreateStore(Casted, ExnPtrTmp);
- // Bind the reference to the temporary.
- AdjustedExn = ExnPtrTmp.getPointer();
- }
+ // Bind the reference to the temporary.
+ AdjustedExn = ExnPtrTmp.getPointer();
}
llvm::Value *ExnCast =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
