[PATCH] D121342: [lld-macho] Align cstrings less conservatively

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:1375-1378
+// We differ slightly from ld64 in how we've chosen to align these cstrings.
+// Both LLD and ld64 preserve the number of trailing zeros in each cstring's
+// address in the input object files. When deduplicating identical cstrings,
+// both linkers pick the cstring whose address has more trailing zeros, and
----------------
should we note soemthing about this in the lld-vs-ld64.rst doc?
(Not this implementation detail particularly but rather the possible observable difference in the final binaries)


================
Comment at: lld/MachO/SyntheticSections.cpp:1419-1429
+      auto s = isec->getCachedHashStringRef(i);
+      auto it = stringOffsetMap.find(s);
+      assert(it != stringOffsetMap.end());
+      uint64_t &outSecOff = it->second.outSecOff;
+      if (outSecOff == UINT64_MAX) {
+        outSecOff = alignTo(size, 1 << it->second.trailingZeros);
+        size = outSecOff + s.size();
----------------
how's this? 
just to make it a bit clearer you're updating the entry - otherwise it's a bit easy to miss at first glance.

(related nit: could have a static constant (eg., TombStone) so we dont have to remember using UINT64_MAX)


================
Comment at: lld/MachO/SyntheticSections.h:534
 public:
-  DeduplicatedCStringSection();
-  uint64_t getSize() const override { return builder.getSize(); }
+  DeduplicatedCStringSection() = default;
+  uint64_t getSize() const override { return size; }
----------------
just delete the whole decl? (default ctor is implicitly provided already)


================
Comment at: lld/MachO/SyntheticSections.h:542
+    uint8_t trailingZeros;
+    uint64_t outSecOff = UINT64_MAX;
+
----------------
(consistency with "isec")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121342