[lld][macho] Support order cstrings with -order_file #140307

SharonXSharon · 2025-05-16T21:01:43Z

Expand the -order_file also accept cstrings to order.
The purpose is to order hot cstrings for performance (implemented in this diff), and then later on we can also order cold cstrings for compression size win.

Due to the speciality of cstrings, there's no way to pass in symbol names in the order file as the existing -order_file, so we expect <hash of cstring literal content> to represent/identify each cstring.

// An order file has one entry per line, in the following format:
  //
  //   <cpu>:<object file>:[<symbol name> | CStringEntryPrefix <cstring hash>]
  //
  // <cpu> and <object file> are optional.
  // If not specified, then that entry tries to match either,
  //
  // 1) any symbol of the <symbol name>;
  // Parsing this format is not quite straightforward because the symbol name
  // itself can contain colons, so when encountering a colon, we consider the
  // preceding characters to decide if it can be a valid CPU type or file path.
  // If a symbol is matched by multiple entries, then it takes the
  // lowest-ordered entry (the one nearest to the front of the list.)
  //
  // or 2) any cstring literal with the given hash, if the entry has the
  // CStringEntryPrefix prefix defined below in the file. <cstring hash> is the
  // hash of cstring literal content.
  //
  // Cstring literals are not symbolized, we can't identify them by name
  // However, cstrings are deduplicated, hence unique, so we use the hash of
  // the content of cstring literals to identify them and assign priority to it.
  // We use the same hash as used in StringPiece, i.e. 31 bit:
  // xxh3_64bits(string) & 0x7fffffff
  //

The ordering of cstring has to happen during/before the finalizing of the cstring section content in the finalizeContents() function, which happens before the writer is run

llvmbot · 2025-05-16T22:02:52Z

@llvm/pr-subscribers-lld-macho

@llvm/pr-subscribers-lld

Author: None (SharonXSharon)

Changes

Add a new -order_file_cstring flag, which can take a order file for ordering cstrings, similar to the existing -order_file.

Due to the speciality of cstrings, there's no way to pass in symbol names in the order file as the existing -order_file, so we expect <hash of cstring literal content> in the cstring order file. Given the cstrings are deduplicated by default, the hash should be able to identify each cstring. The order file can also accept comments starting with #, same with existing -order_file.

The ordering of cstring has to happen during/before the finalizing of the cstring section content in the finalizeContents() function, which happens before the writer is run

Full diff: https://github.com/llvm/llvm-project/pull/140307.diff

7 Files Affected:

(modified) lld/MachO/Config.h (+1)
(modified) lld/MachO/Driver.cpp (+12-9)
(modified) lld/MachO/Options.td (+4)
(modified) lld/MachO/SectionPriorities.cpp (+71)
(modified) lld/MachO/SectionPriorities.h (+20)
(modified) lld/MachO/SyntheticSections.cpp (+29-29)
(added) lld/test/MachO/ordre-file-cstring.s (+222)

diff --git a/lld/MachO/Config.h b/lld/MachO/Config.h
index a01e60efbe761..d0217b38c3007 100644
--- a/lld/MachO/Config.h
+++ b/lld/MachO/Config.h
@@ -225,6 +225,7 @@ struct Configuration {
   bool callGraphProfileSort = false;
   llvm::StringRef printSymbolOrder;
 
+  llvm::StringRef cStringOrderFilePath;
   llvm::StringRef irpgoProfilePath;
   bool bpStartupFunctionSort = false;
   bool bpCompressionSortStartupFunctions = false;
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 5c32055166da6..0f2957295d136 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -337,15 +337,15 @@ static InputFile *addFile(StringRef path, LoadType loadType,
         for (const object::Archive::Child &c : file->getArchive().children(e)) {
           StringRef reason;
           switch (loadType) {
-            case LoadType::LCLinkerOption:
-              reason = "LC_LINKER_OPTION";
-              break;
-            case LoadType::CommandLineForce:
-              reason = "-force_load";
-              break;
-            case LoadType::CommandLine:
-              reason = "-all_load";
-              break;
+          case LoadType::LCLinkerOption:
+            reason = "LC_LINKER_OPTION";
+            break;
+          case LoadType::CommandLineForce:
+            reason = "-force_load";
+            break;
+          case LoadType::CommandLine:
+            reason = "-all_load";
+            break;
           }
           if (Error e = file->fetch(c, reason)) {
             if (config->warnThinArchiveMissingMembers)
@@ -2178,6 +2178,9 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
     StringRef orderFile = args.getLastArgValue(OPT_order_file);
     if (!orderFile.empty())
       priorityBuilder.parseOrderFile(orderFile);
+    config->cStringOrderFilePath = args.getLastArgValue(OPT_order_file_cstring);
+    if (!config->cStringOrderFilePath.empty())
+      priorityBuilder.parseOrderFileCString(config->cStringOrderFilePath);
 
     referenceStubBinder();
 
diff --git a/lld/MachO/Options.td b/lld/MachO/Options.td
index 4f0602f59812b..34faa75103224 100644
--- a/lld/MachO/Options.td
+++ b/lld/MachO/Options.td
@@ -400,6 +400,10 @@ def order_file : Separate<["-"], "order_file">,
     MetaVarName<"<file>">,
     HelpText<"Layout functions and data according to specification in <file>">,
     Group<grp_opts>;
+def order_file_cstring : Separate<["-"], "order_file_cstring">,
+    MetaVarName<"<file>">,
+    HelpText<"Layout cstrings according to specification in <file>">,
+    Group<grp_opts>;
 def no_order_inits : Flag<["-"], "no_order_inits">,
     HelpText<"Disable default reordering of initializer and terminator functions">,
     Flags<[HelpHidden]>,
diff --git a/lld/MachO/SectionPriorities.cpp b/lld/MachO/SectionPriorities.cpp
index 7a4a5d8465f64..e7372050b7601 100644
--- a/lld/MachO/SectionPriorities.cpp
+++ b/lld/MachO/SectionPriorities.cpp
@@ -388,3 +388,74 @@ macho::PriorityBuilder::buildInputSectionPriorities() {
 
   return sectionPriorities;
 }
+
+void macho::PriorityBuilder::parseOrderFileCString(StringRef path) {
+  std::optional<MemoryBufferRef> buffer = readFile(path);
+  if (!buffer) {
+    error("Could not read cstring order file at " + path);
+    return;
+  }
+  MemoryBufferRef mbref = *buffer;
+  int priority = std::numeric_limits<int>::min();
+  for (StringRef line : args::getLines(mbref)) {
+    if (line.empty())
+      continue;
+    uint32_t hash = 0;
+    if (!to_integer(line, hash))
+      continue;
+    auto it = cStringPriorities.find(hash);
+    if (it == cStringPriorities.end())
+      cStringPriorities[hash] = ++priority;
+    else
+      assert(it->second <= priority);
+  }
+}
+
+std::vector<StringPiecePair> macho::PriorityBuilder::buildCStringPriorities(
+    ArrayRef<CStringInputSection *> inputs) {
+  std::vector<StringPiecePair> orderedStringPieces;
+  if (config->cStringOrderFilePath.empty()) {
+    for (CStringInputSection *isec : inputs) {
+      for (const auto &[stringPieceIdx, piece] :
+           llvm::enumerate(isec->pieces)) {
+        if (!piece.live)
+          continue;
+        orderedStringPieces.emplace_back(isec, stringPieceIdx);
+      }
+    }
+    return orderedStringPieces;
+  }
+
+  // Split the input strings into hold and cold sets.
+  // Order hot set based on -order_file_cstring for performance improvement;
+  // TODO: Order cold set of cstrings for compression via BP.
+  std::vector<std::pair<int, StringPiecePair>>
+      hotStringPrioritiesAndStringPieces;
+  std::vector<StringPiecePair> coldStringPieces;
+
+  for (CStringInputSection *isec : inputs) {
+    for (const auto &[stringPieceIdx, piece] : llvm::enumerate(isec->pieces)) {
+      if (!piece.live)
+        continue;
+
+      auto it = cStringPriorities.find(piece.hash);
+      if (it != cStringPriorities.end())
+        hotStringPrioritiesAndStringPieces.emplace_back(
+            it->second, std::make_pair(isec, stringPieceIdx));
+      else
+        coldStringPieces.emplace_back(isec, stringPieceIdx);
+    }
+  }
+
+  // Order hot set for perf
+  llvm::stable_sort(hotStringPrioritiesAndStringPieces);
+  for (auto &[priority, stringPiecePair] : hotStringPrioritiesAndStringPieces)
+    orderedStringPieces.push_back(stringPiecePair);
+
+  // TODO: Order cold set for compression
+
+  orderedStringPieces.insert(orderedStringPieces.end(),
+                             coldStringPieces.begin(), coldStringPieces.end());
+
+  return orderedStringPieces;
+}
diff --git a/lld/MachO/SectionPriorities.h b/lld/MachO/SectionPriorities.h
index 44fb101990c51..5593494d8a274 100644
--- a/lld/MachO/SectionPriorities.h
+++ b/lld/MachO/SectionPriorities.h
@@ -16,6 +16,7 @@
 namespace lld::macho {
 
 using SectionPair = std::pair<const InputSection *, const InputSection *>;
+using StringPiecePair = std::pair<CStringInputSection *, size_t>;
 
 class PriorityBuilder {
 public:
@@ -55,6 +56,23 @@ class PriorityBuilder {
   // contains.
   llvm::DenseMap<const InputSection *, int> buildInputSectionPriorities();
 
+  // Reads the cstring order file at `path` into cStringPriorities.
+  // An cstring order file has one entry per line, in the following format:
+  //
+  // <hash of cstring literal content>
+  //
+  // Cstring literals are not symbolized, we can't identify them by name
+  // However, cstrings are deduplicated, hence unique, so we use the hash of
+  // the content of cstring literals to identify them and assign priority to it.
+  // We use the same hash as used in StringPiece, i.e. 31 bit:
+  // xxh3_64bits(string) & 0x7fffffff
+  //
+  // Additionally, given they are deduplicated and unique, we don't need to know
+  // which object file they are from.
+  void parseOrderFileCString(StringRef path);
+  std::vector<StringPiecePair>
+      buildCStringPriorities(ArrayRef<CStringInputSection *>);
+
 private:
   // The symbol with the smallest priority should be ordered first in the output
   // section (modulo input section contiguity constraints).
@@ -68,6 +86,8 @@ class PriorityBuilder {
 
   std::optional<int> getSymbolPriority(const Defined *sym);
   llvm::DenseMap<llvm::StringRef, SymbolPriorityEntry> priorities;
+  /// A map from cstring literal hashes to priorities
+  llvm::DenseMap<uint32_t, int> cStringPriorities;
   llvm::MapVector<SectionPair, uint64_t> callGraphProfile;
 };
 
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index dfacaf2ef4e0d..beddcfa2174e0 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -15,6 +15,7 @@
 #include "MachOStructs.h"
 #include "ObjC.h"
 #include "OutputSegment.h"
+#include "SectionPriorities.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 
@@ -1766,26 +1767,25 @@ void DeduplicatedCStringSection::finalizeContents() {
     }
   }
 
-  // Assign an offset for each string and save it to the corresponding
+  // Sort the strings for performance and compression size win, and then
+  // assign an offset for each string and save it to the corresponding
   // StringPieces for easy access.
-  for (CStringInputSection *isec : inputs) {
-    for (const auto &[i, piece] : llvm::enumerate(isec->pieces)) {
-      if (!piece.live)
-        continue;
-      auto s = isec->getCachedHashStringRef(i);
-      auto it = stringOffsetMap.find(s);
-      assert(it != stringOffsetMap.end());
-      StringOffset &offsetInfo = it->second;
-      if (offsetInfo.outSecOff == UINT64_MAX) {
-        offsetInfo.outSecOff =
-            alignToPowerOf2(size, 1ULL << offsetInfo.trailingZeros);
-        size =
-            offsetInfo.outSecOff + s.size() + 1; // account for null terminator
-      }
-      piece.outSecOff = offsetInfo.outSecOff;
+  for (auto &[isec, i] : priorityBuilder.buildCStringPriorities(inputs)) {
+    auto &piece = isec->pieces[i];
+    auto s = isec->getCachedHashStringRef(i);
+    auto it = stringOffsetMap.find(s);
+    assert(it != stringOffsetMap.end());
+    lld::macho::DeduplicatedCStringSection::StringOffset &offsetInfo =
+        it->second;
+    if (offsetInfo.outSecOff == UINT64_MAX) {
+      offsetInfo.outSecOff =
+          alignToPowerOf2(size, 1ULL << offsetInfo.trailingZeros);
+      size = offsetInfo.outSecOff + s.size() + 1; // account for null terminator
     }
-    isec->isFinal = true;
+    piece.outSecOff = offsetInfo.outSecOff;
   }
+  for (CStringInputSection *isec : inputs)
+    isec->isFinal = true;
 }
 
 void DeduplicatedCStringSection::writeTo(uint8_t *buf) const {
@@ -1908,18 +1908,18 @@ ObjCImageInfoSection::parseImageInfo(const InputFile *file) {
 
 static std::string swiftVersionString(uint8_t version) {
   switch (version) {
-    case 1:
-      return "1.0";
-    case 2:
-      return "1.1";
-    case 3:
-      return "2.0";
-    case 4:
-      return "3.0";
-    case 5:
-      return "4.0";
-    default:
-      return ("0x" + Twine::utohexstr(version)).str();
+  case 1:
+    return "1.0";
+  case 2:
+    return "1.1";
+  case 3:
+    return "2.0";
+  case 4:
+    return "3.0";
+  case 5:
+    return "4.0";
+  default:
+    return ("0x" + Twine::utohexstr(version)).str();
   }
 }
 
diff --git a/lld/test/MachO/ordre-file-cstring.s b/lld/test/MachO/ordre-file-cstring.s
new file mode 100644
index 0000000000000..138f1685467ee
--- /dev/null
+++ b/lld/test/MachO/ordre-file-cstring.s
@@ -0,0 +1,222 @@
+# RUN: rm -rf %t; split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin  %t/test.s -o %t/test.o
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %t/more-cstrings.s -o %t/more-cstrings.o
+
+# RUN: %lld --deduplicate-strings -arch arm64 -lSystem -e _main -o %t/test-0 %t/test.o %t/more-cstrings.o
+# RUN: llvm-nm --numeric-sort --format=just-symbols %t/test-0 | FileCheck %s --check-prefix=ORIGIN_SYM
+# RUN: llvm-objdump --macho --section="__TEXT,__cstring" %t/test-0 | FileCheck %s --check-prefix=ORIGIN_SEC
+
+# RUN: %lld --deduplicate-strings -arch arm64 -lSystem -e _main -o %t/test-1 %t/test.o %t/more-cstrings.o -order_file_cstring %t/ord-1
+# RUN: llvm-nm --numeric-sort --format=just-symbols %t/test-1 | FileCheck %s --check-prefix=ONE_SYM
+# RUN: llvm-objdump --macho --section="__TEXT,__cstring" %t/test-1 | FileCheck %s --check-prefix=ONE_SEC
+
+
+# RUN: %lld --deduplicate-strings -arch arm64 -lSystem -e _main -o %t/test-2 %t/test.o %t/more-cstrings.o -order_file_cstring %t/ord-2
+# RUN: llvm-nm --numeric-sort --format=just-symbols %t/test-2 | FileCheck %s --check-prefix=TWO_SYM
+# RUN: llvm-objdump --macho --section="__TEXT,__cstring" %t/test-2 | FileCheck %s --check-prefix=TWO_SEC
+
+# RUN: %lld --deduplicate-strings -arch arm64 -lSystem -e _main -o %t/test-3 %t/test.o %t/more-cstrings.o -order_file_cstring %t/ord-3
+# RUN: llvm-nm --numeric-sort --format=just-symbols %t/test-3 | FileCheck %s --check-prefix=THREE_SYM
+# RUN: llvm-objdump --macho --section="__TEXT,__cstring" %t/test-3 | FileCheck %s --check-prefix=THREE_SEC
+
+# RUN: %lld --deduplicate-strings -arch arm64 -lSystem -e _main -o %t/test-4 %t/test.o %t/more-cstrings.o -order_file_cstring %t/ord-4
+# RUN: llvm-nm --numeric-sort --format=just-symbols %t/test-4 | FileCheck %s --check-prefix=FOUR_SYM
+# RUN: llvm-objdump --macho --section="__TEXT,__cstring" %t/test-4 | FileCheck %s --check-prefix=FOUR_SEC
+# RUN: llvm-readobj --string-dump=__cstring %t/test-4 | FileCheck %s --check-prefix=FOUR_SEC_ESCAPE
+
+
+# We expect:
+# 1) Covered cstring symbols are reordered
+# 2) the rest of the cstring symbols remain original relative order within the cstring section
+
+# ORIGIN_SYM: _local_foo1
+# ORIGIN_SYM: _globl_foo2
+# ORIGIN_SYM: _local_foo2
+# ORIGIN_SYM: _bar
+# ORIGIN_SYM: _baz
+# ORIGIN_SYM: _baz_dup
+# ORIGIN_SYM: _bar2
+# ORIGIN_SYM: _globl_foo3
+
+# ORIGIN_SEC: foo1
+# ORIGIN_SEC: foo2
+# ORIGIN_SEC: bar
+# ORIGIN_SEC: baz
+# ORIGIN_SEC: bar2
+# ORIGIN_SEC: foo3
+
+
+# ONE_SYM: _globl_foo2
+# ONE_SYM: _local_foo2
+# ONE_SYM: _bar
+# ONE_SYM: _bar2
+# ONE_SYM: _globl_foo3
+# ONE_SYM: _local_foo1
+# ONE_SYM: _baz
+# ONE_SYM: _baz_dup
+
+# ONE_SEC: foo2
+# ONE_SEC: bar
+# ONE_SEC: bar2
+# ONE_SEC: foo3
+# ONE_SEC: foo1
+# ONE_SEC: baz
+
+
+# TWO_SYM: _globl_foo2
+# TWO_SYM: _local_foo2
+# TWO_SYM: _local_foo1
+# TWO_SYM: _baz
+# TWO_SYM: _baz_dup
+# TWO_SYM: _bar
+# TWO_SYM: _bar2
+# TWO_SYM: _globl_foo3
+
+# TWO_SEC: foo2
+# TWO_SEC: foo1
+# TWO_SEC: baz
+# TWO_SEC: bar
+# TWO_SEC: bar2
+# TWO_SEC: foo3
+
+
+# THREE_SYM: _local_foo1
+# THREE_SYM: _baz
+# THREE_SYM: _baz_dup
+# THREE_SYM: _bar
+# THREE_SYM: _bar2
+# THREE_SYM: _globl_foo2
+# THREE_SYM: _local_foo2
+# THREE_SYM: _globl_foo3
+
+# THREE_SEC: foo1
+# THREE_SEC: baz
+# THREE_SEC: bar
+# THREE_SEC: bar2
+# THREE_SEC: foo2
+# THREE_SEC: foo3
+
+
+# FOUR_SYM: _local_escape_white_space
+# FOUR_SYM: _globl_foo2
+# FOUR_SYM: _local_foo2
+# FOUR_SYM: _local_escape
+# FOUR_SYM: _globl_foo3
+# FOUR_SYM: _bar
+# FOUR_SYM: _local_foo1
+# FOUR_SYM: _baz
+# FOUR_SYM: _baz_dup
+# FOUR_SYM: _bar2
+
+# FOUR_SEC: \t\n
+# FOUR_SEC: foo2
+# FOUR_SEC: @\"NSDictionary\"
+# FOUR_SEC: foo3
+# FOUR_SEC: bar
+# FOUR_SEC: foo1
+# FOUR_SEC: baz
+# FOUR_SEC: bar2
+
+# FOUR_SEC_ESCAPE: ..
+# FOUR_SEC_ESCAPE: foo2
+# FOUR_SEC_ESCAPE: @"NSDictionary"
+# FOUR_SEC_ESCAPE: foo3
+# FOUR_SEC_ESCAPE: bar
+# FOUR_SEC_ESCAPE: foo1
+# FOUR_SEC_ESCAPE: baz
+# FOUR_SEC_ESCAPE: bar2
+
+# original order, but only parital covered
+#--- ord-1
+#foo2
+0x55783A95
+#bar
+0x2032D362
+#bar2
+0x592F855B
+#foo3
+0x501BCC31
+
+# change order, parital covered
+#--- ord-2
+#foo2
+0x55783A95
+#foo1
+0x6326A039
+#baz
+0x336F8925
+#bar
+0x2032D362
+#bar2
+0x592F855B
+
+# change order, parital covered, with mismatches, duplicates
+#--- ord-3
+foo2222
+0x11111111
+#foo1
+0x6326A039
+#baz
+0x336F8925
+#bar
+0x2032D362
+#bar2
+0x592F855B
+#baz
+0x336F8925
+
+# test escape strings
+#--- ord-4
+#\t\n
+0x3DBEA0C9
+#foo2
+0x55783A95
+#@\"NSDictionary\"
+0x47AF4776
+#foo3
+0x501BCC31
+#bar
+0x2032D362
+
+
+#--- test.s
+.text
+.globl _main
+
+_main:
+  ret
+
+.cstring
+.p2align 2
+_local_foo1:
+  .asciz "foo1"
+_local_foo2:
+  .asciz "foo2"
+L_.foo1_dup:
+  .asciz "foo1"
+L_.foo2_dup:
+  .asciz "foo2"
+_local_escape:
+  .asciz "@\"NSDictionary\""
+_local_escape_white_space:
+  .asciz "\t\n"
+
+_bar:
+  .asciz "bar"
+_baz:
+  .asciz "baz"
+_bar2:
+  .asciz "bar2"
+_baz_dup:
+  .asciz "baz"
+
+.subsections_via_symbols
+
+#--- more-cstrings.s
+.globl _globl_foo1, _globl_foo3
+.cstring
+.p2align 4
+_globl_foo3:
+  .asciz "foo3"
+_globl_foo2:
+  .asciz "foo2"

lld/MachO/Driver.cpp

ellishg · 2025-05-16T22:34:53Z

lld/MachO/SectionPriorities.cpp

+  std::vector<StringPiecePair> orderedStringPieces;
+  if (config->cStringOrderFilePath.empty()) {
+    for (CStringInputSection *isec : inputs) {
+      for (const auto &[stringPieceIdx, piece] :
+           llvm::enumerate(isec->pieces)) {
+        if (!piece.live)
+          continue;
+        orderedStringPieces.emplace_back(isec, stringPieceIdx);
+      }
+    }
+    return orderedStringPieces;
+  }


If cStringOrderFilePath is empty, then cStringPriorities will be empty. So I think you can just remove this code entirely.

Suggested change

std::vector<StringPiecePair> orderedStringPieces;

if (config->cStringOrderFilePath.empty()) {

for (CStringInputSection *isec : inputs) {

for (const auto &[stringPieceIdx, piece] :

llvm::enumerate(isec->pieces)) {

if (!piece.live)

continue;

orderedStringPieces.emplace_back(isec, stringPieceIdx);

}

}

return orderedStringPieces;

}

yes, i was thinking adding this early return can avoid the unnecessary checks of cStringPriorities when cStringOrderFilePath is empty, but perhaps those operations aren't really expensive?

DenseMap::find() returns pretty quickly if it's empty. I don't think it would impact performance

llvm-project/llvm/include/llvm/ADT/DenseMap.h

Lines 650 to 654 in 286ab11

template <typename LookupKeyT> BucketT *doFind(const LookupKeyT &Val) {

BucketT *BucketsPtr = getBuckets();

const unsigned NumBuckets = getNumBuckets();

if (NumBuckets == 0)

return nullptr;

lld/MachO/SectionPriorities.cpp

alx32 · 2025-05-16T23:33:43Z

lld/MachO/SectionPriorities.cpp

+    if (!to_integer(line, hash))
+      continue;


Do we want to just silently ignore these errors? Is it valid to just continue parsing once we've encountered such a scenario?
Could this ever be a hex value?

Yea I guess we don't want to exit just because we encounter an invalid line?
The hash is indeed a hex, the to_integer should be true for parsing a hex number. The hash we are using is the existing hash lld use for cstring literal dedup,
in

llvm-project/lld/MachO/SyntheticSections.cpp

Line 1803 in 4d0a735

uint32_t hash = xxh3_64bits(str) & 0x7fffffff;

alx32 · 2025-05-16T23:41:33Z

lld/MachO/SectionPriorities.cpp

+      continue;
+    auto it = cStringPriorities.find(hash);
+    if (it == cStringPriorities.end())
+      cStringPriorities[hash] = ++priority;


This performs the lookup twice, which could be a noticeable performance hit. Instead:

auto [it, inserted] = cStringPriorities.try_emplace(hash, 0); // If actually inserted, update with the new priority if (inserted) it->second = ++priority;

ellishg · 2025-05-17T00:05:22Z

lld/MachO/SectionPriorities.cpp

+    auto it = cStringPriorities.find(hash);
+    if (it == cStringPriorities.end())
+      cStringPriorities[hash] = ++priority;


@alx32 if we want to use try_emplace() we can do this change if we are ok with setting the priority first and then incrementing

Suggested change

auto it = cStringPriorities.find(hash);

if (it == cStringPriorities.end())

cStringPriorities[hash] = ++priority;

auto [it, wasInserted] = cStringPriorities.try_emplace(hash, priority);

if (wasInserted)

++priority;

snehasish

Consider this alternative approach -- can we extend --symbol_ordering_file format to include cstrings? E.g. instead of the symbol name on each line we would have something like <cstr>:hash.

The concern I have with the current approach is the inability to specify a relative ordering of strings with respect to other static data objects. This is limting for fine grained tuning. Additionally build systems will need to be updated to handle the new flag and additional ordering file artefact. Post-link optimizations like Propeller and Intel's thin layout optimizer use the symbol ordering file too. I think it would be easier for these tools to adopt the static data layout enhancements if we reuse --symbol_ordering_file in my opinion.

Wdyt?

SharonXSharon · 2025-05-20T16:48:06Z

can we extend --symbol_ordering_file format to include cstrings? E.g. instead of the symbol name on each line we would have something like <cstr>:hash.

I am not super familiar with ELF, I assume --symbol_ordering_file is the same with -order_file in MachO right?
implementation wise it seems okay to extend this flag.

the inability to specify a relative ordering of strings with respect to other static data objects.

In MachO, cstrings are put in a their own section which is separate from where the other static data symbols are put, so we can only order cstrings within their own section but not the relative order with other static data symbols. In ELF, are they both put in the same section so their relative orders also matter?

ellishg · 2025-05-20T17:51:46Z

--symbol_ordering_file is only available for ELF and for MachO we have -order_file. On MachO we have a special __cstring section that only holds string data instead of the __data section. For ELF, that string data is stored in the .rodata data section which presumably could be ordered relative to other read-only data.

https://godbolt.org/z/sKqfWj5Kq

The format of -order_file already allows for prefixes depending on the platform and object file.

llvm-project/lld/MachO/SectionPriorities.cpp

Lines 307 to 314 in 51e222e

    
           CPUType cpuType = StringSwitch<CPUType>(line) 
        
                                 .StartsWith("i386:", CPU_TYPE_I386) 
        
                                 .StartsWith("x86_64:", CPU_TYPE_X86_64) 
        
                                 .StartsWith("arm:", CPU_TYPE_ARM) 
        
                                 .StartsWith("arm64:", CPU_TYPE_ARM64) 
        
                                 .StartsWith("ppc:", CPU_TYPE_POWERPC) 
        
                                 .StartsWith("ppc64:", CPU_TYPE_POWERPC64) 
        
                                 .Default(CPU_TYPE_ANY);

I think we could extend this to support the literal prefix <cstr>:. I do see the benefit of reusing the same flag and input file, especially if we can share the same format with ELF.

And on ELF it looks like the format doesn't support any prefixes yet, but I can't think of any reason why it couldn't.

llvm-project/lld/ELF/Driver.cpp

Lines 1237 to 1246 in 0931874

    
           static SmallVector<StringRef, 0> getSymbolOrderingFile(Ctx &ctx, 
        
                                                                  MemoryBufferRef mb) { 
        
             SetVector<StringRef, SmallVector<StringRef, 0>> names; 
        
             for (StringRef s : args::getLines(mb)) 
        
               if (!names.insert(s) && ctx.arg.warnSymbolOrdering) 
        
                 Warn(ctx) << mb.getBufferIdentifier() 
        
                           << ": duplicate ordered symbol: " << s; 
        
             return names.takeVector(); 
        
           }

SharonXSharon · 2025-05-20T19:17:18Z

@ellishg

The format of -order_file already allows for prefixes depending on the platform and object file.
I think we could extend this to support the literal prefix :.

do you mean we should accept format like

arm64:file_name1:symbol1
file_name2:symbol2
symbol3
cstr:0x111
cstr:file_name4:0x222

where the first prefix is cpu_type|"cstr":?
or do you mean we prepend an optional "cstr:", so that we can accept
cstr:arm64:file_name1:0x222

ellishg · 2025-05-20T19:20:57Z

@ellishg

The format of -order_file already allows for prefixes depending on the platform and object file.
I think we could extend this to support the literal prefix :.

do you mean we should accept format like
arm64:file_name1:symbol1
file_name2:symbol2
symbol3
cstr:0x111
cstr:file_name4:0x222
where the first prefix is cpu_type|"cstr":? or do you mean we prepend an optional "cstr:", so that we can accept cstr:arm64:file_name1:0x222

I think it makes sense to put cstr: after the cpu type and file separately: [<CPU>:][<OBJ>.o:][cstr:]<symbol>

Maybe we should put up an RFC on https://discourse.llvm.org/

snehasish · 2025-05-22T01:38:37Z

--symbol_ordering_file is only available for ELF and for MachO we have -order_file. On MachO we have a special __cstring section that only holds string data instead of the __data section. For ELF, that string data is stored in the .rodata data section which presumably could be ordered relative to other read-only data.

https://godbolt.org/z/sKqfWj5Kq

The format of -order_file already allows for prefixes depending on the platform and object file.

llvm-project/lld/MachO/SectionPriorities.cpp

Lines 307 to 314 in 51e222e

CPUType cpuType = StringSwitch<CPUType>(line)

.StartsWith("i386:", CPU_TYPE_I386)

.StartsWith("x86_64:", CPU_TYPE_X86_64)

.StartsWith("arm:", CPU_TYPE_ARM)

.StartsWith("arm64:", CPU_TYPE_ARM64)

.StartsWith("ppc:", CPU_TYPE_POWERPC)

.StartsWith("ppc64:", CPU_TYPE_POWERPC64)

.Default(CPU_TYPE_ANY);

I think we could extend this to support the literal prefix <cstr>:. I do see the benefit of reusing the same flag and input file, especially if we can share the same format with ELF.

And on ELF it looks like the format doesn't support any prefixes yet, but I can't think of any reason why it couldn't.

llvm-project/lld/ELF/Driver.cpp

Lines 1237 to 1246 in 0931874

static SmallVector<StringRef, 0> getSymbolOrderingFile(Ctx &ctx,

MemoryBufferRef mb) {

SetVector<StringRef, SmallVector<StringRef, 0>> names;

for (StringRef s : args::getLines(mb))

if (!names.insert(s) && ctx.arg.warnSymbolOrdering)

Warn(ctx) << mb.getBufferIdentifier()

<< ": duplicate ordered symbol: " << s;

return names.takeVector();

}

Thanks looking into t looking into this @ellishg . It wasn't clear to me initially that this was meant for MachO but as you noted if we can share the format with ELF it would make things even simpler.

SharonXSharon · 2025-05-28T01:34:13Z

@ellishg @snehasish I have updated the patch to use the same flag/functions (in Macho it's -order_file), to read cstring entries too. Here are the description of the new format

/
  // An order file has one entry per line, in the following format:
  //
  //   <cpu>:<object file>:[<symbol name> | CStringEntryPrefix <cstring hash>]
  //
  // <cpu> and <object file> are optional.
  // If not specified, then that entry tries to match either,
  //
  // 1) any symbol of the <symbol name>;
  // Parsing this format is not quite straightforward because the symbol name
  // itself can contain colons, so when encountering a colon, we consider the
  // preceding characters to decide if it can be a valid CPU type or file path.
  // If a symbol is matched by multiple entries, then it takes the
  // lowest-ordered entry (the one nearest to the front of the list.)
  //
  // or 2) any cstring literal with the given hash, if the entry has the
  // CStringEntryPrefix prefix defined below in the file. <cstring hash> is the
  // hash of cstring literal content.
  //
  // Cstring literals are not symbolized, we can't identify them by name
  // However, cstrings are deduplicated, hence unique, so we use the hash of
  // the content of cstring literals to identify them and assign priority to it.
  // We use the same hash as used in StringPiece, i.e. 31 bit:
  // xxh3_64bits(string) & 0x7fffffff
  //

could you please review again? Thanks

github-actions · 2025-05-28T01:34:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ellishg

I just realized that CSTR:1234 is a valid symbol name (but so is i386:blah, so this format wasn't good in the first place). I don't know if ; is an allowed character in a symbol, should we use that?

ellishg · 2025-05-28T15:08:50Z

lld/MachO/SectionPriorities.cpp

+        continue;
+
+      std::optional<int> priority = getSymbolOrCStringPriority(
+          std::to_string(piece.hash), isec->getFile());


Right now we convert the string hash in the orderfile into an integer (but we don't use the result), then we also convert each hash in the object file to a string to look it up in the priority map. It would be more efficient to create a new priority map, cstrPriorities that maps integer hashes to priorities.

I don't know if ; is an allowed character in a symbol, should we use that?

@ellishg Do you mean just for CSTR; or do you mean all the other places using : currently should be changed to something like ;?

Ideally we would use ; in place of :, but this is a breaking change. Maybe in the future we can support both and then eventually drop support for :, but that would definitely require an RFC since it would break existing orderfiles.

For now, I think we can use the CSTR; format.

/bikeshedding

I believe the only character disallowed in the symbolname would be the null character (at least for ELF). More practical constraints on symbol names arise from the programming language used to generate the executable. So in this case I believe most c-like languages that we care about disallow : and ; in identifier names so they are equal in this regard. Given the existing usage of ':' I'd prefer we continue using the same prefix delimiter.

I believe most c-like languages that we care about disallow : and ; in identifier names

Actually we still support Objective-C which commonly has : in the symbol names. I don't think we should land as is.

SGTM to proceed with a different delimiter.

we still support Objective-C which commonly has : in the symbol names.

oh you're right, forgot about the objC symbol names. but on the other hand, all the existing : for CPU and files have already been causing this issue for ObjC symbols names...
maybe right now the best trade-off is to use CSTR;?

ellishg · 2025-05-28T15:15:24Z

lld/MachO/SectionPriorities.cpp

+    // The rest of the line is either <symbol name> or
+    // CStringEntryPrefix<cstring hash>
+    if (line.starts_with(CStringEntryPrefix)) {
+      StringRef possibleHash = line.drop_front(CStringEntryPrefix.size());
+      uint32_t hash = 0;
+      if (to_integer(possibleHash, hash))
+        line = possibleHash;
+    }
+    symbolOrCStrHash = line.trim();
+
+    if (!symbolOrCStrHash.empty()) {
+      SymbolPriorityEntry &entry = priorities[symbolOrCStrHash];


I think this is a good place to report an error if the orderfile cannot be parsed. It might take some refactoring to avoid code duplication

Suggested change

// The rest of the line is either <symbol name> or

// CStringEntryPrefix<cstring hash>

if (line.starts_with(CStringEntryPrefix)) {

StringRef possibleHash = line.drop_front(CStringEntryPrefix.size());

uint32_t hash = 0;

if (to_integer(possibleHash, hash))

line = possibleHash;

}

symbolOrCStrHash = line.trim();

if (!symbolOrCStrHash.empty()) {

SymbolPriorityEntry &entry = priorities[symbolOrCStrHash];

// The rest of the line is either <symbol name> or

// CStringEntryPrefix<cstring hash>

if (line.consume_front(CStringEntryPrefix)) {

uint32_t hash;

if (!line.consume_integer(/*Radix=*/0, hash))

// report error

if (!line.trim().empty())

// report error

SymbolPriorityEntry &entry = cstrPriorities[hash];

// assign priority and return?

}

symbol = line.trim();

if (!symbol.empty()) {

SymbolPriorityEntry &entry = priorities[symbol];

snehasish

Wondering what the next steps are and how we can help. Are you planning to put up an RFC?

snehasish · 2025-05-30T16:55:54Z

lld/test/MachO/ordre-file-cstring.s

+# ORIGIN_SEC: bar2
+# ORIGIN_SEC: foo3
+
+# original order, but only parital covered


typo: partial

Are you planning to put up an RFC?

@snehasish Do you mean for this patch itself or do you mean for the whole memprof for Darwin?

(Sorry was having a couple of sick days, probably going to submit the draft patch for the binary access profiling early next week)

I asked about the RFC since @ellishg mentioned it earlier (so it would be about this patch).

I think it makes sense to put cstr: after the cpu type and file separately: [<CPU>:][<OBJ>.o:][cstr:]<symbol>

Maybe we should put up an RFC on https://discourse.llvm.org/

^ This is the comment I am referring to.

In my opinion the changes and limited and have precedent in terms of the prefix based format so it isn't necessary.

(Sorry was having a couple of sick days, probably going to submit the draft patch for the binary access profiling early next week)

Thanks! No rush, please tag @teresajohnson @mingmingl-llvm and me when you are ready.

@snehasish if you don't think this needs an RFC, I don't mind not having one. Especially if we don't break existing orderfiles.

Let's proceed as is unless LLD owners disagree.

snehasish

Overall looks good to me.

SharonXSharon · 2025-06-02T16:23:40Z

Overall looks good to me.

@snehasish thanks for reviewing and accepting the patch! Could you please also help me merge the PR? (I don't have permission to merge yet)

snehasish · 2025-06-02T18:12:50Z

Overall looks good to me.

@snehasish thanks for reviewing and accepting the patch! Could you please also help me merge the PR? (I don't have permission to merge yet)

I would be happy to but lets wait for @ellishg to approve the change. Also let's give LLD owners this week to chime in if they have any concerns. Ping me next Monday again if you need me to merge it. Thanks for working on this change!

thevinster · 2025-06-02T23:00:37Z

Jfyi, title and summary should be updated to reflect the new changes in the PR. Otherwise lgtm.

ellishg

LGTM

SharonXSharon · 2025-06-03T17:57:36Z

Hi @SharonXSharon, we are seeing check-lld failed on lld::order-file-strip-hashes.s likely due to this PR. Could you please take a look at it? Thanks!

buildbot: https://lab.llvm.org/buildbot/#/builders/10/builds/6601

Hi @Kewen12 thanks for reporting the issue, I just fixed the issue in #142649

Kewen12 · 2025-06-03T18:09:11Z

Hi @SharonXSharon, we are seeing check-lld failed on lld::order-file-strip-hashes.s likely due to this PR. Could you please take a look at it? Thanks!
buildbot: https://lab.llvm.org/buildbot/#/builders/10/builds/6601

Hi @Kewen12 thanks for reporting the issue, I just fixed the issue in #142649

oops, sorry for the disruption. I mean to comment on another merged pr. Appreciated for the quick fix!

Expand the `-order_file` also accept cstrings to order. The purpose is to order hot cstrings for performance (implemented in this diff), and then later on we can also order cold cstrings for compression size win. Due to the speciality of cstrings, there's no way to pass in symbol names in the order file as the existing -order_file, so we expect `<hash of cstring literal content>` to represent/identify each cstring. ``` // An order file has one entry per line, in the following format: // // <cpu>:<object file>:[<symbol name> | CStringEntryPrefix <cstring hash>] // // <cpu> and <object file> are optional. // If not specified, then that entry tries to match either, // // 1) any symbol of the <symbol name>; // Parsing this format is not quite straightforward because the symbol name // itself can contain colons, so when encountering a colon, we consider the // preceding characters to decide if it can be a valid CPU type or file path. // If a symbol is matched by multiple entries, then it takes the // lowest-ordered entry (the one nearest to the front of the list.) // // or 2) any cstring literal with the given hash, if the entry has the // CStringEntryPrefix prefix defined below in the file. <cstring hash> is the // hash of cstring literal content. // // Cstring literals are not symbolized, we can't identify them by name // However, cstrings are deduplicated, hence unique, so we use the hash of // the content of cstring literals to identify them and assign priority to it. // We use the same hash as used in StringPiece, i.e. 31 bit: // xxh3_64bits(string) & 0x7fffffff // ``` The ordering of cstring has to happen during/before the finalizing of the cstring section content in the `finalizeContents()` function, which happens before the writer is run --------- Co-authored-by: Sharon Xu <[email protected]>

[lld] Support order cstrings with -order_file_cstring

4d0a735

SharonXSharon marked this pull request as ready for review May 16, 2025 22:02

llvmbot added lld lld:MachO labels May 16, 2025

ellishg requested review from int3, MaskRay, alx32 and ellishg May 16, 2025 22:17

ellishg reviewed May 16, 2025

View reviewed changes

lld/MachO/SectionPriorities.cpp Outdated Show resolved Hide resolved

SharonXSharon changed the title ~~[lld] Support order cstrings with -order_file_cstring~~ [lld][macho] Support order cstrings with -order_file_cstring May 16, 2025

alx32 reviewed May 16, 2025

View reviewed changes

ellishg reviewed May 17, 2025

View reviewed changes

address comments

9d251c4

SharonXSharon mentioned this pull request May 19, 2025

[StaticDataLayout][PGO]Implement reader and writer change for data access profiles #139997

Merged

snehasish reviewed May 20, 2025

View reviewed changes

use -order_file for cstring hashes

f39f908

Sharon Xu added 2 commits May 27, 2025 18:37

fix typo

7c08999

fix typo2

b2c4acb

ellishg reviewed May 28, 2025

View reviewed changes

snehasish reviewed May 30, 2025

View reviewed changes

snehasish approved these changes May 30, 2025

View reviewed changes

SharonXSharon changed the title ~~[lld][macho] Support order cstrings with -order_file_cstring~~ [lld][macho] Support order cstrings with -order_file Jun 3, 2025

fix the cstr prefix from CSTR: to CSTR;

af0f019

ellishg approved these changes Jun 3, 2025

View reviewed changes

Merge branch 'main' into SharonXSharon-lld-order

2ea09c3

Sharon Xu added 2 commits June 3, 2025 11:58

fix format

46ddc4a

fix test by adding # REQUIRES: aarch64

2bc748e

ellishg merged commit 40933fd into llvm:main Jun 5, 2025
11 checks passed

SharonXSharon mentioned this pull request Jun 5, 2025

Request Commit Access For SharonXSharon #142675

Closed

	template <typename LookupKeyT> BucketT *doFind(const LookupKeyT &Val) {
	BucketT *BucketsPtr = getBuckets();
	const unsigned NumBuckets = getNumBuckets();
	if (NumBuckets == 0)
	return nullptr;

[lld][macho] Support order cstrings with -order_file #140307

[lld][macho] Support order cstrings with -order_file #140307

Uh oh!

Conversation

SharonXSharon commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SharonXSharon May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snehasish left a comment

Choose a reason for hiding this comment

Uh oh!

SharonXSharon commented May 20, 2025

Uh oh!

ellishg commented May 20, 2025

Uh oh!

SharonXSharon commented May 20, 2025

Uh oh!

ellishg commented May 20, 2025

Uh oh!

snehasish commented May 22, 2025

Uh oh!

SharonXSharon commented May 28, 2025

Uh oh!

github-actions bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellishg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SharonXSharon May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snehasish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SharonXSharon May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SharonXSharon commented May 16, 2025 •

edited

Loading

llvmbot commented May 16, 2025 •

edited

Loading

SharonXSharon May 19, 2025 •

edited

Loading

github-actions bot commented May 28, 2025 •

edited

Loading

SharonXSharon May 30, 2025 •

edited

Loading

SharonXSharon May 30, 2025 •

edited

Loading