[CodeGen] don't emit addrsig symbol if it's used only by metadata

[zequanwu]

Summary

Value only used by metadata can be removed from .addrsig table.
This solves the undefined symbol error when enabling addrsig table on COFF LTO.

[rnk]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1853

I see, so, the GV is used by a constant, which is then used by metadata. Metadata uses do not appear in the use list.

I think this condition is slightly incorrect, though, the bitcast constant expression could be used by metadata *and* a real Value. So, consider your test case, but then add in a real instruction that uses the same bitcast. In this case, we should still put GV into the addrsig table.

One refinement would be to check that each user is a Constant with no uses, instead of checking if it is used by metadata. This could be:

if (isa<Constant>(U) && U->use_empty())

However, this will not handle nested constant expressions. Consider a series of getelementptr and bitcast constant expressions. To be fully general, you need a worklist to check that all transitive users of GV are non-GlobalValue Constants that are only used by other constants.

[zequanwu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1853

Yea, I see the problem.

if (isa<Constant>(U) && U->use_empty())

I understand this, but confused by below.

check that all transitive users of GV are non-GlobalValue Constants that are only used by other constants.

IIUC, if all transitive users of the GV are non-GlobalValue constants that are not transitively used by any non-GlobalValue constants, the GV is not address significant. For example, in the test case below, @a1 and @i1 are not address significant, right?

[rnk]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1853

In the test below, I see two Instructions that use @a1 and @i1, so I think they are address significant.

I mentioned GlobalValues because they are also subclasses of Constant, but if a GlobalVariable uses a Function, for example, that is a real use and we should consider the function address significant. Consider:

int someFunc(int x) { return x + 1; }
void *p = (void *)&someFunc;

The only user of someFunc will be a bitcast constant, and then the only use of that bitcast will be p.

[zequanwu]

Address comment.

[rnk]

Looks good, but please factor it out for readability.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1850

Please factor this out into its own function and give it some comments. I can suggest a name of hasTransitiveNonMetadataUse, or maybe just hasNonMetadataUse.

[zequanwu]

Refactor.

llvm/include/llvm/IR/Value.h

#include "llvm/IR/Value.def"
   }
 
   /// Return true if there is a value handle associated with this value.
   bool hasValueHandle() const { return HasValueHandle; }
 
   /// Return true if there is metadata referencing this value.
   bool isUsedByMetadata() const { return IsUsedByMD; }
 
+  // Return true if this value is only transitively referenced by metadata.
+  bool isTransitiveUsedByMetadataOnly() const;
+
 protected:
   /// Get the current metadata attachments for the given kind, if any.
   ///
   /// These functions require that the value have at most a single attachment
   /// of the given kind, and return \c nullptr if such an attachment is missing.
   /// @{
   MDNode *getMetadata(unsigned KindID) const;
   MDNode *getMetadata(StringRef Kind) const;

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

bool AsmPrinter::doFinalization(Module &M) {
   Function *InitTrampolineIntrinsic = M.getFunction("llvm.init.trampoline");
   if (!InitTrampolineIntrinsic || InitTrampolineIntrinsic->use_empty())
     if (MCSection *S = MAI->getNonexecutableStackSection(OutContext))
       OutStreamer->SwitchSection(S);
 
   if (TM.Options.EmitAddrsig) {
     // Emit address-significance attributes for all globals.
     OutStreamer->emitAddrsig();
-    for (const GlobalValue &GV : M.global_values())
-      if (!GV.use_empty() && !GV.isThreadLocal() &&
-          !GV.hasDLLImportStorageClass() && !GV.getName().startswith("llvm.") &&
-          !GV.hasAtLeastLocalUnnamedAddr())
+    for (const GlobalValue &GV : M.global_values()) {
+      if (!GV.use_empty() && !GV.isTransitiveUsedByMetadataOnly() &&
+          !GV.isThreadLocal() && !GV.hasDLLImportStorageClass() &&
+          !GV.getName().startswith("llvm.") && !GV.hasAtLeastLocalUnnamedAddr())
         OutStreamer->emitAddrsigSym(getSymbol(&GV));
     }
+  }
 
   // Emit symbol partition specifications (ELF only).
   if (TM.getTargetTriple().isOSBinFormatELF()) {
     unsigned UniqueID = 0;
     for (const GlobalValue &GV : M.global_values()) {
       if (!GV.hasPartition() || GV.isDeclarationForLinker() ||
           GV.getVisibility() != GlobalValue::DefaultVisibility)
         continue;

llvm/lib/IR/Value.cpp

bool Value::isSwiftError() const {
   if (Arg)
     return Arg->hasSwiftErrorAttr();
   auto *Alloca = dyn_cast<AllocaInst>(this);
   if (!Alloca)
     return false;
   return Alloca->isSwiftError();
 }
 
+bool Value::isTransitiveUsedByMetadataOnly() const {
+  if (use_empty())
+    return false;
+  llvm::SmallVector<const User *, 32> WorkList;
+  llvm::SmallPtrSet<const User *, 32> Visited;
+  WorkList.insert(WorkList.begin(), user_begin(), user_end());
+  while (!WorkList.empty()) {
+    const User *U = WorkList.back();
+    WorkList.pop_back();
+    Visited.insert(U);
+    // If it is transitively used by a global value or a non-constant value,
+    // it's obviously not only used by metadata.
+    if (!isa<Constant>(U) || isa<GlobalValue>(U))
+      return false;
+    for (const User *UU : U->users())
+      if (!Visited.count(UU))
+        WorkList.push_back(UU);
+  }
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 //                             ValueHandleBase Class
 //===----------------------------------------------------------------------===//
 
 void ValueHandleBase::AddToExistingUseList(ValueHandleBase **List) {
   assert(List && "Handle list is null?");
 
   // Splice ourselves into the list.

llvm/test/CodeGen/X86/addrsig.ll

define void @f1() {
   %g2 = bitcast i32* @g2 to i8*
   %g3 = bitcast i32* @g3 to i8*
   %dllimport = bitcast i32* @dllimport to i8*
   %tls = bitcast i32* @tls to i8*
   %a1 = bitcast i32* @a1 to i8*
   %a2 = bitcast i32* @a2 to i8*
   %i1 = bitcast void()* @i1 to i8*
   %i2 = bitcast void()* @i2 to i8*
+  call void @llvm.dbg.value(metadata i8* bitcast (void()* @metadata_f1 to i8*), metadata !5, metadata !DIExpression()), !dbg !7
+  call void @llvm.dbg.value(metadata i8* bitcast (void()* @metadata_f2 to i8*), metadata !5, metadata !DIExpression()), !dbg !7
+  call void @f4(i8* bitcast (void()* @metadata_f2 to i8*))
   unreachable
 }
 
+declare void @f4(i8*) unnamed_addr
+
+; CHECK-NOT: .addrsig_sym metadata_f1
+declare void @metadata_f1()
+
+; CHECK: .addrsig_sym metadata_f2
+declare void @metadata_f2()
+
 ; CHECK-NOT: .addrsig_sym f2
 define internal void @f2() local_unnamed_addr {
   unreachable
 }
 
 ; CHECK-NOT: .addrsig_sym f3
 declare void @f3() unnamed_addr
 

 @a1 = alias i32, i32* @g1
 ; CHECK-NOT: .addrsig_sym a2
 @a2 = internal local_unnamed_addr alias i32, i32* @g2
 
 ; CHECK: .addrsig_sym i1
 @i1 = ifunc void(), void()* @f1
 ; CHECK-NOT: .addrsig_sym i2
 @i2 = internal local_unnamed_addr ifunc void(), void()* @f2
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug)
+!1 = !DIFile(filename: "a", directory: "")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !0)
+!4 = !DILocation(line: 0, scope: !3)
+!5 = !DILocalVariable(scope: !6)
+!6 = distinct !DISubprogram(scope: null, isLocal: false, isDefinition: true, isOptimized: false, unit: !0)
+!7 = !DILocation(line: 0, scope: !6, inlinedAt: !4)