allow-multiple-definitions should completely suppress errors instead of making them warnings.

[ruiu]

Summary

We found that when you pass --allow-multiple-definitions or -z muldefs
to GNU linkers, they don't complain about duplicate symbols at all. They
don't even print out warnings on it. We emit warnings in that case.
If you pass --fatal-warnings, that difference results in a link failure.

[• espindola]

lld/ELF/SymbolTable.cpp

420 ↗

(On Diff #138650)

Why was it necessary to inline this reportDuplicate?

[ruiu]

lld/ELF/SymbolTable.cpp

420 ↗

(On Diff #138650)

It's not necessary, but since it is now short that it is easier to read than having a overloaded function.

[• espindola]

lld/ELF/SymbolTable.cpp

420 ↗

(On Diff #138650)

Leave that for a followup patch. There may be a better way of merging these two overloads and doing it in one patch makes the patch harder to read.

[grimar]

Logic by itself looks OK to me.
+1 to leave inlining for the follow-up.

lld/ELF/SymbolTable.cpp

420 ↗

(On Diff #138650)

That is not what was changed by this patch, but have to note
that this (and below places) probably should be errorOrWarn from Config.h to support -noinhibit-exec.

[ruiu]

• address review comment.

[grimar]

LGTM

[• espindola]

lld/ELF/SymbolTable.cpp

515 ↗

(On Diff #138744)

Please leave the check for AllowMultipleDefinition in reportDuplicate. Both overloads should check it or none.

[ruiu]

lld/ELF/SymbolTable.cpp

515 ↗

(On Diff #138744)

Done.

[• espindola]

lld/ELF/SymbolTable.cpp

515 ↗

(On Diff #138744)

Have you forgot to upload a new patch? In any case, LGTM with that.

lld/trunk/ELF/SymbolTable.cpp

if (Cmp > 0) {
     if (Size > Bss->Size) {
       D->File = Bss->File = &File;
       D->Size = Bss->Size = Size;
     }
   }
   return S;
 }
 
-static void warnOrError(const Twine &Msg) {
-  if (Config->AllowMultipleDefinition)
-    warn(Msg);
-  else
-    error(Msg);
-}
-
 static void reportDuplicate(Symbol *Sym, InputFile *NewFile) {
-  warnOrError("duplicate symbol: " + toString(*Sym) + "\n>>> defined in " +
-              toString(Sym->File) + "\n>>> defined in " + toString(NewFile));
+  if (!Config->AllowMultipleDefinition)
+    error("duplicate symbol: " + toString(*Sym) + "\n>>> defined in " +
 }
 
 static void reportDuplicate(Symbol *Sym, InputFile *NewFile,
                             InputSectionBase *ErrSec, uint64_t ErrOffset) {
+  if (Config->AllowMultipleDefinition)
+    return;
+
   Defined *D = cast<Defined>(Sym);
   if (!D->Section || !ErrSec) {
     reportDuplicate(Sym, NewFile);
     return;
   }
 
   // Construct and print an error message in the form of:
   //
static void reportDuplicate(Symbol *Sym, InputFile *NewFile,
 
   std::string Msg = "duplicate symbol: " + toString(*Sym) + "\n>>> defined at ";
   if (!Src1.empty())
     Msg += Src1 + "\n>>>            ";
   Msg += Obj1 + "\n>>> defined at ";
   if (!Src2.empty())
     Msg += Src2 + "\n>>>            ";
   Msg += Obj2;
-  warnOrError(Msg);
+  error(Msg);
 }
 
 Symbol *SymbolTable::addRegular(StringRef Name, uint8_t StOther, uint8_t Type,
                                 uint64_t Value, uint64_t Size, uint8_t Binding,
                                 SectionBase *Section, InputFile *File) {
   Symbol *S;
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name, Type, getVisibility(StOther),

lld/trunk/test/ELF/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 ld.lld %t1 %t2 -o %t3
 # RUN: not ld.lld --allow-multiple-definition --no-allow-multiple-definition %t1 %t2 -o %t3
-# RUN: ld.lld --allow-multiple-definition %t1 %t2 -o %t3
-# RUN: ld.lld --allow-multiple-definition %t2 %t1 -o %t4
+# RUN: ld.lld --allow-multiple-definition --fatal-warnings %t1 %t2 -o %t3
+# RUN: ld.lld --allow-multiple-definition --fatal-warnings %t2 %t1 -o %t4
 # RUN: llvm-objdump -d %t3 | FileCheck %s
 # RUN: llvm-objdump -d %t4 | FileCheck -check-prefix=REVERT %s
 
-# RUN: ld.lld -z muldefs %t1 %t2 -o %t3
-# RUN: ld.lld -z muldefs %t2 %t1 -o %t4
+# RUN: ld.lld -z muldefs --fatal-warnings  %t1 %t2 -o %t3
+# RUN: ld.lld -z muldefs --fatal-warnings  %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.