LTO: Pass SF_Executable flag through to InputFile::Symbol

[tobiasvk]

Summary

The linker needs to be able to determine whether a symbol is text or data to
handle the case of a common being overridden by a strong definition in an
archive. If the archive contains a text member of the same name as the common,
that function is discarded. However, if the archive contains a data member of
the same name, that strong definition overrides the common. This is a behavior
of ld.bfd, which the Qualcomm linker also supports in LTO.

Here's a test case to illustrate:

cat > 1.c << \!
int blah;
!

cat > 2.c << \!
int blah() {

return 0;

}
!

cat > 3.c << \!
int blah = 20;
!

clang -c 1.c
clang -c 2.c
clang -c 3.c

ar cr lib.a 2.o 3.o
ld 1.o lib.a -t

The correct output is:

1.o
(lib.a)3.o

Thanks to Shankar Easwaran and Hemant Kulkarni for the test case!

[davide]

This needs a testcase.

[pcc]

This ld.bfd behaviour [0,1] is very strange. It appears to be loading archive members to resolve common symbols, but will only do so if the archive member's definition is neither a common symbol itself (which is reasonable enough I suppose) nor of function type (which doesn't really seem necessary to check). I have no strong objections to extending the API to allow clients to implement compatible behaviour, but I don't think we should implement it in lld for example.

Agreed with @davide that this needs a test case. I am working on a change to llvm-lto2 that adds a feature for dumping the LTO symbol table, which should allow you to test this. @tobiasvk would you be fine with waiting until that change lands?

[0] https://github.com/bminor/binutils-gdb/blob/8170f7693bc0a9442c0aa280197925db92d48ca6/bfd/elflink.c#L5372
[1] https://github.com/bminor/binutils-gdb/blob/8170f7693bc0a9442c0aa280197925db92d48ca6/bfd/elflink.c#L3124

lib/Object/IRSymtab.cpp

129 ↗

(On Diff #94711)

clang-format

[tobiasvk]

In D31901#723155, @pcc wrote:

Agreed with @davide that this needs a test case. I am working on a change to llvm-lto2 that adds a feature for dumping the LTO symbol table, which should allow you to test this. @tobiasvk would you be fine with waiting until that change lands?

Yes, absolutely agree we need a test case. It's just currently not possible to test it inside llvm without the linker, so I couldn't add one. It would be great if you could add a symbol table dump feature to llvm-lto2. Please add me to the review when you get to it, so we can merge this once your patch has landed.

Thanks!

[pcc]

In D31901#723759, @tobiasvk wrote:

In D31901#723155, @pcc wrote:

Agreed with @davide that this needs a test case. I am working on a change to llvm-lto2 that adds a feature for dumping the LTO symbol table, which should allow you to test this. @tobiasvk would you be fine with waiting until that change lands?

Yes, absolutely agree we need a test case. It's just currently not possible to test it inside llvm without the linker, so I couldn't add one. It would be great if you could add a symbol table dump feature to llvm-lto2. Please add me to the review when you get to it, so we can merge this once your patch has landed.

This is D31920, I added you as a subscriber.

[tobiasvk]

Add test case now that D31920 is merged.

[pcc]

LGTM

llvm/trunk/include/llvm/LTO/LTO.h

public:
     using irsymtab::Symbol::getName;
     using irsymtab::Symbol::getVisibility;
     using irsymtab::Symbol::canBeOmittedFromSymbolTable;
     using irsymtab::Symbol::isTLS;
     using irsymtab::Symbol::getComdatIndex;
     using irsymtab::Symbol::getCommonSize;
     using irsymtab::Symbol::getCommonAlignment;
     using irsymtab::Symbol::getCOFFWeakExternalFallback;
+    using irsymtab::Symbol::isExecutable;
   };
 
   /// A range over the symbols in this InputFile.
   ArrayRef<Symbol> symbols() const { return Symbols; }
 
   /// Returns linker options specified in the input file.
   StringRef getCOFFLinkerOpts() const { return COFFLinkerOpts; }
 

llvm/trunk/include/llvm/Object/IRSymtab.h

enum FlagBits {
     FB_common,
     FB_indirect,
     FB_used,
     FB_tls,
     FB_may_omit,
     FB_global,
     FB_format_specific,
     FB_unnamed_addr,
+    FB_executable,
   };
 
   /// The index into the Uncommon table, or -1 if this symbol does not have an
   /// Uncommon.
   Word UncommonIndex;
 };
 
 /// This data structure contains rarely used symbol fields and is optionally
struct Symbol {
   bool isUsed() const { return (Flags >> S::FB_used) & 1; }
   bool isTLS() const { return (Flags >> S::FB_tls) & 1; }
   bool canBeOmittedFromSymbolTable() const {
     return (Flags >> S::FB_may_omit) & 1;
   }
   bool isGlobal() const { return (Flags >> S::FB_global) & 1; }
   bool isFormatSpecific() const { return (Flags >> S::FB_format_specific) & 1; }
   bool isUnnamedAddr() const { return (Flags >> S::FB_unnamed_addr) & 1; }
+  bool isExecutable() const { return (Flags >> S::FB_executable) & 1; }
 
   uint64_t getCommonSize() const {
     assert(isCommon());
     return CommonSize;
   }
   uint32_t getCommonAlignment() const {
     assert(isCommon());
     return CommonAlign;

llvm/trunk/lib/Object/IRSymtab.cpp

Error Builder::addSymbol(ModuleSymbolTable::Symbol Msym) {
   if (Flags & object::BasicSymbolRef::SF_Common)
     Sym.Flags |= 1 << storage::Symbol::FB_common;
   if (Flags & object::BasicSymbolRef::SF_Indirect)
     Sym.Flags |= 1 << storage::Symbol::FB_indirect;
   if (Flags & object::BasicSymbolRef::SF_Global)
     Sym.Flags |= 1 << storage::Symbol::FB_global;
   if (Flags & object::BasicSymbolRef::SF_FormatSpecific)
     Sym.Flags |= 1 << storage::Symbol::FB_format_specific;
+  if (Flags & object::BasicSymbolRef::SF_Executable)
+    Sym.Flags |= 1 << storage::Symbol::FB_executable;
 
   Sym.ComdatIndex = -1;
   auto *GV = Msym.dyn_cast<GlobalValue *>();
   if (!GV) {
     setStr(Sym.IRName, "");
     return Error::success();
   }
 

llvm/trunk/test/LTO/Resolution/X86/symtab.ll

 ; RUN: llvm-as -o %t %s
 ; RUN: llvm-lto2 dump-symtab %t | FileCheck %s
 
 target triple = "i686-pc-windows-msvc18.0.0"
 target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
 
 ; CHECK: source filename: src.c
 source_filename = "src.c"
 
 ; CHECK: linker opts (COFF only): /include:foo
 !0 = !{i32 6, !"Linker Options", !{!{!"/include:foo"}}}
 !llvm.module.flags = !{ !0 }
 
-; CHECK: H------ _g1
+; CHECK: D------X _fun
+define i32 @fun() {
+  ret i32 0
+}
+
+; CHECK: H------- _g1
 @g1 = hidden global i32 0
 
-; CHECK: P------ _g2
+; CHECK: P------- _g2
 @g2 = protected global i32 0
 
-; CHECK: D------ _g3
+; CHECK: D------- _g3
 @g3 = global i32 0
 
-; CHECK: DU----- _g4
+; CHECK: DU------ _g4
 @g4 = external global i32
 
-; CHECK: D--W--- _g5
+; CHECK: D--W---- _g5
 @g5 = weak global i32 0
 
-; CHECK: D--W-O- _g6
+; CHECK: D--W-O-- _g6
 @g6 = linkonce_odr unnamed_addr global i32 0
 
-; CHECK: D-----T _g7
+; CHECK: D-----T- _g7
 @g7 = thread_local global i32 0
 
-; CHECK: D-C---- _g8
+; CHECK: D-C----- _g8
 ; CHECK-NEXT: size 4 align 8
 @g8 = common global i32 0, align 8
 
-; CHECK: D------ _g9
+; CHECK: D------- _g9
 ; CHECK-NEXT: comdat g9
 $g9 = comdat any
 @g9 = global i32 0, comdat
 
-; CHECK: D--WI-- _g10
+; CHECK: D--WI--- _g10
 ; CHECK-NEXT: comdat g9
 ; CHECK-NEXT: fallback _g9
 @g10 = weak alias i32, i32* @g9

llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp

for (const InputFile::Symbol &Sym : Input->symbols()) {
 
       auto PrintBool = [&](char C, bool B) { outs() << (B ? C : '-'); };
       PrintBool('U', Sym.isUndefined());
       PrintBool('C', Sym.isCommon());
       PrintBool('W', Sym.isWeak());
       PrintBool('I', Sym.isIndirect());
       PrintBool('O', Sym.canBeOmittedFromSymbolTable());
       PrintBool('T', Sym.isTLS());
+      PrintBool('X', Sym.isExecutable());
       outs() << ' ' << Sym.getName() << '\n';
 
       if (Sym.isCommon())
-        outs() << "        size " << Sym.getCommonSize() << " align "
+        outs() << "         size " << Sym.getCommonSize() << " align "
                << Sym.getCommonAlignment() << '\n';
 
       int Comdat = Sym.getComdatIndex();
       if (Comdat != -1)
-        outs() << "        comdat " << ComdatTable[Comdat] << '\n';
+        outs() << "         comdat " << ComdatTable[Comdat] << '\n';
 
       if (Sym.isWeak() && Sym.isIndirect())
-        outs() << "        fallback " << Sym.getCOFFWeakExternalFallback() << '\n';
+        outs() << "         fallback " << Sym.getCOFFWeakExternalFallback() << '\n';
     }
 
     outs() << '\n';
   }
 
   return 0;
 }