[ELF2] - Implemented --allow-multiple-definition flag

[grimar]

Summary

Implementation of --allow-multiple-definition flag

For tests I created 2 functions with the same name but different first instruction. Then checking that order of files in linker cmd line affects on which one is used.
Should anything else be checked ?

[• rafael]

Please name the Input file Inputs/allow-multiple-definition.s.

test/elf2/allow-multiple-definition.s

7

Where is "%t" (with no number suffix) being created?

[ruiu]

ELF/SymbolTable.cpp

153–160

Usually when we concatenate strings, we use Twine. The new code uses String::operator+ instead, which is less preferred way to do the same thing. This would be better.

std::string Msg = (Twine("duplicate symbol: ") + Old.getName() + ...).str();

I also dropped "const" since it's obvious (and that use is more consistent with other code in LLD).

[ruiu]

Also no need to send a patch to both ruiu and rui314. They are just me :) Please send patches to ruiu.

[grimar]

Review comments addressed

[grimar]

ELF/SymbolTable.cpp

5–1

Ok. Fixed.

test/elf2/allow-multiple-definition.s

8

Good question. That definitely was a bug. Fixed.

[• rafael]

I can't apply the patch:

$ git apply -p0 ~/Downloads/D13169.diff
/home/espindola/Downloads/D13169.diff:9: trailing whitespace.

bool AllowMultipleDefinition = false;

/home/espindola/Downloads/D13169.diff:21: trailing whitespace.

if (Args.hasArg(OPT_allow_multiple_definition))

/home/espindola/Downloads/D13169.diff:22: trailing whitespace.

Config->AllowMultipleDefinition = true;

/home/espindola/Downloads/D13169.diff:23: trailing whitespace.

/home/espindola/Downloads/D13169.diff:35: trailing whitespace.

error: patch failed: ELF/Config.h:25
error: ELF/Config.h: patch does not apply
error: patch failed: ELF/Driver.cpp:97
error: ELF/Driver.cpp: patch does not apply
error: patch failed: ELF/Options.td:37
error: ELF/Options.td: patch does not apply
error: patch failed: ELF/SymbolTable.cpp:150
error: ELF/SymbolTable.cpp: patch does not apply

[grimar]

@rafael wrote:

I can't apply the patch:

Hmm I thought I fixed that. Going to investigate then.

[grimar]

In D13169#254626, @rafael wrote:

I can't apply the patch:

Looks like you need to use
--whitespace=warn flag as stated in
http://stackoverflow.com/questions/14509950/my-diff-contains-trailing-whitespace-how-to-get-rid-of-it

[grimar]

Ot probably it was caused by CRLF. I thought I replaced them with LF but looks like no. I reuploaded the patch with LF instead of CRLF.

[ruiu]

LGTM

ELF/SymbolTable.cpp

155

This indentation looked weird at first but that is indeed clang-format would format. So this is what we want.

[grimar]

I still have no write permissions. Could anyone commit this please ?

[grimar]

r248733

ELF/Config.h

struct Configuration {
   llvm::StringRef DynamicLinker;
   std::string RPath;
   bool Shared = false;
   bool DiscardAll = false;
   bool DiscardLocals = false;
   bool DiscardNone = false;
   bool ExportDynamic = false;
   bool NoInhibitExec = false;
+  bool AllowMultipleDefinition = false;
 };
 
 extern Configuration *Config;
 
 } // namespace elf2
 } // namespace lld
 
 #endif

ELF/Driver.cpp

if (Args.hasArg(OPT_discard_none))
     Config->DiscardNone = true;
 
   if (Args.hasArg(OPT_export_dynamic))
     Config->ExportDynamic = true;
 
   if (Args.hasArg(OPT_noinhibit_exec))
     Config->NoInhibitExec = true;
 
+  if (Args.hasArg(OPT_allow_multiple_definition))
+    Config->AllowMultipleDefinition = true;
+
   // Create a list of input files.
   std::vector<MemoryBufferRef> Inputs;
 
   for (auto *Arg : Args.filtered(OPT_INPUT)) {
     StringRef Path = Arg->getValue();
     Inputs.push_back(openFile(Path));
   }
 

ELF/Options.td

def noinhibit_exec : Flag<["--"], "noinhibit-exec">,
      HelpText<"Retain the executable output file whenever"
               " it is still usable">;
 
 def discard_none : Flag<["-"], "discard-none">,
      HelpText<"Keep all symbols in the symbol table">;
 
 def export_dynamic : Flag<["--"], "export-dynamic">,
      HelpText<"Put symbols in the dynamic symbol table">;
+
+def allow_multiple_definition: Flag<["--"], "allow-multiple-definition">,
+    HelpText<"Allow multiple definitions">;

ELF/SymbolTable.cpp

 //===- SymbolTable.cpp ----------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
for (const std::unique_ptr<ObjectFileBase> &F : ObjectFiles) {
     const auto &File = cast<ObjectFile<ELFT>>(*F);
     Elf_Sym_Range Syms = File.getObj().symbols(File.getSymbolTable());
     if (&OldE > Syms.begin() && &OldE < Syms.end())
       OldFile = F.get();
     if (&NewE > Syms.begin() && &NewE < Syms.end())
       NewFile = F.get();
   }
 
-  error(Twine("duplicate symbol: ") + Old.getName() + " in " +
-        OldFile->getName() + " and " + NewFile->getName());
+  std::string Msg = (Twine("duplicate symbol: ") + Old.getName() + " in " +
+                     OldFile->getName() + " and " + NewFile->getName())
+                        .str();
+  if (Config->AllowMultipleDefinition)
+    warning(Msg);
+  else
+    error(Msg);
 }
 
 // This function resolves conflicts if there's an existing symbol with
 // the same name. Decisions are made based on symbol type.
 template <class ELFT> void SymbolTable::resolve(SymbolBody *New) {
   Symbol *Sym = insert(New);
   if (Sym->Body == New)
     return;
 

test/elf2/Inputs/allow-multiple-definition.s

+.globl _bar
+.type _bar, @function
+_bar:
+  mov $2, %eax

test/elf2/allow-multiple-definition.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/allow-multiple-definition.s -o %t2
+# RUN: not lld -flavor gnu2 %t1 %t2 -o %t3
+# RUN: lld -flavor gnu2 --allow-multiple-definition %t1 %t2 -o %t3
+# RUN: lld -flavor gnu2 --allow-multiple-definition %t2 %t1 -o %t4
+# RUN: llvm-objdump -d %t3 | FileCheck %s
+# RUN: llvm-objdump -d %t4 | FileCheck -check-prefix=REVERT %s
+
+# inputs contain different constants for instuction movl.
+# Tests below checks that order of files in command line 
+# affects on what symbol will be used.
+# If flag allow-multiple-definition is enabled the first 
+# meet symbol should be used.
+
+# CHECK: _bar:
+# CHECK-NEXT: 11000:   b8 01 00 00 00   movl   $1, %eax
+
+# REVERT: _bar:
+# REVERT-NEXT: 11000:   b8 02 00 00 00   movl   $2, %eax
+
+.globl _bar
+.type _bar, @function
+_bar:
+  mov $1, %eax
+  
+.globl _start;
+_start: