Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference.

[jyknight]

Summary

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

[jyknight]

(Note: if folks agree with this direction, I'll file a new bug for the newly broken behavior -- even though we probably cannot actually fix it -- and link it from the commit.)

[rjmccall]

Should we just diagnose this and report an error that catching pointers by non-const reference is unsupported on Itanium? We don't support it properly for class pointers anyway.

clang/lib/CodeGen/ItaniumCXXABI.cpp

if (isa<ReferenceType>(CatchType)) {
     bool EndCatchMightThrow = CaughtType->isRecordType();
 
     // __cxa_begin_catch returns the adjusted object pointer.
     llvm::Value *AdjustedExn = CallBeginCatch(CGF, Exn, EndCatchMightThrow);
 
     // 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();
-
-      // 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 =
           CGF.CreateTempAlloca(PtrTy, CGF.getPointerAlign(), "exn.byref.tmp");
-        llvm::Value *Casted = CGF.Builder.CreateBitCast(AdjustedExn, PtrTy);
-        CGF.Builder.CreateStore(Casted, ExnPtrTmp);
 
-        // Bind the reference to the temporary.
-        AdjustedExn = ExnPtrTmp.getPointer();
-      }
-    }
 
     llvm::Value *ExnCast =
       CGF.Builder.CreateBitCast(AdjustedExn, LLVMCatchTy, "exn.byref");
     CGF.Builder.CreateStore(ExnCast, ParamAddr);
     return;
   }
 
   // Scalars and complexes.

clang/lib/CodeGen/TargetInfo.h

public:
 
   /// Any further codegen related checks that need to be done on a function call
   /// in a target specific manner.
   virtual void checkFunctionCallABI(CodeGenModule &CGM, SourceLocation CallLoc,
                                     const FunctionDecl *Caller,
                                     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:
   ///   - that have 64-bit pointers, or
   ///   - that cannot address through registers larger than pointers, or
   ///   - that implicitly ignore/truncate the top bits when addressing
   ///     through such registers.
   virtual bool extendPointerWithSExt() const { return false; }

clang/lib/CodeGen/TargetInfo.cpp

static Address emitMergePHI(CodeGenFunction &CGF,
   PHI->addIncoming(Addr1.getPointer(), Block1);
   PHI->addIncoming(Addr2.getPointer(), Block2);
   CharUnits Align = std::min(Addr1.getAlignment(), Addr2.getAlignment());
   return Address(PHI, Addr1.getElementType(), Align);
 }
 
 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:
   //   x86_stdcall
   //   MIPS
   // For everything else, we just prefer false unless we opt out.
   return false;
 }
bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                llvm::Value *Address) const override {
     llvm::Value *Four8 = llvm::ConstantInt::get(CGF.Int8Ty, 4);
 
     // 0-15 are the 16 integer registers.
     AssignToArrayRange(CGF.Builder, Address, Four8, 0, 15);
     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())
       return;
     const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
     if (!FD)
       return;
     auto *Fn = cast<llvm::Function>(GV);
public:
   ABIArgInfo classifyArgumentType(QualType RetTy, uint64_t &Offset) const;
   void computeInfo(CGFunctionInfo &FI) const override;
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                     QualType Ty) const override;
   ABIArgInfo extendType(QualType Ty) const;
 };
 
 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;
   }
 
   void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
                            CodeGen::CodeGenModule &CGM) const override {
     const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
 
     Fn->addFnAttr("interrupt", Kind);
 
   }
 
   bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                llvm::Value *Address) const override;
 
-  unsigned getSizeOfUnwindException() const override {
-    return SizeOfUnwindException;
-  }
 };
 }
 
 void MipsABIInfo::CoerceToIntArgs(
     uint64_t TySize, SmallVectorImpl<llvm::Type *> &ArgList) const {
   llvm::IntegerType *IntTy =
     llvm::IntegerType::get(getVMContext(), MinABIStackAlignInBytes * 8);
 

clang/test/CodeGenCXX/eh.cpp

void foo() {
     // CHECK:      call i8* @__cxa_begin_catch
     // CHECK-NEXT: invoke void @__cxa_end_catch()
     }
 
     // CHECK: call void @_ZN6test101AD1Ev(
   }
 }
 
-// __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()
   void bar() {
     try {
       // CHECK:      [[EXNSLOT:%.*]] = alloca i8*
       // CHECK-NEXT: [[SELECTORSLOT:%.*]] = alloca i32
       // CHECK-NEXT: [[P:%.*]] = alloca [[A:%.*]]**,

clang/test/CodeGenCXX/sizeof-unwind-exception.cpp

-// 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 }