[PATCH] D74791: Add a --shuffle-sections=seed option to lld

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:959
   config->saveTemps = args.hasArg(OPT_save_temps);
+  if (args.hasArg(OPT_shuffle_sections)) {
+    config->shuffleSectionSeed = args::getInteger(args, OPT_shuffle_sections, 0);
----------------
Delete braces around simple statements.


================
Comment at: lld/ELF/Options.td:503
+def shuffle_sections: J<"shuffle-sections=">,
+  HelpText<"Shuffle input sections using the given seed. If 0, uses a random seed">;
 def thinlto_cache_dir: J<"thinlto-cache-dir=">,
----------------
Add `MetaVarName<seed>`


================
Comment at: lld/ELF/Writer.cpp:1208
 
+// Adds random priorities to sections not already in the map
+static void maybeShuffle(DenseMap<const InputSectionBase *, int> &order) {
----------------
End with a period.


================
Comment at: lld/ELF/Writer.cpp:1219
+    prio = curPrio++;
+  const uint32_t &seed = *config->shuffleSectionSeed;
+  std::mt19937 g;
----------------
Does `const uint32_t &` -> `uint32_t` work?


================
Comment at: lld/ELF/Writer.cpp:1225
+    std::random_device rd;
+    g = std::mt19937(rd());
+  }
----------------
`std::mt19937 g(seed ? seed : std::random_device()());`



================
Comment at: lld/ELF/Writer.cpp:1230
+  for (InputSectionBase *sec : inputSections) {
+    auto p = order.insert(std::make_pair(sec, priorities[prioIndex]));
+    if (p.second)
----------------
try_emplace avoids `std::make_pair`

`p` can be deleted


================
Comment at: lld/test/ELF/symbol-ordering-file-shuffle.s:2
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld %t.o -o %t.out
----------------
Nit: `-triple=x86_64` (general ELF) makes it clear this is not Linux specific (applies to FreeBSD, etc).


================
Comment at: lld/test/ELF/symbol-ordering-file-shuffle.s:8
+
+# RUN: ld.lld --symbol-ordering-file %t_order.txt --shuffle-sections=1 %t.o -o %t2.out
+# RUN: llvm-objdump --section=.text -s %t2.out| FileCheck %s --check-prefix=SHUFFLE1
----------------
`%t1.out`

I think you intended to differentiate output filenames.


================
Comment at: lld/test/ELF/symbol-ordering-file-shuffle.s:9
+# RUN: ld.lld --symbol-ordering-file %t_order.txt --shuffle-sections=1 %t.o -o %t2.out
+# RUN: llvm-objdump --section=.text -s %t2.out| FileCheck %s --check-prefix=SHUFFLE1
+# SHUFFLE1: Contents of section .text:
----------------
Nit: `llvm-readelf -x .text` is slightly better.

llvm-objdump's output, except `-d`, is usually strange.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74791/new/

https://reviews.llvm.org/D74791