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

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

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; }
 

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;

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();
   }