[ELF2] Implement --allow-shlib-undefined as default behavior.

[grimar]

Summary

We ignore --{no,}allow-shlib-undefined options and always allow undefined
symbols if we are building a DSO.

[denis-protivensky]

ELF/Config.h

28 ↗

(On Diff #35965)

AllowShlibUndefined

28 ↗

(On Diff #35965)

As I can read from the description provided, this flag has different meanings depending on whether executable or DSO is being linked. If so, it's initial value is neither false nor true, because it depends on other flags. Hence you should use Optional for such a value.

ELF/Driver.cpp

102 ↗

(On Diff #35965)

Better to have two checks of the form:

auto *Arg = Args.getLastArg(OPT_allow_shlib_undefined);
if (Arg || Config->Shared)
  Config->AllowShlibUndefines = true;

The same is for the contrary option. And remove the code above in OPT_shared check.
Also, watch local variables to start from the capital letter.

ELF/Options.td

6

Why executables? It affects DSOs as well.

10

Ditto.

ELF/Writer.cpp

3–1

Don't you need to check that this symbol is from DSO, not from object file, for example?

ELF/Writer.h

25 ↗

(On Diff #35965)

Use StringRef to pass string arguments.

test/elf2/allow-shlib-undefined.s

14

Didn't check linking of DSO without --[no]-allow-shlib-undefined flags.

[ruiu]

The GNU linker's behavior of trying to resolve all undefined symbols in DSOs looks broken in the first place. This is a linker and not a dynamic linker nor loader, it can just leave undefined symbols in DSO for the dynamic linker. LLVM itself, for example, always sets --allow-shlib-undefined to "[W]ork around a broken bfd ld behavior" (CMakeList.txt in the toplevel directory). In my opinion, we should just implement --allow-shlib-undefined behavior as default and ignore that option.

[grimar]

ELF/Config.h

28 ↗

(On Diff #35965)

Honestly I dont see advantage of using it here. In continue of that logic - all flags depends on command line arguments :)
We are going to set AllowShlibUndefined flag once at start and only read it later. Optional seems to be too heavy solution for this.
So I would like to hear more opinions about change to Optional here.

ELF/Driver.cpp

102 ↗

(On Diff #35965)

What if both flags --allow_shlib_undefined and --no-allow_shlib_undefined be declared at the same time ? My solution will choose the latest in command line I think. Your one will choose one depending of what condition is latest in code.
Not sure what is better and if we should report an error in this case.

ELF/Options.td

6

Will be fixed.

ELF/Writer.cpp

3–1

Not here.
All checks are performed in single undefError function:
void undefError(const Twine &Symbol, const Twine &SymFile)

DSOs calls it directly in SharedFile<ELFT>::parse()

ELF/Writer.h

25 ↗

(On Diff #35965)

Could you please explain why ? It looks like Twine arguments used much more often in llvm code.

test/elf2/allow-shlib-undefined.s

14

Actually I did. The second line:

1. RUN: lld -shared -flavor gnu2 %t -o %t.so

[grimar]

In D13244#255737, @ruiu wrote:

The GNU linker's behavior of trying to resolve all undefined symbols in DSOs looks broken in the first place. This is a linker and not a dynamic linker nor loader, it can just leave undefined symbols in DSO for the dynamic linker. LLVM itself, for example, always sets --allow-shlib-undefined to "[W]ork around a broken bfd ld behavior" (CMakeList.txt in the toplevel directory). In my opinion, we should just implement --allow-shlib-undefined behavior as default and ignore that option.

May be it is more reasonable to implement --allow-shlib-undefined behavior as default for both shared and executables but still allow to use ---no-allow-shlib-undefined if needed ?

[denis-protivensky]

ELF/Config.h

28 ↗

(On Diff #35965)

The problem is that in this logic the flag is neither false nor true by default. It's in the intermediate state. There was a discussion to have indeterminate values for such cases. Optional may be not the best case, but leaving the value of false is also a bit odd.

ELF/Driver.cpp

102 ↗

(On Diff #35965)

The linker applies command line arguments in the order it encounters them. So if multiple flags are provided - you need to pick the last. You may use filtered to accomplish that.

ELF/Writer.h

25 ↗

(On Diff #35965)

I agree that you *may* pass Twine here because you won't store its value inside. I wouldn't do that, but I see other error functions accept Twines, so ok.

test/elf2/allow-shlib-undefined.s

14

So the comment above is misleading: it states you check three options, but you do check only two of them.

[grimar]

--allow-shlib-undefined behavior implemented as default.
--[no]-allow-shlib-undefined options are ignored.
I also made undefError visible outside writer because it will be useful for other flags (like --no-undefined I am working on)

[ruiu]

ELF/Options.td

3–9

We don't need help texts for these no-op flags.

ELF/Writer.cpp

4

Export this function when you need it. Currently no one is using this except undefError().

Also please avoid overriding function unless it really makes sense. We already have this function,

template <class ELFT> static void undefError(const SymbolTable &, const SymbolBody &);

and this patch defines

void undefError(const Twine &, const Twine &)

That's confusing. You can just give different names.

[grimar]

Review comments addressed.

[ruiu]

LGTM

Please update the commit message.

[ELF2] Implement --allow-shlib-undefined as default behavior.

We ignore --{no,}allow-shlib-undefined options and always allow undefined
symbols if we are building a DSO.

ELF/Options.td

65–69

Write each of them in one line and move at the end of the file below "these flags are ignored ..." fold after rebasing.

ELF/Writer.cpp

274–275

My concern was it returns without doing anything although the name implies it's fatal. But I renamed this function, so it should be fine now.

[ruiu]

ELF/Options.td

65–69

Sorry, this was a stale comment. Please ignore this one.

[grimar]

r249036

ELF/Options.td

65–69

I Ignored, but it looks like "Options listed below.." is really good place for them now since them are really ignored.
If no - then I guess them should be placed to their alphabetical order position ?

ELF/Options.td

 include "llvm/Option/OptParser.td"
 
 //===----------------------------------------------------------------------===//
 /// Utility Functions
 //===----------------------------------------------------------------------===//
 
 def output : Separate<["-"], "o">, MetaVarName<"<path>">,
      HelpText<"Path to file to write output">;
 
 def dynamic_linker : Separate<["-"], "dynamic-linker">,
      HelpText<"Which dynamic linker to use">;
 
 def rpath : Separate<["-"], "rpath">,
      HelpText<"Add a DT_RUNPATH to the output">;
 
 def shared : Flag<["-"], "shared">,
      HelpText<"Build a shared object">;
 

 def sysroot : Joined<["--"], "sysroot=">,
      HelpText<"Set the system root">;
 
 def entry : Separate<["--", "-"], "entry">, MetaVarName<"<entry>">,
      HelpText<"Name of entry point symbol">;
 
 def alias_entry : Separate<["-"], "e">,
      Alias<entry>;
+
+def no_allow_shlib_undefined
+    : Flag<["--"], "no-allow-shlib-undefined">;
+
+def allow_shlib_undefined
+    : Flag<["--"], "allow-shlib-undefined">;

ELF/Writer.cpp

 //===- Writer.cpp ---------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Writer.h"
 #include "Config.h"
 #include "OutputSections.h"
void Writer<ELFT>::scanRelocs(const InputSection<ELFT> &C) {
   }
 }
 
 template <class ELFT>
 static void undefError(const SymbolTable &S, const SymbolBody &Sym) {
   typedef typename ELFFile<ELFT>::Elf_Sym Elf_Sym;
   typedef typename ELFFile<ELFT>::Elf_Sym_Range Elf_Sym_Range;
 
+  if (Config->Shared)
+    return;
+
   const Elf_Sym &SymE = cast<ELFSymbolBody<ELFT>>(Sym).Sym;
   ELFFileBase *SymFile = nullptr;
 
   for (const std::unique_ptr<ObjectFileBase> &F : S.getObjectFiles()) {
     const auto &File = cast<ObjectFile<ELFT>>(*F);
     Elf_Sym_Range Syms = File.getObj().symbols(File.getSymbolTable());
     if (&SymE > Syms.begin() && &SymE < Syms.end())
       SymFile = F.get();

test/elf2/Inputs/allow-shlib-undefined.s

+.globl _shared
+_shared: 
+  call _unresolved

test/elf2/allow-shlib-undefined.s

+# --allow-shlib-undefined and --no-allow-shlib-undefined are fully
+# ignored in linker implementation. 
+# --allow-shlib-undefined is set by default
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+# RUN: %p/Inputs/allow-shlib-undefined.s -o %t
+# RUN: lld -shared -flavor gnu2 %t -o %t.so 
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
+
+# Executable: should link with DSO containing undefined symbols in any case.
+# RUN: lld -flavor gnu2 %t1 %t.so -o %t2
+# RUN: lld -flavor gnu2 --no-allow-shlib-undefined %t1 %t.so -o %t2
+# RUN: lld -flavor gnu2 --allow-shlib-undefined %t1 %t.so -o %t2
+ 
+# DSO with undefines: 
+# should link with or without any of these options.
+# RUN: lld -shared -flavor gnu2 %t -o %t.so 
+# RUN: lld -shared --allow-shlib-undefined -flavor gnu2 %t -o %t.so 
+# RUN: lld -shared --no-allow-shlib-undefined -flavor gnu2 %t -o %t.so 
+ 
+# Executable still should not link when have undefines inside.
+# RUN: not lld -flavor gnu2 %t -o %t.so 
+
+.globl _start
+_start: 
+  call _shared