[ELF] - Fix case of using both --icf and --symbol-ordering-file together.

[grimar]

Summary

Imagine that we have sections A, B, C, where A == C and
symbol ordering file containing symbols: symC, symB, symA

Previously because of ICF it was possible that final order would be
B, A or B, C. That violates order specified in ordering file.
Patch changes that.

Alternative can be to disable ICF for ordered sections. Not sure what
is better. May be we should do that instead ?

[ruiu]

I think that this patch isn't necessarily wrong but perhaps not desirable. Why did you pick the highest priority instead of the lowest priority, for example? The stuff you want to achieve with --symbol-ordering-file is to force some layout, and if your file contains duplicate entries due to ICF, you can't achieve your goal.

I think that you create a symbol ordering file from profiler output. COMDAT names that the profiler uses is the one that is selected by ICF, no? So, the generated symbol ordering file naturally uses the COMDAT names that were selected by ICF. So I don't think we need to do anything about it.

[jhenderson]

In D43234#1007032, @ruiu wrote:

I think that you create a symbol ordering file from profiler output. COMDAT names that the profiler uses is the one that is selected by ICF, no? So, the generated symbol ordering file naturally uses the COMDAT names that were selected by ICF. So I don't think we need to do anything about it.

I think this is the normal use case, but I don't think it's the only use case, so I'm not sure we can assume that these files have always been auto-generated. I think that warning for the removed symbol is probably sufficient though, to alert users to something going wrong. This is the current behaviour, following my earlier change, and see also D43336, where I realised that it should be tested explicitly. One counter-argument to this is that profilers may not load the symbols in symbol table order, so this means that they could load the "discarded" symbol first (since it appears in the symbol table), and try to add it to the order file, instead of the selected one, resulting in warnings for what should seem like a perfectly good input on first look.

I think the only way we can avoid warnings in both cases is to disable --icf with --symbol-ordering-file, at least for the contents of that file (symbols in the ordering file could still be in "selected" sections, but not removed ones). I'm not sure if this is ideal either though.

[grimar]

• Addressed comment.

[• espindola]

LGTM.

Please land this. It is now trivial and there is always post commit review.

ELF/Writer.cpp

for (Symbol *Sym : File->getSymbols()) {
         else if (D && !D->Section->Live)
           warn(File->getName() +
                ": unable to order discarded symbol: " + Sym->getName());
       }
       if (!D)
         continue;
 
       if (auto *Sec = dyn_cast_or_null<InputSectionBase>(D->Section)) {
-        int &Priority = SectionOrder[Sec];
+        int &Priority = SectionOrder[cast<InputSectionBase>(Sec->Repl)];
         Priority = std::min(Priority, Ent.Priority);
       }
     }
   }
 
   if (Config->WarnSymbolOrdering)
     for (auto OrderEntry : SymbolOrder)
       if (!OrderEntry.second.Present)

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
+
+.section .text.foo,"ax",@progbits
+.align 4
+.global foo
+foo:
+  retq
+
+.section .text.bar,"ax",@progbits
+.align 4
+.global bar
+bar:
+  nop
+  retq
+
+.section .text.zed,"ax",@progbits
+.align 4
+.global zed
+zed:
+  retq