[lld] [ELF] Support for warn-once option

[spetrovic]

Summary

This patch adds warn-once option in LLD.

[ruiu]

I personally did not find this option very useful. Could you explain in what situation you find this useful?

[spetrovic]

I have something like this in many places:

extern void foo(int a);

int main() {

int i;
for (i=0;i<20;i++)
  foo(i);
return 0;

}

Without warn-once option on this example lld prints 20 error messages (-O2 optimization while making object file), with warn-once option it prints just one error message, so if we have similar code in many places this option might be useful.

[grimar]

In D56722#1361661, @spetrovic wrote:

Without warn-once option on this example lld prints 20 error messages (-O2 optimization while making object file), with warn-once option it prints just one error message, so if we have similar code in many places this option might be useful.

May we you just can use -error-limit=0 (no limit)?

[spetrovic]

Well yes, but it would be nicer not having to modify the scripts which we currently use in our build systems.

[grimar]

In theory, spamming user with the same errors is not very friendly/useful indeed. Especially when we have 20 errors limit by default. Rui, what do you think if we would always enable -warn-once functionality?
(i.e. we would continue ignoring the option, because does not seem we would want to have -no-warn-once, but change the behavior to report an error only once for the symbol).

[spetrovic]

ping

[ruiu]

I think I'm fine with this feature. I wouldn't personally use this that much, but I can imagine that some people would find this useful.

ELF/Config.h

185

Remove = false

ELF/Driver.cpp

869

This is the right place to set to Config->WarnOnce

1114–1116

instead of this place.

ELF/Options.td

383

per -> for each

ELF/Symbols.h

213

This comment should end with a full stop. Please also mention that this bit is used for --warn-once.

IsWarned -> Warned

test/ELF/warn-once.s

14

Please make sure that this file ends with a newline character.

[grimar]

test/ELF/warn-once.s

19

nit: remove ;

[grimar]

test/ELF/warn-once.s

12

BTW, this probably can be just the following for simplicity:

# CHECK:  error: undefined symbol: foo
# CHECK-NOT: error: undefined symbol: foo

(no need to check reporting of the location, we have a lot of other tests for that)

[grimar]

Rui, should we make -warn-once ON by default?

ELF/Driver.cpp

872

I think this is not clang-formatted.
(Should be single line).

[ruiu]

Rui, should we make -warn-once ON by default?

No, I don't think so. Either on or off could work as a default, but ON is currently the default, and I don't think we have a reason to change the default.

ELF/Options.td

382

You should follow the local convention of a file. Please add a blank line before each def line.

[ruiu]

LGTM

[MaskRay]

Is this still needed after https://reviews.llvm.org/D63344 ? @thakis

ELF/Config.h

struct Configuration {
   bool ThinLTOIndexOnly;
   bool TocOptimize;
   bool UndefinedVersion;
   bool UseAndroidRelrTags = false;
   bool WarnBackrefs;
   bool WarnCommon;
   bool WarnIfuncTextrel;
   bool WarnMissingEntry;
+  bool WarnOnce;
   bool WarnSymbolOrdering;
   bool WriteAddends;
   bool ZCombreloc;
   bool ZCopyreloc;
   bool ZExecstack;
   bool ZGlobal;
   bool ZHazardplt;
   bool ZInitfirst;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->UseAndroidRelrTags = Args.hasFlag(
       OPT_use_android_relr_tags, OPT_no_use_android_relr_tags, false);
   Config->UnresolvedSymbols = getUnresolvedSymbolPolicy(Args);
   Config->WarnBackrefs =
       Args.hasFlag(OPT_warn_backrefs, OPT_no_warn_backrefs, false);
   Config->WarnCommon = Args.hasFlag(OPT_warn_common, OPT_no_warn_common, false);
   Config->WarnIfuncTextrel =
       Args.hasFlag(OPT_warn_ifunc_textrel, OPT_no_warn_ifunc_textrel, false);
   Config->WarnSymbolOrdering =
       Args.hasFlag(OPT_warn_symbol_ordering, OPT_no_warn_symbol_ordering, true);
+  Config->WarnOnce = Args.hasArg(OPT_warn_once);
   Config->ZCombreloc = getZFlag(Args, "combreloc", "nocombreloc", true);
   Config->ZCopyreloc = getZFlag(Args, "copyreloc", "nocopyreloc", true);
   Config->ZExecstack = getZFlag(Args, "execstack", "noexecstack", false);
   Config->ZGlobal = hasZOption(Args, "global");
   Config->ZHazardplt = hasZOption(Args, "hazardplt");
   Config->ZInitfirst = hasZOption(Args, "initfirst");
   Config->ZInterpose = hasZOption(Args, "interpose");
   Config->ZKeepTextSectionPrefix = getZFlag(
       Args, "keep-text-section-prefix", "nokeep-text-section-prefix", false);
case OPT_no_whole_archive:
       InWholeArchive = false;
       break;
     case OPT_just_symbols:
       if (Optional<MemoryBufferRef> MB = readFile(Arg->getValue())) {
         Files.push_back(createObjectFile(*MB));
         Files.back()->JustSymbols = true;
       }
       break;
     case OPT_start_group:
       if (InputFile::IsInGroup)
         error("nested --start-group");
       InputFile::IsInGroup = true;
       break;
     case OPT_end_group:
       if (!InputFile::IsInGroup)
         error("stray --end-group");
       InputFile::IsInGroup = false;
       ++InputFile::NextGroupId;
       break;

ELF/Options.td

defm warn_ifunc_textrel: B<"warn-ifunc-textrel",
     "Do not warn about using ifunc symbols with text relocations (default)">;
 
 defm warn_symbol_ordering: B<"warn-symbol-ordering",
     "Warn about problems with the symbol ordering file (default)",
     "Do not warn about problems with the symbol ordering file">;
 
 def warn_unresolved_symbols: F<"warn-unresolved-symbols">,
   HelpText<"Report unresolved symbols as warnings">;
   
+def warn_once: F<"warn-once">,
+  HelpText<"Warn only once for each undefined symbol">;
+
 defm whole_archive: B<"whole-archive",
     "Force load of all members in a static library",
     "Do not force load of all members in a static library (default)">;
 
 defm wrap: Eq<"wrap", "Use wrapper functions for symbol">,
   MetaVarName<"<symbol>=<symbol>">;
 
 def z: JoinedOrSeparate<["-"], "z">, MetaVarName<"<option>">,

 def: F<"no-keep-memory">;
 def: F<"no-mmap-output-file">;
 def: F<"no-warn-mismatch">;
 def: Separate<["--", "-"], "rpath-link">;
 def: J<"rpath-link=">;
 def: F<"sort-common">;
 def: F<"stats">;
 def: F<"warn-execstack">;
-def: F<"warn-once">;
 def: F<"warn-shared-textrel">;
 def: F<"EB">;
 def: F<"EL">;
 def: JoinedOrSeparate<["-"], "G">;
 def: F<"Qy">;
 
 // Hidden option used for testing MIPS multi-GOT implementation.
 defm mips_got_size:
   Eq<"mips-got-size", "Max size of a single MIPS GOT. 0x10000 by default.">,
   Flags<[HelpHidden]>;

ELF/Relocations.cpp

static bool maybeReportUndefined(Symbol &Sym, InputSectionBase &Sec,
   if (Sym.isLocal() || !Sym.isUndefined() || Sym.isWeak())
     return false;
 
   bool CanBeExternal =
       Sym.computeBinding() != STB_LOCAL && Sym.Visibility == STV_DEFAULT;
   if (Config->UnresolvedSymbols == UnresolvedPolicy::Ignore && CanBeExternal)
     return false;
 
+  if (Config->WarnOnce) {
+    if (Sym.Warned)
+      return false;
+    Sym.Warned = true;
+  }
+
   std::string Msg =
       "undefined symbol: " + toString(Sym) + "\n>>> referenced by ";
 
   std::string Src = Sec.getSrcMsg(Sym, Offset);
   if (!Src.empty())
     Msg += Src + "\n>>>               ";
   Msg += Sec.getObjMsg(Offset);
 

ELF/Symbols.h

 
 protected:
   Symbol(Kind K, InputFile *File, StringRefZ Name, uint8_t Binding,
          uint8_t StOther, uint8_t Type)
       : File(File), NameData(Name.Data), NameSize(Name.Size), Binding(Binding),
         Type(Type), StOther(StOther), SymbolKind(K), NeedsPltAddr(false),
         IsInIplt(false), IsInIgot(false), IsPreemptible(false),
         Used(!Config->GcSections), NeedsTocRestore(false),
-        ScriptDefined(false) {}
+        ScriptDefined(false), Warned(false) {}
 
 public:
   // True the symbol should point to its PLT entry.
   // For SharedSymbol only.
   unsigned NeedsPltAddr : 1;
 
   // True if this symbol is in the Iplt sub-section of the Plt.
   unsigned IsInIplt : 1;
public:
 
   // True if a call to this symbol needs to be followed by a restore of the
   // PPC64 toc pointer.
   unsigned NeedsTocRestore : 1;
 
   // True if this symbol is defined by a linker script.
   unsigned ScriptDefined : 1;
 
+  // True if this symbol is already reported as undefined. This
+  // bit is used for --warn-once option.
+  unsigned Warned : 1;
+
   bool isSection() const { return Type == llvm::ELF::STT_SECTION; }
   bool isTls() const { return Type == llvm::ELF::STT_TLS; }
   bool isFunc() const { return Type == llvm::ELF::STT_FUNC; }
   bool isGnuIFunc() const { return Type == llvm::ELF::STT_GNU_IFUNC; }
   bool isObject() const { return Type == llvm::ELF::STT_OBJECT; }
   bool isFile() const { return Type == llvm::ELF::STT_FILE; }
 };
 

test/ELF/warn-once.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: not ld.lld -warn-once %t.o -o %t.exe 2>&1 | FileCheck %s
+
+# CHECK-COUNT-1: error: undefined symbol: foo
+# CHECK-COUNT-1: >>> referenced by warn-once.s
+
+.file "warn-once.s"
+
+  .globl _start
+_start:
+  call foo
+  call foo