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.

[jhenderson]

I'm not sure about the ICF behaviour change. If a user has two functions, and wants them to appear in two very different locations in the order file for some reason, ICF will silently break this order. Example order file:

identical1
not_identical
identical2

With ICF enabled, identical1 and identical2 will be placed at the same location in the output (as is normal for ICF), and so there will not be one at the start and one at the end as expected. Therefore, we haven't created the order that was explicitly asked for. I think therefore it is reasonable to warn still in this case.

The flip-side is that if only one of identical1 or identical2 is specified in the order file, then we should probably not warn, since it is possible to order the symbols in this case! One possible solution is to record all symbols in the order file in sections which have been discarded due to ICF, then after completing the loop, looking for any such symbols whose section doesn't have the Priority that would be expected. It's not an ideal solution though.

lld/ELF/Writer.cpp

1048 ↗

(On Diff #137292)

We should probably avoid using "Warn" for this name, as it is too easy for a developer to accidentally call the wrong function (e.g. via type), and bypass the check. Perhaps "WarnOrder" or similar.

[ruiu]

I don't want to do stuff that is too clever, and I'm OK in either warning on it and don't sort, or don't warn on it and do sort. I just want to fix the issue that we currently tell user that the section is ignored but actually be sorted.

To keep things simple, I'd suggest we do warn on ICF'ed functions and skip them when we see them in the loop. Does this sound OK?

[ruiu]

• print out a different error message for ICF'ed symbols
• do not sort ICF'ed symbols

[• rafael]

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

I'm sorry I accidentally submitted this change along with some other
change. I'll create another patch now to address the review comments.

NP. If you could revert r326911 and have the new patch on top of that it
would probably be the least confusing.

Cheers,
Rafael

[ruiu]

I'm sorry I accidentally submitted this change along with some other
change. I'll create another patch now to address the review comments.

[• rafael]

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

ruiu added a comment.

I don't want to do stuff that is too clever, and I'm OK in either warning on it and don't sort, or don't warn on it and do sort. I just want to fix the issue that we currently tell user that the section is ignored but actually be sorted.

To keep things simple, I'd suggest we do warn on ICF'ed functions and skip them when we see them in the loop. Does this sound OK?

I think your original idea of not warning is better.

On the thread about call graph based sorting pcc reported a win by
taking ICF into consideration. If there are multiple symbols in a
section using the highest priority symbol as the priority of the section
seems like the best solution for me.

I would recommend going with something like your original patch and then
having an independent discussion on whether we should add a warning when
two symbols are in the same section and would result in different
priorities.

Cheers,
Rafael

[• rafael]

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

Overall direction and ICF related change looks good.

• if (auto *Sec = dyn_cast_or_null<InputSectionBase>(D->Section)) {
• int &Priority = SectionOrder[cast<InputSectionBase>(Sec->Repl)];
• Priority = std::min(Priority, Ent.Priority);

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

This will fire for symbols like _GLOBAL_OFFSET_TABLE_ that point to an
OutputSection, no?

Cheers,
Rafael

[• rafael]

For giving a user as much information as possible we would warn only
when an ICFed section causes a symbol to not be at the desired location.

But note that we don't even warn when two symbols are in the same
section:

.text
.global foo
foo:
.global bar
bar:

The symbols foo and bar will always have the same values, regardless of
where they are in the order file.

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.

Cheers,
Rafael

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

jhenderson added a comment.

I'm not sure about the ICF behaviour change. If a user has two functions, and wants them to appear in two very different locations in the order file for some reason, ICF will silently break this order. Example order file:

identical1
not_identical
identical2

With ICF enabled, identical1 and identical2 will be placed at the same location in the output (as is normal for ICF), and so there will not be one at the start and one at the end as expected. Therefore, we haven't created the order that was explicitly asked for. I think therefore it is reasonable to warn still in this case.

The flip-side is that if only one of identical1 or identical2 is specified in the order file, then we should probably not warn, since it is possible to order the symbols in this case! One possible solution is to record all symbols in the order file in sections which have been discarded due to ICF, then after completing the loop, looking for any such symbols whose section doesn't have the Priority that would be expected. It's not an ideal solution though.

Comment at: lld/ELF/Writer.cpp:1048

• 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 Warn = [&](StringRef Msg) {

+ if (Config->WarnSymbolOrdering)

We should probably avoid using "Warn" for this name, as it is too easy for a developer to accidentally call the wrong function (e.g. via type), and bypass the check. Perhaps "WarnOrder" or similar.

https://reviews.llvm.org/D44180

---

llvm-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

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;
+      }
+      if (!Sec->Live) {
+        if (Sec->Repl == Sec)
+          Warning("unable to order discarded symbol");
+        else
+          Warning("unable to order a symbol merged by ICF");
         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;
 }

test/ELF/symbol-ordering-file-icf.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 # RUN: echo "zed" > %t.order
 # RUN: echo "bar" >> %t.order
 # RUN: echo "foo" >> %t.order
 # RUN: ld.lld --icf=all --symbol-ordering-file %t.order -shared %t.o -o %t.so
 # RUN: llvm-nm %t.so | FileCheck %s
 
-## Check that after ICF merges 'foo' and 'zed' we still
-## place them before 'bar', in according to ordering file.
-# CHECK-DAG: 0000000000001000 T foo
-# CHECK-DAG: 0000000000001000 T zed
-# CHECK-DAG: 0000000000001004 T bar
+## Check that we do not sort ICF'ed symbols 'foo' and 'zed'.
+# CHECK-DAG: 0000000000001000 T bar
+# CHECK-DAG: 0000000000001004 T foo
+# CHECK-DAG: 0000000000001004 T zed
 
 .section .text.foo,"ax",@progbits
 .align 4
 .global foo
 foo:
   retq
 
 .section .text.bar,"ax",@progbits

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

 # 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
+# ICF:         warning: {{.*}}1.o: unable to order a symbol merged by ICF: 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