[PATCH] D53044: [ELF] Don't warn on undefined symbols if UnresolvedPolicy::Ignore is used

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Symbols.cpp:280-288
     Warn(": unable to order shared symbol: ");
-  else if (D && !D->Section)
-    Warn(": unable to order absolute symbol: ");
-  else if (D && isa<OutputSection>(D->Section))
-    Warn(": unable to order synthetic symbol: ");
-  else if (D && !D->Section->Repl->Live)
-    Warn(": unable to order discarded symbol: ");
+  } else if (D) {
+    if (!D->Section)
+      Warn(": unable to order absolute symbol: ");
+    else if (isa<OutputSection>(D->Section))
+      Warn(": unable to order synthetic symbol: ");
+    else if (!D->Section->Repl->Live)
----------------
MaskRay wrote:
> MaskRay wrote:
> > ruiu wrote:
> > > MaskRay wrote:
> > > > ruiu wrote:
> > > > > Please do not change this -- this code was written in the way it is to reduce indentation depth.
> > > > Should I use `if (Sym->isUndefined() && Config->UnresolvedSymbols != UnresolvedPolicy::Ignore)` then?
> > > > 
> > > > I actually find dispatching by `SymbolKind` first makes the code a bit easier to understand:
> > > > 
> > > > * Undefined
> > > > * Shared
> > > > * Others (Defined)
> > > > 
> > > > For the `Defined` case, fine-grained conditions are checked for absolute/syunthetic/discarded.
> > > > 
> > > > I lean to this style but if you have strong opinion on this, I can change it.
> > > Yeah, maybe you should write that `if` at the beginning of this function to exit early.
> > But that way:
> > 
> > ```
> >   if (Sym->isUndefined() && Config->UnresolvedSymbols != UnresolvedPolicy::Ignore)
> >     Warn(": unable to order undefined symbol: ");
> >   else if (Sym->isShared())
> >     Warn(": unable to order undefined symbol: ");
> >   else if (Sym->isShared())
> >     Warn(": unable to order shared symbol: ");
> > ////// Sym->isUndefined() is possible here
> >   else if (D && !D->Section)
> >     Warn(": unable to order absolute symbol: ");
> >   else if (D && isa<OutputSection>(D->Section))
> >     Warn(": unable to order synthetic symbol: ");
> >   else if (D && !D->Section->Repl->Live)
> >     Warn(": unable to order discarded symbol: ");
> > ```
> > 
> > Will that cause a problem?
> Sorry I misread your words. I think you mean:
> 
> ```
> void elf::warnUnorderableSymbol(const Symbol *Sym) {
>   if (!Config->WarnSymbolOrdering ||
>       Config->UnresolvedSymbols == UnresolvedPolicy::Ignore)
>     return;
> ```
> 
> But this way `UnresolvedPolicy::Ignore` also changes how `Shared` `Defined` symbols are interpreted. `UnresolvedPolicy::Ignore` is also used otherwhere, but only related to how undefined symbols are interpreted.
I mean you can exit early if `Sym->isUndefined() && Config->UnresolvedSymbols != UnresolvedPolicy::Ignore`, can't you?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53044