[ELF] Add --{,no-}call-graph-profile-sort (enabled by default)

[MaskRay]

Summary

Add an option to disable sorting sections with call graph profile

[MaskRay]

Two tests

[Bigcheese]

I really like adding the option to disable it, but I'm not sure about disabling it by default. Is there any situation where the profile is present where a user wouldn't want to perform layout? The algorithm is very fast so unexpected link time increase isn't really an issue, and the exact order of sections already isn't guaranteed by ELF and differs between linkers so I don't expect there to be correctness issues.

[ruiu]

Speaking of whether we should enable or disable it by default, I second Michael. In most cases, if your object files contain .llvm.call-graph-profile sections, that's because you are instructed clang/llvm to do so, and if lld doesn't reorder sections in that case, most users wonder why lld doesn't work "correctly". From that UX perspective, that should be enabled by default.

[MaskRay]

enable by default

[MaskRay]

Update title

[ruiu]

ELF/Driver.cpp

789

nit: you probably should spend a bit more time to read existing code around the location where you add new code to make your new code consistent with existing one. All the other Config members have the exact same name with a corresponding command line flag. For example, Config->EhFrameHdr corresponds to --eh-frame-hdr and Config->EmitRelocs corresponds to --emit-relocs. Please make your new code consistent with existing code.

[MaskRay]

ELF/Driver.cpp

789

I know, but the variable CallGraphProfile is already used as a map, what should I rename it to?

[MaskRay]

Name the bool Config->CallGraphProfile and rename existing COnfig->CallGraphProfile to CallGraphInfo

ELF/Driver.cpp

789

bool CallGraphProfile;
llvm::MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>,
                  uint64_t>
    CallGraphInfo;

Let me know if you have other preference

[ruiu]

ELF/Driver.cpp

789

Yeah you should, because you essentially should keep code beautiful/readable for those who read this code for the first time. If in doubt, forget about history of code and see new code with fresh eye. I'm sure you'll find this code inconsistent and odd.

[Bigcheese]

ELF/Driver.cpp

789

This is a much less descriptive name. I would call the option {-no}-call-graph-profile-sort as you are disabling the sorting based on the profile, not the profile itself. Then the option name becomes CallGraphProfileSort.

[ruiu]

I like that flag name.

[MaskRay]

Use option name suggested by Bigcheese

[MaskRay]

Revert a comment change

[ruiu]

ELF/CallGraphSort.cpp

99

Unrelated change?

[MaskRay]

.

[MaskRay]

ELF/CallGraphSort.cpp

99

Sorry I noticed it in my "Revert a comment change" but maybe that command failed

[ruiu]

Looks good. Michael?

[MaskRay]

Change the text file from cgprofile-obj-warn.s to cgprofile-obj.s as I will delete cgprofile-obj-warn.s in https://reviews.llvm.org/D53669

Use llvm-nm instead of llvm-readobj to check symbol addresses

[MaskRay]

Rebase as it would otherwise have to touch file touched by D53669

[MaskRay]

Use llvm-nm --no-sort

[Bigcheese]

lgtm

ELF/CallGraphSort.cpp

 
 // Maximum cluster size in bytes.
 constexpr uint64_t MAX_CLUSTER_SIZE = 1024 * 1024;
 } // end anonymous namespace
 
 typedef std::pair<const InputSectionBase *, const InputSectionBase *>
     SectionPair;
 
-// Take the edge list in Config->CallGraphProfile, resolve symbol names to
+// Take the edge list in Config->CallGraphInfo, resolve symbol names to
 // Symbols, and generate a graph between InputSections with the provided
 // weights.
 CallGraphSort::CallGraphSort() {
   MapVector<SectionPair, uint64_t> &Profile = Config->CallGraphProfile;
   DenseMap<const InputSectionBase *, int> SecToCluster;
 
   auto GetOrCreateNode = [&](const InputSectionBase *IS) -> int {
     auto Res = SecToCluster.insert(std::make_pair(IS, Clusters.size()));

ELF/Config.h

struct Configuration {
   bool AllowMultipleDefinition;
   bool AndroidPackDynRelocs;
   bool ARMHasBlx = false;
   bool ARMHasMovtMovw = false;
   bool ARMJ1J2BranchEncoding = false;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
+  bool CallGraphProfileSort;
   bool CheckSections;
   bool CompressDebugSections;
   bool Cref;
   bool DefineCommon;
   bool Demangle = true;
   bool DisableVerify;
   bool EhFrameHdr;
   bool EmitRelocs;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->Demangle = Args.hasFlag(OPT_demangle, OPT_no_demangle, true);
   Config->DisableVerify = Args.hasArg(OPT_disable_verify);
   Config->Discard = getDiscard(Args);
   Config->DwoDir = Args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);
   Config->DynamicLinker = getDynamicLinker(Args);
   Config->EhFrameHdr =
       Args.hasFlag(OPT_eh_frame_hdr, OPT_no_eh_frame_hdr, false);
   Config->EmitRelocs = Args.hasArg(OPT_emit_relocs);
+  Config->CallGraphProfileSort = Args.hasFlag(
+      OPT_call_graph_profile_sort, OPT_no_call_graph_profile_sort, true);
   Config->EnableNewDtags =
       Args.hasFlag(OPT_enable_new_dtags, OPT_disable_new_dtags, true);
   Config->Entry = Args.getLastArgValue(OPT_entry);
   Config->ExecuteOnly =
       Args.hasFlag(OPT_execute_only, OPT_no_execute_only, false);
   Config->ExportDynamic =
       Args.hasFlag(OPT_export_dynamic, OPT_no_export_dynamic, false);
   Config->FilterList = args::getStrings(Args, OPT_filter);
template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   demoteSharedSymbols<ELFT>();
   mergeSections();
   if (Config->ICF != ICFLevel::None) {
     findKeepUniqueSections<ELFT>(Args);
     doIcf<ELFT>();
   }
 
   // Read the callgraph now that we know what was gced or icfed
+  if (Config->CallGraphProfileSort) {
     if (auto *Arg = Args.getLastArg(OPT_call_graph_ordering_file))
       if (Optional<MemoryBufferRef> Buffer = readFile(Arg->getValue()))
         readCallGraph(*Buffer);
     readCallGraphsFromObjectFiles<ELFT>();
+  }
 
   // Write the result to the file.
   writeResult<ELFT>();
 }

ELF/Options.td

 
 defm as_needed: B<"as-needed",
     "Only set DT_NEEDED for shared libraries if used",
     "Always set DT_NEEDED for shared libraries (default)">;
 
 defm call_graph_ordering_file:
   Eq<"call-graph-ordering-file", "Layout sections to optimize the given callgraph">;
 
+defm call_graph_profile_sort: B<"call-graph-profile-sort",
+    "Reorder sections with call graph profile (default)",
+    "Do not reorder sections with call graph profile">;
+
 // -chroot doesn't have a help text because it is an internal option.
 def chroot: Separate<["--", "-"], "chroot">;
 
 def color_diagnostics: F<"color-diagnostics">,
   HelpText<"Alias for --color-diagnostics=always">;
 
 def color_diagnostics_eq: J<"color-diagnostics=">,
   HelpText<"Use colors in diagnostics">,

test/ELF/cgprofile-obj-warn.s

A:
     .cg_profile A, C, 40
     .cg_profile B, C, 30
     .cg_profile adena1, A, 30
     .cg_profile A, adena2, 30
     .cg_profile poppy, A, 30
 
 # CHECK: unable to order absolute symbol: B
 
-# RUN: ld.lld %t -o /dev/null \
-# RUN:   -noinhibit-exec -icf=all --no-warn-symbol-ordering 2>&1 \
-# RUN:   | FileCheck %s --check-prefix=NOWARN
+# No warnings if --no-call-graph-profile-sort or --no-warn-symbol-ordering
+
+# RUN: ld.lld --no-warn-symbol-ordering %t -o /dev/null \
+# RUN:   --noinhibit-exec --icf=all 2>&1 | FileCheck %s --check-prefix=NOWARN
+# RUN: ld.lld --no-call-graph-profile-sort %t -o /dev/null \
+# RUN:   --noinhibit-exec --icf=all 2>&1 | FileCheck %s --check-prefix=NOWARN
 # NOWARN-NOT: unable to order