[ELF][Driver] Fix precedence of symbol ordering file and CGProfile

[tcwang]

Summary

This patch is a fix for https://bugs.llvm.org/show_bug.cgi?id=41804.
We try to solve the precedence of user-specified symbol ordering file and C3 ordering provided as call graph. It deals with two case:
(1) When both --symbol-ordering-file=<file> and --call-graph-order-file=<file> are present, whichever flag comes later will take precedence.
(2) When only --symbol-ordering-file=<file> is present, it takes precedence over implicit call graph (CGProfile) generated by CGProfilePass enabled in new pass manager.

[ruiu]

(1) When both --symbol-ordering-file=<file> and --call-graph-order-file=<file> are present, whichever flag comes later will take precedence.

It looks like this combination is a misuse of the feature. Instead of silently ignore a flag, I think we should report an error that these flags cannot be used together.

(2) When only --symbol-ordering-file=<file> is present, it takes precedence over implicit call graph (CGProfile) generated by CGProfilePass enabled in new pass manager.

This is fine by me.

[tcwang]

This patch is a fix for https://bugs.llvm.org/show_bug.cgi?id=41804.
We try to solve the precedence of user-specified symbol ordering file and C3 ordering provided as call graph. It deals with two case:
(1) When both --symbol-ordering-file=<file> and --call-graph-order-file=<file> are present, LLD reports an error and aborts.
(2) When only --symbol-ordering-file=<file> is present, it takes precedence over implicit call graph (CGProfile) generated by CGProfilePass enabled in new pass manager.

[tcwang]

In D61711#1495928, @ruiu wrote:

(1) When both --symbol-ordering-file=<file> and --call-graph-order-file=<file> are present, whichever flag comes later will take precedence.

It looks like this combination is a misuse of the feature. Instead of silently ignore a flag, I think we should report an error that these flags cannot be used together.

(2) When only --symbol-ordering-file=<file> is present, it takes precedence over implicit call graph (CGProfile) generated by CGProfilePass enabled in new pass manager.

This is fine by me.

Thanks! Fixed the patch with the suggestions.

[ruiu]

lld/ELF/Driver.cpp

968–970

Check for invalid combination of flags are usually done in checkOptions(). Can you write a check for this combination there?

[MaskRay]

lld/test/ELF/symbol-ordering-file-cgprofile-conflicts.s

3

Nit: name the object file %t.o, the executable %t

32

rm -f. -f suppresses the error if the file doesn't exist.

68

Delete the empty line.

[tcwang]

Fix comments on the unit test.

[tcwang]

lld/ELF/Driver.cpp

968–970

checkOptions() function doesn't take any arguments in but I need Args to check if OPT_call_graph_ordering_file is present or not. It's because the flag is read very late in the stage (just before writeResult<ELFT>() ) and not saved to any Config field, I have to use Args to directly check its presence. Whereas in checkOptions(), all other checkings are done by checking Config fields. Also, if I do check in checkOptions(), which happens after readConfigs(), it needs to read in the file even if an error will be reported later?

[tcwang]

Friendly ping on the patch.

[ruiu]

lld/ELF/Driver.cpp

968–970

OK, but please make the wording consistent with other error messages. If you look at checkOptions, you'd notice that we use the same wording for reporting incompatible option combinations.

[tcwang]

Fix comments in options checking.

[tcwang]

lld/ELF/Driver.cpp

968–970

Yeah, that makes sense. Thanks for pointing out.

[ruiu]

LGTM

[MaskRay]

The commit description of rLLD361190 does not contain Differential Revision: https://reviews.llvm.org/D61711, so the revision was not closed when you committed that...

Also, the description should probably say the two options are now exclusive:

(1) When both --symbol-ordering-file=<file> and --call-graph-order-file=<file> are present, whichever flag comes later will take precedence.

[tcwang]

In D61711#1509397, @MaskRay wrote:

The commit description of rLLD361190 does not contain Differential Revision: https://reviews.llvm.org/D61711, so the revision was not closed when you committed that...

Also, the description should probably say the two options are now exclusive:

(1) When both --symbol-ordering-file=<file> and --call-graph-order-file=<file> are present, whichever flag comes later will take precedence.

Thank you! I'll keep in mind to add the differential revision to LLVM commit next time. Also, is it possible to change the commit message on the other commits now? Sorry that I forgot to change them...

lld/ELF/Driver.cpp

static void readConfigs(opt::InputArgList &Args) {
   if (Config->Omagic)
     Config->ZRelro = false;
 
   std::tie(Config->BuildId, Config->BuildIdVector) = getBuildId(Args);
 
   std::tie(Config->AndroidPackDynRelocs, Config->RelrPackDynRelocs) =
       getPackDynRelocs(Args);
 
-  if (auto *Arg = Args.getLastArg(OPT_symbol_ordering_file))
-    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
+  if (auto *Arg = Args.getLastArg(OPT_symbol_ordering_file)){
+    if (Args.hasArg(OPT_call_graph_ordering_file))
+      error("--symbol-ordering-file should NOT be used together"
+            " with --call-graph-order-file");
+    if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue())){
       Config->SymbolOrderingFile = getSymbolOrderingFile(*Buffer);
+      // Also need to disable CallGraphProfileSort to prevent
+      // LLD order symbols with CGProfile
+      Config->CallGraphProfileSort = false;
+    }
+  }
 
   // If --retain-symbol-file is used, we'll keep only the symbols listed in
   // the file and discard all others.
   if (auto *Arg = Args.getLastArg(OPT_retain_symbols_file)) {
     Config->DefaultSymbolVersion = VER_NDX_LOCAL;
     if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
       for (StringRef S : args::getLines(*Buffer))
         Config->VersionScriptGlobals.push_back(

lld/test/ELF/symbol-ordering-file-cgprofile-conflicts.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+// RUN: ld.lld -e A %t.o --no-call-graph-profile-sort -o %t
+// RUN: llvm-nm --numeric-sort %t | FileCheck %s --check-prefix=NO_ORDERING
+// NO_ORDERING:         0000000000201000 t D
+// NO_ORDERING-NEXT:    0000000000201001 T C
+// NO_ORDERING-NEXT:    0000000000201002 T B
+// NO_ORDERING-NEXT:    0000000000201003 T A  
+
+// RUN: ld.lld -e A %t.o -o %t
+// RUN: llvm-nm --numeric-sort %t | FileCheck %s --check-prefix=CALL_GRAPH
+// CALL_GRAPH:    0000000000201000 T A
+// CALL_GRAPH-NEXT: 0000000000201000 t Aa
+// CALL_GRAPH-NEXT: 0000000000201001 T B
+// CALL_GRAPH-NEXT: 0000000000201002 T C
+// CALL_GRAPH-NEXT: 0000000000201003 t D
+
+// RUN: rm -f %t.symbol_order
+// RUN: echo "C" >> %t.symbol_order
+// RUN: echo "B" >> %t.symbol_order
+// RUN: echo "D" >> %t.symbol_order
+// RUN: echo "A" >> %t.symbol_order
+
+// RUN: ld.lld -e A %t.o --symbol-ordering-file %t.symbol_order -o %t
+// RUN: llvm-nm --numeric-sort %t | FileCheck %s --check-prefix=SYMBOL_ORDER
+// SYMBOL_ORDER:  0000000000201000 T C
+// SYMBOL_ORDER-NEXT: 0000000000201001 T B
+// SYMBOL_ORDER-NEXT: 0000000000201002 t D
+// SYMBOL_ORDER-NEXT: 0000000000201003 T A
+
+// RUN: rm -f %t.call_graph
+// RUN: echo "A B 5" > %t.call_graph
+// RUN: echo "B C 50" >> %t.call_graph
+// RUN: echo "C D 40" >> %t.call_graph
+// RUN: echo "D B 10" >> %t.call_graph
+
+// RUN: not ld.lld -e A %t.o --symbol-ordering-file %t.symbol_order --call-graph-ordering-file %t.call_graph -o %t 2>&1 | FileCheck %s
+// RUN: not ld.lld -e A %t.o --call-graph-ordering-file %t.call_graph --symbol-ordering-file %t.symbol_order -o %t 2>&1 | FileCheck %s
+// CHECK: error: --symbol-ordering-file should NOT be used together with --call-graph-order-file
+
+    .section    .text.D,"ax",@progbits
+D:
+    retq
+
+    .section    .text.C,"ax",@progbits
+    .globl  C
+C:
+    retq
+
+    .section    .text.B,"ax",@progbits
+    .globl  B
+B:
+    retq
+
+    .section    .text.A,"ax",@progbits
+    .globl  A
+A:
+Aa:
+    retq
+
+    .cg_profile A, B, 10
+    .cg_profile A, B, 10
+    .cg_profile Aa, B, 80
+    .cg_profile A, C, 40
+    .cg_profile B, C, 30
+    .cg_profile C, D, 90
+