CodeGen: Add two more conditions for adding symbols to the address-significance table.

[pcc]

Summary

Firstly, require the symbol to be used within the module. If a
symbol is unused within a module, then by definition it cannot be
address-significant within that module. This condition is useful on all
platforms because it could make symbol tables smaller -- without this
change, emitting an address-significance table could cause otherwise
unused undefined symbols to be added to the object file.

But this change is necessary with COFF specifically in order to
preserve the property that an unreferenced undefined symbol in an IR
module does not result in a link failure. This is already the case for
ELF because ELF linkers only reject links with unresolved symbols if
there is a relocation to that symbol, but COFF linkers require all
undefined symbols to be resolved regardless of relocations. So if
a module contains an unreferenced undefined symbol, we need to make
sure not to add it to the address-significance table (and thus the
symbol table) in case it doesn't end up resolved at link time.

Secondly, do not add dllimport symbols to the table. These symbols
won't be able to be resolved because their definitions live in another
module and are accessed via the IAT, and the address-significance
table has no effect on other modules anyway. It wouldn't make sense
to add the IAT entry symbol to the address-significance table either
because the IAT entry isn't address-significant -- the generated code
never takes its address.

[rnk]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1546 ↗

(On Diff #162303)

Dllimport things aren't actually always imported, sometimes they can be resolved to symbols in the same image. MSVC will warn for this by default, but in my experience developers disable the warning, especially if they don't use Windows for development.

Can we end up with an incomplete table if we apply dllimport, resolve the symbol locally, and take the symbols address in that TU?

[rnk]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1546 ↗

(On Diff #162303)

To answer my own question, maybe we just don't worry about doing extra ICF on symbols like that. Basically, if you have code like this, you see this warning, and then disable it, and *then* you rely on the symbol's address being unique, you are out of luck.

These symbols tend to be the result of template instantiation anyway, which often have non-unique addresses with -fvisibility-inlines-hidden is set, which is pretty common.

[smeenai]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1546 ↗

(On Diff #162303)

Even if a dllimported symbol ends up being found in the same image, you'll still end up going through the __imp_ thunk, so the argument for the IAT entry never being address-significant should still apply?

[pcc]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1546 ↗

(On Diff #162303)

Either that or we could pessimise the case where code uses a locally-imported symbol. That is, we should set KeepUnique on a symbol's section when we create a DefinedLocalImport for it.

[rnk]

lgtm

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1546 ↗

(On Diff #162303)

@smeenai I'm not sure exactly how it works, but if the code is taking the address of a locally imported symbol, I think it will load the correct, local address of the symbol.

@pcc I'm not sure it's worth the complexity. I probably shouldn't have brought up this corner case. Thinking more on it, I doubt any user will ever hit it.

llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (const auto *LU = M.getNamedGlobal("llvm.used")) {
       }
     }
   }
 
   if (TM.Options.EmitAddrsig) {
     // Emit address-significance attributes for all globals.
     OutStreamer->EmitAddrsig();
     for (const GlobalValue &GV : M.global_values())
-      if (!GV.isThreadLocal() && !GV.getName().startswith("llvm.") &&
+      if (!GV.use_empty() && !GV.isThreadLocal() &&
+          !GV.hasDLLImportStorageClass() && !GV.getName().startswith("llvm.") &&
           !GV.hasAtLeastLocalUnnamedAddr())
         OutStreamer->EmitAddrsigSym(getSymbol(&GV));
   }
 
   // Allow the target to emit any magic that it wants at the end of the file,
   // after everything else has gone out.
   EmitEndOfAsmFile(M);
 

llvm/trunk/test/CodeGen/X86/addrsig.ll

 ; RUN: llc < %s -mtriple=x86_64-unknown-linux | FileCheck --check-prefix=NO-ADDRSIG %s
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux -addrsig | FileCheck %s
 
 ; NO-ADDRSIG-NOT: .addrsig
 
 ; CHECK: .addrsig
 
 ; CHECK: .addrsig_sym f1
 define void @f1() {
+  %f1 = bitcast void()* @f1 to i8*
+  %f2 = bitcast void()* @f2 to i8*
+  %f3 = bitcast void()* @f3 to i8*
+  %g1 = bitcast i32* @g1 to i8*
+  %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*
   unreachable
 }
 
 ; CHECK-NOT: .addrsig_sym f2
 define internal void @f2() local_unnamed_addr {
   unreachable
 }
 
 ; CHECK-NOT: .addrsig_sym f3
 declare void @f3() unnamed_addr
 
 ; CHECK: .addrsig_sym g1
 @g1 = global i32 0
 ; CHECK-NOT: .addrsig_sym g2
 @g2 = internal local_unnamed_addr global i32 0
 ; CHECK-NOT: .addrsig_sym g3
 @g3 = external unnamed_addr global i32
 
+; CHECK-NOT: .addrsig_sym unref
+@unref = external global i32
+
+; CHECK-NOT: .addrsig_sym dllimport
+@dllimport = external dllimport global i32
+
 ; CHECK-NOT: .addrsig_sym tls
 @tls = thread_local global i32 0
 
 ; CHECK: .addrsig_sym a1
 @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