[lld] Avoid promoting an undefined weak entry symbol to global.

[ikudrin]

Summary

Without this patch, lld emits "error: undefined symbol: _start" if it encountered only weak references to that symbol.

[ruiu]

Is there any practical reason to do this? A system having _start as a weak symbol looks a bit odd.

[ikudrin]

We came across the issue in our environment, where an ELF linker is used only as an intermediate step and the value of entry field in the header is ignored. But in one of the libraries, we have a weak reference to _start, because sometimes this symbol might be defined for a special purpose. I can guess that for some other embedded environments and third-party libraries the same situation might take place.

[ruiu]

ELF/Driver.cpp

1038

What the comment describing this code says is that we do this to load a start symbol from a static library. So maybe it is better.

if (SymbolBody *B = Symtab->find(Config->Entry))
  if (B->isLazy())
    Symtab->addUndefined<ELFT>(Config->Entry);

[ikudrin]

Yes, this variant is more straightforward. However, as addUndefined() implicitly marks the symbol to be preserved by LTO, we now have to set that flag directly; overwise, many tests in the lto test-suite fail. Moreover, without that flag, the entry field in the ELF header is not filled correctly in case of LTO because symbol _start is missing.

[ruiu]

ELF/Driver.cpp

1039

It seems a bit odd because we are not doing this for symbols given by the -undefined option. Is this a bug of -undefined, or don't we need it here as well?

[ikudrin]

ELF/Driver.cpp

1039

I can't tell about "-u", but if we miss this for the entry symbols, and it is defined in a BC file, we won't be able to fill the entry field in the ELF header correctly.

[ruiu]

It looks like a symbol specified by -e can be handled as if it were specified by -u. However, it won't work because I believe -u doesn't keep symbols over LTO. So, if -u has the same bug, then it is probably better to fix it and then add a symbol specified by -e to Config->Undefined.

[ikudrin]

Did you mean something like this?
I am inclined not to mix an entry symbol into Config->Undefined in order to preserve clarity.

[ruiu]

ELF/SymbolTable.cpp

585

Can you make this a member of SymbolTable and use this instead of addEntrySymbol?

587

... this symbol even if B is a bitcode symbol.

We should do this only when B is a lazy symbol, no?

[ikudrin]

Use fetchIfLazy() instead of addEntrySymbol().

[ikudrin]

ELF/SymbolTable.cpp

587

Unfortunately, we should set the flag for regular symbols, too. If it is set only for lazy symbols, lots of tests in LTO suit fail:

lld :: ELF/lto/archive-2.ll
lld :: ELF/lto/codemodel.ll
lld :: ELF/lto/combined-lto-object-name.ll
lld :: ELF/lto/dynamic-list.ll
lld :: ELF/lto/internalize-basic.ll
lld :: ELF/lto/lto-start.ll
lld :: ELF/lto/opt-level.ll
lld :: ELF/lto/parallel-internalize.ll
lld :: ELF/lto/shlib-undefined.ll
lld :: ELF/lto/thin-archivecollision.ll
lld :: ELF/lto/undefined-puts.ll

Some of them might be fixed in tests themselves, but others show why regular symbols should be marked. See ELF/lto/lto-start.ll for example.

[ruiu]

ELF/SymbolTable.cpp

587

But still, setting IsUsedInRegularObj to true even if B is Undefined seems a bit odd. ("An undefined/lazy symbol that is used in regular obj" sounds like an odd concept.)

[ikudrin]

ELF/SymbolTable.cpp

587

Do you mean something like

if (!B->isUndefined())
  B->symbol()->IsUsedInRegularObj = true;

or even

if (!B->isUndefined() && B->symbol()->File &&
    B->symbol()->File->kind() == InputFile::BitcodeKind)
  B->symbol()->IsUsedInRegularObj = true;

?

[ikudrin]

Ping. What can I do to improve the patch?

[ruiu]

ELF/SymbolTable.cpp

599–600

This changes the semantics of the -u option because the new code sets IsUsedInRegularObj to true. And I believe that's not a desired change.

[ikudrin]

I assumed that here...

I believe -u doesn't keep symbols over LTO. So, if -u has the same bug, then it is probably better to fix it

... you suggested that --undefined symbols should be preserved during LTO, as well as the entry symbol. Am I right?

In any case, the entry symbol should survive LTO; otherwise, we won't be able to fill the entry field in the header.

[ruiu]

... you suggested that --undefined symbols should be preserved during LTO, as well as the entry symbol. Am I right?

Oh sorry, I forgot about my comment! But regardless of its correctness, it should be done in a separate patch, as this patch doesn't have any test for that change.

[ikudrin]

Extract handling of '-u' symbols into a separate patch, see D38348.

[ruiu]

LGTM

ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   // Some symbols (such as __ehdr_start) are defined lazily only when there
   // are undefined symbols for them, so we add these to trigger that logic.
   for (StringRef Sym : Script->Opt.ReferencedSymbols)
     Symtab->addUndefined<ELFT>(Sym);
 
   // If an entry symbol is in a static archive, pull out that file now
   // to complete the symbol table. After this, no new names except a
   // few linker-synthesized ones will be added to the symbol table.
-  if (Symtab->find(Config->Entry))
-    Symtab->addUndefined<ELFT>(Config->Entry);
+  Symtab->fetchIfLazy<ELFT>(Config->Entry);
 
   // Return if there were name resolution errors.
   if (ErrorCount)
     return;
 
   // Handle the `--undefined <sym>` options.
   Symtab->scanUndefinedFlags<ELFT>();
 
   // Handle undefined symbols in DSOs.
   Symtab->scanShlibUndefined<ELFT>();

ELF/SymbolTable.h

Symbol *addCommon(StringRef N, uint64_t Size, uint32_t Alignment,
                     uint8_t Binding, uint8_t StOther, uint8_t Type,
                     InputFile *File);
 
   std::pair<Symbol *, bool> insert(StringRef Name);
   std::pair<Symbol *, bool> insert(StringRef Name, uint8_t Type,
                                    uint8_t Visibility, bool CanOmitFromDynSym,
                                    InputFile *File);
 
+  template <class ELFT> void fetchIfLazy(StringRef Name);
   template <class ELFT> void scanUndefinedFlags();
   template <class ELFT> void scanShlibUndefined();
   void scanVersionScript();
 
   SymbolBody *find(StringRef Name);
 
   void trace(StringRef Name);
 

ELF/SymbolTable.cpp

void SymbolTable::addLazyObject(StringRef Name, LazyObjFile &Obj) {
 
   // See comment for addLazyArchive above.
   if (S->isWeak())
     replaceBody<LazyObject>(S, &Obj, Name, S->body()->Type);
   else if (InputFile *F = Obj.fetch())
     addFile<ELFT>(F);
 }
 
-// Process undefined (-u) flags by loading lazy symbols named by those flags.
-template <class ELFT> void SymbolTable::scanUndefinedFlags() {
-  for (StringRef S : Config->Undefined)
-    if (SymbolBody *B = find(S)) {
-      // Mark the symbol not to be eliminated by LTO
-      // even if it is a bitcode symbol.
-      B->symbol()->IsUsedInRegularObj = true;
-      if (auto *L = dyn_cast_or_null<Lazy>(B))
-        if (InputFile *File = L->fetch())
-          addFile<ELFT>(File);
-    }
+// If we already saw this symbol, force loading its file.
+template <class ELFT> void SymbolTable::fetchIfLazy(StringRef Name) {
+  if (SymbolBody *B = find(Name)) {
 }
 
+// Process undefined (-u) flags by loading lazy symbols named by those flags.
+template <class ELFT> void SymbolTable::scanUndefinedFlags() {
+  for (StringRef S : Config->Undefined)
+    fetchIfLazy<ELFT>(S);
+}
+
 // This function takes care of the case in which shared libraries depend on
 // the user program (not the other way, which is usual). Shared libraries
 // may have undefined symbols, expecting that the user program provides
 // the definitions for them. An example is BSD's __progname symbol.
 // We need to put such symbols to the main program's .dynsym so that
 // shared libraries can find them.
 // Except this, we ignore undefined symbols in DSOs.
 template <class ELFT> void SymbolTable::scanShlibUndefined() {
template void SymbolTable::addShared<ELF32BE>(SharedFile<ELF32BE> *, StringRef,
                                               const typename ELF32BE::Verdef *);
 template void SymbolTable::addShared<ELF64LE>(SharedFile<ELF64LE> *, StringRef,
                                               const typename ELF64LE::Sym &,
                                               const typename ELF64LE::Verdef *);
 template void SymbolTable::addShared<ELF64BE>(SharedFile<ELF64BE> *, StringRef,
                                               const typename ELF64BE::Sym &,
                                               const typename ELF64BE::Verdef *);
 
+template void SymbolTable::fetchIfLazy<ELF32LE>(StringRef);
+template void SymbolTable::fetchIfLazy<ELF32BE>(StringRef);
+template void SymbolTable::fetchIfLazy<ELF64LE>(StringRef);
+template void SymbolTable::fetchIfLazy<ELF64BE>(StringRef);
+
 template void SymbolTable::scanUndefinedFlags<ELF32LE>();
 template void SymbolTable::scanUndefinedFlags<ELF32BE>();
 template void SymbolTable::scanUndefinedFlags<ELF64LE>();
 template void SymbolTable::scanUndefinedFlags<ELF64BE>();
 
 template void SymbolTable::scanShlibUndefined<ELF32LE>();
 template void SymbolTable::scanShlibUndefined<ELF32BE>();
 template void SymbolTable::scanShlibUndefined<ELF64LE>();
 template void SymbolTable::scanShlibUndefined<ELF64BE>();

test/ELF/weak-entry.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t
+# RUN: ld.lld %t -o %tout
+# RUN: llvm-nm %tout | FileCheck %s
+
+# CHECK:      w _start
+# CHECK-NEXT: T foo
+
+.global foo
+.weak _start
+.text
+foo:
+  .dc.a _start