[PATCH] D102964: [lld-macho] Implement cstring deduplication

[Greg McGary via Phabricator via llvm-commits]

gkm accepted this revision.
gkm added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: lld/MachO/InputSection.h:102
+  uint32_t inSecOff;
+  uint32_t hash;
+  uint64_t outSecOff;
----------------
Why are we truncating 64-bit hashes to 32 bits? Because the low-order 32 bits are sufficient, and it's more important that `StringPiece` be 16 bytes vs. 24 bytes?


================
Comment at: lld/MachO/SyntheticSections.cpp:1081-1082
+      builder(StringTableBuilder::RAW,
+              /*Alignment=*/target->cpuType == CPU_TYPE_X86_64 ? 16 : 1) {
+  align = target->cpuType == CPU_TYPE_X86_64 ? 16 : 1;
+  flags = S_CSTRING_LITERALS;
----------------
The extremity of the target-dependent difference in alignment requirement is surprising, and worthy of a comment.


================
Comment at: lld/test/MachO/cstring-merging.s:54-65
+_local_foo1:
+  .asciz "foo"
+_local_foo2:
+  .asciz "foo"
+L_.foo1:
+  .asciz "foo"
+L_.foo2:
----------------
Is there value in testing ...
* Strings of length other than 3?
* Zero length?
* Non-null terminated?
* Prefix matches? (e.g. "foo" and "fool", or "bar" and "barf")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102964