[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

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

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/CodeGenModule.cpp

if (getLangOpts().OpenCL && ASTTy->isSamplerT())
     return;
 
   // If this is OpenMP device, check if it is legal to emit this global
   // normally.
   if (LangOpts.OpenMPIsDevice && OpenMPRuntime &&
       OpenMPRuntime->emitTargetGlobalVariable(D))
     return;
 
-  llvm::Constant *Init = nullptr;
+  llvm::TrackingVH<llvm::Constant> Init;
   bool NeedsGlobalCtor = false;
   bool NeedsGlobalDtor =
       D->needsDestruction(getContext()) == QualType::DK_cxx_destructor;
 
   const VarDecl *InitDecl;
   const Expr *InitExpr = D->getAnyInitializer(InitDecl);
 
   Optional<ConstantEmitter> emitter;
else if (!InitExpr) {
     // never attempt to emit a tentative definition if a real one
     // exists. A use may still exists, however, so we still may need
     // to do a RAUW.
     assert(!ASTTy->isIncompleteType() && "Unexpected incomplete type");
     Init = EmitNullConstant(D->getType());
   } else {
     initializedGlobalDecl = GlobalDecl(D);
     emitter.emplace(*this);
-    Init = emitter->tryEmitForInitializer(*InitDecl);
-
-    if (!Init) {
+    llvm::Constant *Initializer = emitter->tryEmitForInitializer(*InitDecl);
+    if (!Initializer) {
       QualType T = InitExpr->getType();
       if (D->getType()->isReferenceType())
         T = D->getType();
 
       if (getLangOpts().CPlusPlus) {
         Init = EmitNullConstant(T);
         NeedsGlobalCtor = true;
       } else {
         ErrorUnsupported(D, "static initializer");
         Init = llvm::UndefValue::get(getTypes().ConvertType(T));
       }
     } else {
+      Init = Initializer;
       // We don't need an initializer, so remove the entry for the delayed
       // initializer position (just in case this entry was delayed) if we
       // also don't need to register a destructor.
       if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
         DelayedCXXInitPosition.erase(D);
     }
   }
 
if (LangOpts.CUDAIsDevice) {
         GV->setExternallyInitialized(true);
     } else {
       getCUDARuntime().internalizeDeviceSideVar(D, Linkage);
     }
     getCUDARuntime().handleVarRegistration(D, *GV);
   }
 
   GV->setInitializer(Init);
   if (emitter)
     emitter->finalize(GV);
 
   // If it is safe to mark the global 'constant', do so now.
   GV->setConstant(!NeedsGlobalCtor && !NeedsGlobalDtor &&
                   isTypeConstant(D->getType(), true));
 
   // If it is in a read-only section, mark it 'constant'.
   if (const SectionAttr *SA = D->getAttr<SectionAttr>()) {

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

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fexceptions %s -o - | FileCheck %s --check-prefixes CHECK,CHECK-DEFAULT
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm -fexceptions %s -o - -DHIDDEN | FileCheck %s --check-prefixes CHECK,CHECK-HIDDEN
+
+class A {
+public:
+  ~A();
+} a;
+
+// CHECK-DEFAULT: @__dso_handle = global i8* bitcast (i8** @__dso_handle to i8*), align 8
+// CHECK-HIDDEN: @__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*))
+
+#ifdef HIDDEN
+void *__dso_handle __attribute__((__visibility__("hidden"))) = &__dso_handle;
+#else
+void *__dso_handle = &__dso_handle;
+#endif
+
+void use(void *);
+void use_dso_handle() {
+  use(__dso_handle);
+}