[Clang] Support a user-defined __dso_handle

[asavonic]

Summary

This fixes PR49198: Wrong usage of __dso_handle in user code leads to a compiler
crash.

When Init is an address of the global itself, we need to track it
across RAUW. Otherwise the initializer can be destroyed if the global
is replaced.

[asavonic]

John, can you please review this patch?
I originally wanted to add a diagnostic to prevent the crash in CG (PR49198), but the case seems easy enough to support.

[rjmccall]

What's the crash exactly/ Is IRGen just unhappy about processing the user definition when we've generated a declaration with a different type? Because we're already supposed to be being cautious about this.

[asavonic]

In D101156#2724789, @rjmccall wrote:

What's the crash exactly/ Is IRGen just unhappy about processing the user definition when we've generated a declaration with a different type? Because we're already supposed to be being cautious about this.

Hi John. Sorry for the late reply.

class a {
public:
  ~a();
} b;
void *__dso_handle = &__dso_handle;

This code code causes a crash because the compiler first generates a
__dso_handle with i8 type:

@__dso_handle = external dso_local global i8

... and then reaches the explicit definition of it with a pointer type and a
pointer initializer, and tries to generate this instead:

@__dso_handle = hidden global i8* bitcast (i8** @__dso_handle to i8*), align 8

Since __dso_handle already exists in the module, it must be replaced because the
initializer has an incompatible type.

There is indeed some handling for this case in EmitGlobalVarDefinition, but it
does not work correctly when the initializer is a pointer to the variable
itself. Specifically, the Init variable in EmitGlobalVarDefinition becomes
stale:

p Init->dump()
<badref> = hidden global i8 <null operand!>

The current patch avoids this problem by having two distinct globals, but maybe
it is better to fix EmitGlobalVarDefinition instead? Is it supposed to handle
such case?

[asavonic]

I went ahead and implemented a fix for EmitGlobalVarDefinition. Please let me know what approach is preferable: Diff 339986 or Diff 347341.

[asavonic]

ping

[rjmccall]

clang/lib/CodeGen/CodeGenModule.cpp

4425 ↗

(On Diff #347341)

Can we actually do this bitcast for arbitrary initializers? This seems problematic.

I think we just need to hold Init in an llvm::TrackingVH across the call to GetAddrOfGlobalVar.

[asavonic]

• Used llvm::TrackingVH to track Init changes.

[asavonic]

clang/lib/CodeGen/CodeGenModule.cpp

4425 ↗

(On Diff #347341)

Thanks a lot! TrackingVH works great, with a few adjustments because it cannot be checked for nullptr.

[rjmccall]

Great, LGTM.

[Tuxist]

i have a problem i'am implement a libc in c++ i can't export now __dso_handle as weak so I can't link other shared libs against my libc.

clang/lib/CodeGen/ItaniumCXXABI.cpp

static void emitGlobalDtorWithCXAAtExit(CodeGenFunction &CGF,
 
   // 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");
+      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, AddrInt8PtrTy, handle->getType()};
   llvm::FunctionType *atexitTy =
     llvm::FunctionType::get(CGF.IntTy, paramTys, false);
 
for (const auto &I : DtorsUsingAtExit) {
     }
 
     CGF.FinishFunction();
     AddGlobalCtor(GlobalInitFn, Priority, nullptr);
   }
 
   if (getCXXABI().useSinitAndSterm())
     unregisterGlobalDtorsWithUnAtExit();
+
+  // Prefer user-defined __dso_handle over the compiler generated one
+  if (llvm::GlobalValue *GeneratedDSOHandle = GetGlobalValue("$__dso_handle")) {
+    llvm::GlobalValue *CustomDSOHandle = GetGlobalValue("__dso_handle");
+    if (!CustomDSOHandle) {
+      GeneratedDSOHandle->setName("__dso_handle");
+    } else {
+      if (!isa<llvm::GlobalVariable>(CustomDSOHandle))
+        llvm_unreachable("__dso_handle must be a global variable");
+
+      setDSOLocal(CustomDSOHandle);
+      CustomDSOHandle->setLinkage(llvm::GlobalValue::ExternalLinkage);
+      CustomDSOHandle->setVisibility(llvm::GlobalValue::HiddenVisibility);
+
+      llvm::Constant *DSOHandle = llvm::ConstantExpr::getBitCast(
+          CustomDSOHandle, GeneratedDSOHandle->getType());
+      GeneratedDSOHandle->replaceAllUsesWith(DSOHandle);
+      GeneratedDSOHandle->eraseFromParent();
+    }
+  }
 }
 
 /// Register a global destructor as best as we know how.
 void ItaniumCXXABI::registerGlobalDtor(CodeGenFunction &CGF, const VarDecl &D,
                                        llvm::FunctionCallee dtor,
                                        llvm::Constant *addr) {
   if (D.isNoDestroy(CGM.getContext()))
     return;

clang/test/CodeGenCXX/dso-handle-custom.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fexceptions %s -o - | FileCheck %s
+
+class A {
+public:
+  ~A();
+} a;
+
+// CHECK: @__dso_handle = hidden global i8* bitcast (i8** @__dso_handle to i8*), align 8
+// CHECK: define internal void @__cxx_global_var_init()
+// CHECK:   call i32 @__cxa_atexit({{.*}}, {{.*}}, i8* bitcast (i8** @__dso_handle to i8*))
+
+void *__dso_handle = &__dso_handle;
+
+void use(void *);
+void use_dso_handle() {
+  use(__dso_handle);
+}