Improve --warn-symbol-ordering.

[ruiu]

Summary

I originally tried to simplify code and then noticed that lld doesn't
do what it tells to the user by warn(). It says "unable to order
discarded symbol" but it actually can for sections eliminated by ICF.
With this patch, lld doesn't sort such sections.

[jhenderson]

LGTM (with nit). Having thought about it, I agree with this comment by @espindola:

I am OK with warning on both cases, but it feels like that would be a
separate warning. A non existing symbol showing up in the order file
probably means there is something wrong with how that files was
created. Two symbols that can't be independently moved because they are
in the same section (because of ICF or not) seems like a normal event in
most cases.

I also think it would be fairly simple to add - just detect if the priority was the default or not prior to changing it (though we'd need more info to get a better warning message. I don't mind this being spun off into a separate discussion.

P.S. Rafael - Can you confirm which of the two usernames you'd like us to use, please?

lld/ELF/Writer.cpp

1062

clang-format?

[• rafael]

Rui Ueyama via Phabricator <[e-mail]> writes:

+ auto *Sec = dyn_cast_or_null<InputSectionBase>(cast<Defined>(Sym)->Section);
+ if (!Sec) {
+ Warning("unable to order absolute symbol");
+ continue;

}

This warning will now fire for some linker created symbols like
_GLOBAL_OFFSET_TABLE_, no?

Cheers,
Rafael

[• rafael]

James Henderson via Phabricator <[e-mail]> writes:

jhenderson added a reviewer: espindola.
jhenderson accepted this revision.
jhenderson added a subscriber: espindola.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM (with nit). Having thought about it, I agree with this comment by @espindola:

I am OK with warning on both cases, but it feels like that would be a
separate warning. A non existing symbol showing up in the order file
probably means there is something wrong with how that files was
created. Two symbols that can't be independently moved because they are
in the same section (because of ICF or not) seems like a normal event in
most cases.

I also think it would be fairly simple to add - just detect if the priority was the default or not prior to changing it (though we'd need more info to get a better warning message. I don't mind this being spun off into a separate discussion.

P.S. Rafael - Can you confirm which of the two usernames you'd like us to use, please?

The "espindola" account. I am terribly sorry for the noise. I wanted to
convert the older account to not use a google sign in but could not find
out how to do it.

Cheers,
Rafael

[• espindola]

lld/ELF/Writer.cpp

1063

This will now fire on symbols the linker defines relative to an output section, no?

[jhenderson]

lld/ELF/Writer.cpp

1063

That would be a good thing, right? Attempting to order a symbol not associated with an OutputSection (whether linker-defined or not) would be impossible.

[ruiu]

lld/ELF/Writer.cpp

1063

Yeah, I think this is a desired behavior.

[• espindola]

lld/ELF/Writer.cpp

1063

Warning would be good, calling it an absolute symbol in the warning is the issue.

[• rafael]

This will now fire on symbols the linker defines relative to an output section, no?

That would be a good thing, right? Attempting to order a symbol not associated with an OutputSection (whether linker-defined or not) would be impossible.

Yeah, I think this is a desired behavior.

Warning would be good, calling it an absolute symbol in the warning is the issue.

The attached patch on top of your patch shows the
problem. _GLOBAL_OFFSET_TABLE_ is not an absolute symbol.

• t.diff1 KBDownload
• msg-9335-260.txt18 BDownload

[jhenderson]

lld/ELF/Writer.cpp

1063

Ah, I see what you mean. Perhaps update the message to "absolute or linker-defined symbol"?

This wouldn't be strictly true, since if the linker were to define an input section-relative symbol it could reorder it (do we ever do this?), but should be clear enough.

[• espindola]

lld/ELF/Writer.cpp

1063

True. I was thinking about having two different messages, but just rewording is sufficient.

LGTM with the error updated.

[• espindola]

Rui, are you still working on this?

lld/ELF/Writer.cpp

static DenseMap<const InputSectionBase *, int> buildSectionOrder() {
   for (InputFile *File : ObjectFiles) {
     for (Symbol *Sym : File->getSymbols()) {
       auto It = SymbolOrder.find(Sym->getName());
       if (It == SymbolOrder.end())
         continue;
       SymbolOrderEntry &Ent = It->second;
       Ent.Present = true;
 
-      auto *D = dyn_cast<Defined>(Sym);
-      if (Config->WarnSymbolOrdering) {
-        if (Sym->isUndefined())
-          warn(File->getName() +
-               ": unable to order undefined symbol: " + Sym->getName());
-        else if (Sym->isShared())
-          warn(File->getName() +
-               ": unable to order shared symbol: " + Sym->getName());
-        else if (D && !D->Section)
-          warn(File->getName() +
-               ": unable to order absolute symbol: " + Sym->getName());
-        else if (D && !D->Section->Live)
-          warn(File->getName() +
-               ": unable to order discarded symbol: " + Sym->getName());
+      auto Warning = [&](StringRef Msg) {
+        if (Config->WarnSymbolOrdering)
+          warn(File->getName() + ": " + Msg + ": " + Sym->getName());
+      };
+
+      if (Sym->isUndefined()) {
+        Warning("unable to order undefined symbol");
+        continue;
+      }
+      if (Sym->isShared()) {
+        Warning("unable to order shared symbol");
+        continue;
       }
-      if (!D)
+
+      auto *Sec = dyn_cast_or_null<InputSectionBase>(cast<Defined>(Sym)->Section);
+      if (!Sec) {
+        Warning("unable to order absolute symbol");
+        continue;
+      }
+
+      // Skip sections eliminated by GC, but we don't want to skip
+      // ICFed functions.
+      if (!Sec->Live && Sec->Repl == Sec) {
+        Warning("unable to order discarded symbol");
         continue;
+      }
 
-      if (auto *Sec = dyn_cast_or_null<InputSectionBase>(D->Section)) {
-        int &Priority = SectionOrder[cast<InputSectionBase>(Sec->Repl)];
-        Priority = std::min(Priority, Ent.Priority);
-      }
-    }
-  }
 
   if (Config->WarnSymbolOrdering)
     for (auto OrderEntry : SymbolOrder)
       if (!OrderEntry.second.Present)
         warn("symbol ordering file: no such symbol: " + OrderEntry.first);
 
   return SectionOrder;
 }

lld/test/ELF/symbol-ordering-file-warnings.s

 # RUN: echo "gc" > %t-order-gc.txt
 # RUN: ld.lld %t1.o -o %t --symbol-ordering-file %t-order-gc.txt --gc-sections \
 # RUN:   --unresolved-symbols=ignore-all 2>&1 | FileCheck %s --check-prefixes=WARN,GC
 
 # Check that a warning is emitted for the symbol removed due to --icf.
 # RUN: echo "icf1" > %t-order-icf.txt
 # RUN: echo "icf2" >> %t-order-icf.txt
 # RUN: ld.lld %t1.o -o %t --symbol-ordering-file %t-order-icf.txt --icf=all \
-# RUN:   --unresolved-symbols=ignore-all 2>&1 | FileCheck %s --check-prefixes=WARN,ICF
+# RUN:   --unresolved-symbols=ignore-all --fatal-warnings
 
 # Check that a warning is emitted for symbols discarded due to a linker script /DISCARD/ section.
 # RUN: echo "discard" > %t-order-discard.txt
 # RUN: echo "SECTIONS { /DISCARD/ : { *(.text.discard) } }" > %t.script
 # RUN: ld.lld %t1.o -o %t --symbol-ordering-file %t-order-discard.txt -T %t.script \
 # RUN:   --unresolved-symbols=ignore-all 2>&1 | FileCheck %s --check-prefixes=WARN,DISCARD
 
 # Check that LLD does not warn for discarded COMDAT symbols, if they are present in the kept instance.

 # GC:          warning: {{.*}}1.o: unable to order discarded symbol: gc
 # WARN-NOT:    warning:
 # SHARED:      warning: {{.*}}1.o: unable to order shared symbol: shared
 # WARN-NOT:    warning:
 # UNDEFINED:   warning: {{.*}}3.o: unable to order undefined symbol: undefined
 # WARN-NOT:    warning:
 # MISSING:     warning: symbol ordering file: no such symbol: missing
 # MISSING2:    warning: symbol ordering file: no such symbol: missing_sym
-# ICF:         warning: {{.*}}1.o: unable to order discarded symbol: icf2
 # COMDAT:      warning: {{.*}}1.o: unable to order discarded symbol: comdat
 # COMDAT-NEXT: warning: {{.*}}2.o: unable to order discarded symbol: comdat
 # MULTI:       warning: {{.*}}2.o: unable to order absolute symbol: multi
 # MULTI-NEXT:  warning: {{.*}}3.o: unable to order undefined symbol: multi
 # WARN-NOT:    warning:
 
 absolute = 0x1234