[OpenCL][PR41727] Prevent ICE on global dtors

[Anastasia]

Summary

Preserve address spaces of global objects while generating 'atexit' stub.

[rjmccall]

lib/CodeGen/CGDeclCXX.cpp

131

Should this code be conditional to OpenCL? And why does _cxa_atexit take a __global pointer instead of, say, a __generic one?

[Anastasia]

• Guard OpenCL specific IR generation under OpenCL lang mode

[Anastasia]

lib/CodeGen/CGDeclCXX.cpp

131

The only objects that are destructible globally in OpenCL are __global and __constant. However __constant isn't convertible to __generic. Therefore, I am adding __global directly to avoid extra conversion. I am not yet sure how to handle __constantthough and how much destructing objects in read-only memory segments would make sense anyway. I think I will address this separately.

[rjmccall]

lib/CodeGen/CGDeclCXX.cpp

131

The pointer argument to __cxa_atexit is just an arbitrary bit of context and doesn't have to actually be the address of a global. It's *convenient* to use the address of a global sometimes; e.g. you can use the global as the pointer and its destructor as the function, and then __cxa_atexit will just call the destructor for you without any additional code. But as far as the runtime is concerned, the pointer could be malloc'ed or something; we've never had a need to do that in the ABI, but it's good future-proofing to allow it.

So there are three ways to get a global destructor to destroy a variable in __constant:

• You can pass the pointer bitcast'ed as long as sizeof(__constant void*) <= sizeof(__cxa_atexit_context_pointer).
• You can ignore the argument and just materialize the address separately within the destructor function.
• You can allocate memory for a context and then store the pointer in that.

Obviously you should go with the one of the first two, but you should make sure your ABI doesn't preclude doing the latter in case it's useful for some future language feature. In other words, it doesn't really matter whether this argument is notionally in __global as long as that's wide enough to pass a more-or-less arbitrary pointer through.

[Anastasia]

lib/CodeGen/CGDeclCXX.cpp

131

Ok, I see. I guess option 1 would be fine since we can't setup pointer width per address space in clang currently. However, spec doesn't provide any clarifications in this regard.

So I guess using either __global or __generic for the pointer parameter would be fine... Or perhaps even leave it without any address space (i.e. _`_private`) and just addrspacecast from either __global or __constant. Do you have any preferences?

As for malloc I am not sure that will work for OpenCL since we don't allow mem allocation on the device. Unless you mean the memory is allocated on a host... then I am not sure how it should work.

[rjmccall]

lib/CodeGen/CGDeclCXX.cpp

131

Ok, I see. I guess option 1 would be fine since we can't setup pointer width per address space in clang currently.

Really? What's missing there? It looks to me like getPointerSize does take an address space.

So I guess using either global or generic for the pointer parameter would be fine... Or perhaps even leave it without any address space (i.e. _`_private`) and just addrspacecast from either global or constant. Do you have any preferences?

__private is likely to be a smaller address space, right? I would recommend using the fattest pointer that you want to actually support at runtime — you shouldn't go all the way to __generic if the target relies on eliminating that statically. If you want a target hook for the address space of the notional __cxa_atexit_context_pointer typedef, I think that would be reasonable.

As for malloc I am not sure that will work for OpenCL since we don't allow mem allocation on the device. Unless you mean the memory is allocated on a host... then I am not sure how it should work.

Well, maybe not actually heap-allocated. I just think you should design the ABI so that it's reasonably future-proof against taking any specific sort of reasonable pointer.

[Anastasia]

Added a target hook to query address spaces with the largest pointer width.

[rjmccall]

lib/CodeGen/TargetInfo.h

274 ↗

(On Diff #207574)

This target hook should just return the address space of the __cxa_atexit argument, not to claim anything specific about the relation between that address space and others. That is a global and permanent property of the target ABI. We should *advise* targets to use an address space that other major address spaces can be bitcast to, but ultimately that's just advisory.

This would be a good thing to include in a PR to the Itanium C++ ABI, but it would be reasonable to bundle all the address-space-related changes you need in one PR.

[Anastasia]

Changed API for target hook.

[Anastasia]

lib/CodeGen/TargetInfo.h

274 ↗

(On Diff #207574)

This would be a good thing to include in a PR to the Itanium C++ ABI, but it would be reasonable to bundle all the address-space-related changes you need in one PR.

I can try to do it but I would need some explanation of the process. :)

[rjmccall]

lib/CodeGen/CGDeclCXX.cpp

131

This cast only works if the address space is a subspace of the __cxa_atexit address space, right? Should we be checking that and emitting a diagnostic if that's not true? I think an IRGen-level diagnostic is fine here.

[Anastasia]

lib/CodeGen/CGDeclCXX.cpp

131

Then it would fail to compile for __constant.

You can pass the pointer bitcast'ed as long as sizeof(constant void*) <= sizeof(cxa_atexit_context_pointer).

Do you think I should leave a bitcast then? Not sure if something might assert in LLVM though if there is a bitcast between pointers to different address space... so I am confused...

[rjmccall]

lib/CodeGen/CGDeclCXX.cpp

131

I think LLVM doesn't allow direct bitcasts between different address spaces (to help eliminate obvious bugs), but you can do it with ptrtoint. For generality, though, you should emit a diagnostic if the __cxa_atexit pointer size is actually smaller than the target pointer.

Alternatively, I'm not sure we actually rely on this pointer for anything right now, so you might be able to just use null if the address spaces are different. That decision will eventually need to be able to affect how we generate the global destructor function, though.

[Anastasia]

Generate NULL for pointer param of __cxa_atexit on mismatching addr spaces

[Anastasia]

lib/CodeGen/CGDeclCXX.cpp

131

Thanks! I will go for NULL for now to prevent ICE and unblock some further work.

But I added a FIXME to make sure to come back to this. I would then either change the dtor generation of emit some meaningful non-NULL value as an argument.

[rjmccall]

If you're interested in working on this, great. I actually think there's zero reason to emit a non-null argument here on any target unless we're going to use the destructor as the function pointer — but we can do that on every Itanium target, so we should. So where we want to be is probably:

• use the complete-object destructor and the object pointer as its argument when destroying a (non-array) object of C++ class type
• otherwise use a custom function that materializes the object pointer on its own and uses a null pointer as the argument

And then the address-spaces tweak to that is that we use the second option when we either (1) the destructor isn't in the __cxa_atexit address space or (2) the object pointer can't be converted to that address space.

[rjmccall]

Current patch LGTM

lib/CodeGen/CGDeclCXX.cpp

 //===--- CGDeclCXX.cpp - Emit LLVM Code for C++ declarations --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This contains code dealing with code generation of C++ declarations
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeGenFunction.h"
 #include "CGCXXABI.h"
 #include "CGObjCRuntime.h"
 #include "CGOpenMPRuntime.h"
+#include "TargetInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/Support/Path.h"
 
 using namespace clang;
 using namespace CodeGen;
static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
   // generated elsewhere which uses atexit instead, and it takes the destructor
   // directly.
   bool UsingExternalHelper = !CGM.getCodeGenOpts().CXAAtExit;
   if (Record && (CanRegisterDestructor || UsingExternalHelper)) {
     assert(!Record->hasTrivialDestructor());
     CXXDestructorDecl *Dtor = Record->getDestructor();
 
     Func = CGM.getAddrAndTypeOfCXXStructor(GlobalDecl(Dtor, Dtor_Complete));
+    if (CGF.getContext().getLangOpts().OpenCL) {
+      // FIXME: A solution is needed for opencl_constant. We could create
+      // atexit_constant, but more generic solution would probably be to mangle
+      // address space?
+      auto DestTy = CGF.getTypes().ConvertType(Type)->getPointerTo(
+          CGM.getContext().getTargetAddressSpace(LangAS::opencl_global));
+      auto SrcAS = D.getType().getQualifiers().getAddressSpace();
+      if (LangAS::opencl_global == SrcAS)
+        Argument = llvm::ConstantExpr::getBitCast(Addr.getPointer(), DestTy);
+      else
+        Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast(
+            CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy);
+    } else {
       Argument = llvm::ConstantExpr::getBitCast(
           Addr.getPointer(), CGF.getTypes().ConvertType(Type)->getPointerTo());
-
+    }
   // Otherwise, the standard logic requires a helper function.
   } else {
     Func = CodeGenFunction(CGM)
            .generateDestroyHelper(Addr, Type, CGF.getDestroyer(DtorKind),
                                   CGF.needsEHCleanup(DtorKind), &D);
     Argument = llvm::Constant::getNullValue(CGF.Int8PtrTy);
   }
 

lib/CodeGen/CodeGenModule.cpp

llvm::Constant *CodeGenModule::GetAddrOfGlobalVar(const VarDecl *D,
   return GetOrCreateLLVMGlobal(MangledName, PTy, D, IsForDefinition);
 }
 
 /// CreateRuntimeVariable - Create a new runtime global variable with the
 /// specified type and name.
 llvm::Constant *
 CodeGenModule::CreateRuntimeVariable(llvm::Type *Ty,
                                      StringRef Name) {
-  auto *Ret =
-      GetOrCreateLLVMGlobal(Name, llvm::PointerType::getUnqual(Ty), nullptr);
+  auto PtrTy =
+      getContext().getLangOpts().OpenCL
+          ? llvm::PointerType::get(
+                Ty, getContext().getTargetAddressSpace(LangAS::opencl_global))
+          : llvm::PointerType::getUnqual(Ty);
+  auto *Ret = GetOrCreateLLVMGlobal(Name, PtrTy, nullptr);
   setDSOLocal(cast<llvm::GlobalValue>(Ret->stripPointerCasts()));
   return Ret;
 }
 
 void CodeGenModule::EmitTentativeDefinition(const VarDecl *D) {
   assert(!D->getInit() && "Cannot emit definite definitions here!");
 
   StringRef MangledName = getMangledName(D);

lib/CodeGen/ItaniumCXXABI.cpp

static void emitGlobalDtorWithCXAAtExit(CodeGenFunction &CGF,
   }
 
   // We're assuming that the destructor function is something we can
   // reasonably call with the default CC.  Go ahead and cast it to the
   // right prototype.
   llvm::Type *dtorTy =
     llvm::FunctionType::get(CGF.VoidTy, CGF.Int8PtrTy, false)->getPointerTo();
 
+  // Preserve address space of addr.
+  auto AddrAS = addr ? addr->getType()->getPointerAddressSpace() : 0;
+  auto AddrInt8PtrTy =
+      AddrAS ? CGF.Int8Ty->getPointerTo(AddrAS) : CGF.Int8PtrTy;
+
+  // Create a variable that binds the atexit to this shared object.
+  llvm::Constant *handle =
+      CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
+  auto *GV = cast<llvm::GlobalValue>(handle->stripPointerCasts());
+  GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
   // extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
-  llvm::Type *paramTys[] = { dtorTy, CGF.Int8PtrTy, CGF.Int8PtrTy };
+  llvm::Type *paramTys[] = {dtorTy, AddrInt8PtrTy, handle->getType()};
   llvm::FunctionType *atexitTy =
     llvm::FunctionType::get(CGF.IntTy, paramTys, false);
 
   // Fetch the actual function.
   llvm::FunctionCallee atexit = CGF.CGM.CreateRuntimeFunction(atexitTy, Name);
   if (llvm::Function *fn = dyn_cast<llvm::Function>(atexit.getCallee()))
     fn->setDoesNotThrow();
 
-  // Create a variable that binds the atexit to this shared object.
-  llvm::Constant *handle =
-      CGF.CGM.CreateRuntimeVariable(CGF.Int8Ty, "__dso_handle");
-  auto *GV = cast<llvm::GlobalValue>(handle->stripPointerCasts());
-  GV->setVisibility(llvm::GlobalValue::HiddenVisibility);
-
   if (!addr)
     // addr is null when we are trying to register a dtor annotated with
     // __attribute__((destructor)) in a constructor function. Using null here is
     // okay because this argument is just passed back to the destructor
     // function.
     addr = llvm::Constant::getNullValue(CGF.Int8PtrTy);
 
   llvm::Value *args[] = {llvm::ConstantExpr::getBitCast(
                              cast<llvm::Constant>(dtor.getCallee()), dtorTy),
-                         llvm::ConstantExpr::getBitCast(addr, CGF.Int8PtrTy),
+                         llvm::ConstantExpr::getBitCast(addr, AddrInt8PtrTy),
                          handle};
   CGF.EmitNounwindRuntimeCall(atexit, args);
 }
 
 void CodeGenModule::registerGlobalDtorsWithAtExit() {
   for (const auto I : DtorsUsingAtExit) {
     int Priority = I.first;
     const llvm::TinyPtrVector<llvm::Function *> &Dtors = I.second;

test/CodeGenOpenCLCXX/atexit.cl

+//RUN: %clang_cc1 %s -triple spir -cl-std=c++ -emit-llvm -O0 -o - | FileCheck %s
+
+struct S {
+  ~S(){};
+};
+S s;
+
+//CHECK-LABEL: define internal void @__cxx_global_var_init()
+// call i32 @__cxa_atexit(void (i8*)* bitcast (void (%struct.S addrspace(4)*)* @_ZNU3AS41SD1Ev to void (i8*)*), i8 addrspace(1)* getelementptr inbounds (%struct.S, %struct.S addrspace(1)* @s, i32 0, i32 0), i8 addrspace(1)* @__dso_handle)
+
+//declare i32 @__cxa_atexit(void (i8*)*, i8 addrspace(1)*, i8 addrspace(1)*)