[lld-macho] Preserve alignment for non-deduplicated cstrings

Fixes PR50637.

Downstream bug: https://crbug.com/1218958

Currently, we split __cstring along symbol boundaries with .subsections_via_symbols
when not deduplicating, and along null bytes when deduplicating. This change splits
along null bytes unconditionally, and preserves original alignment in the non-
deduplicated case.

Removing subsections-section-relocs.s because with this change, __cstring
is never reordered based on the order file.

Differential Revision: https://reviews.llvm.org/D104919

lld/MachO/InputFiles.cpp

@@ -262,10 +262,9 @@ void ObjFile::parseSections(ArrayRef<Section> sections) {
     uint32_t align = 1 << sec.align;
     uint32_t flags = sec.flags;
 
-    if (config->dedupLiterals &&
-        (sectionType(sec.flags) == S_CSTRING_LITERALS ||
-         isWordLiteralSection(sec.flags))) {
-      if (sec.nreloc)
+    if (sectionType(sec.flags) == S_CSTRING_LITERALS ||
+        (config->dedupLiterals && isWordLiteralSection(sec.flags))) {
+      if (sec.nreloc && config->dedupLiterals)
         fatal(toString(this) + " contains relocations in " + sec.segname + "," +
               sec.sectname +
               ", so LLD cannot deduplicate literals. Try re-running without "

lld/MachO/InputSection.cpp

@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "InputSection.h"
+#include "Config.h"
 #include "InputFiles.h"
 #include "OutputSegment.h"
 #include "Symbols.h"
@@ -156,7 +157,8 @@ void CStringInputSection::splitIntoPieces() {
     if (end == StringRef::npos)
       fatal(toString(this) + ": string is not null terminated");
     size_t size = end + 1;
-    pieces.emplace_back(off, xxHash64(s.substr(0, size)));
+    uint32_t hash = config->dedupLiterals ? xxHash64(s.substr(0, size)) : 0;
+    pieces.emplace_back(off, hash);
     s = s.substr(size);
     off += size;
   }

lld/MachO/InputSection.h

@@ -145,6 +145,7 @@ struct StringPiece {
   // Offset from the start of the containing input section.
   uint32_t inSecOff;
   uint32_t live : 1;
+  // Only set if deduplicating literals
   uint32_t hash : 31;
   // Offset from the start of the containing output section.
   uint64_t outSecOff = 0;
@@ -180,14 +181,20 @@ public:
   // Split at each null byte.
   void splitIntoPieces();
 
+  LLVM_ATTRIBUTE_ALWAYS_INLINE
+  StringRef getStringRef(size_t i) const {
+    size_t begin = pieces[i].inSecOff;
+    size_t end =
+        (pieces.size() - 1 == i) ? data.size() : pieces[i + 1].inSecOff;
+    return toStringRef(data.slice(begin, end - begin));
+  }
+
   // Returns i'th piece as a CachedHashStringRef. This function is very hot when
   // string merging is enabled, so we want to inline.
   LLVM_ATTRIBUTE_ALWAYS_INLINE
   llvm::CachedHashStringRef getCachedHashStringRef(size_t i) const {
-    size_t begin = pieces[i].inSecOff;
-    size_t end =
-        (pieces.size() - 1 == i) ? data.size() : pieces[i + 1].inSecOff;
-    return {toStringRef(data.slice(begin, end - begin)), pieces[i].hash};
+    assert(config->dedupLiterals);
+    return {getStringRef(i), pieces[i].hash};
   }
 
   static bool classof(const InputSection *isec) {

lld/MachO/SyntheticSections.cpp

@@ -1152,6 +1152,45 @@ void BitcodeBundleSection::writeTo(uint8_t *buf) const {
   remove(xarPath);
 }
 
+CStringSection::CStringSection()
+    : SyntheticSection(segment_names::text, section_names::cString) {
+  flags = S_CSTRING_LITERALS;
+}
+
+void CStringSection::addInput(CStringInputSection *isec) {
+  isec->parent = this;
+  inputs.push_back(isec);
+  if (isec->align > align)
+    align = isec->align;
+}
+
+void CStringSection::writeTo(uint8_t *buf) const {
+  for (const CStringInputSection *isec : inputs) {
+    for (size_t i = 0, e = isec->pieces.size(); i != e; ++i) {
+      if (!isec->pieces[i].live)
+        continue;
+      StringRef string = isec->getStringRef(i);
+      memcpy(buf + isec->pieces[i].outSecOff, string.data(), string.size());
+    }
+  }
+}
+
+void CStringSection::finalizeContents() {
+  uint64_t offset = 0;
+  for (CStringInputSection *isec : inputs) {
+    for (size_t i = 0, e = isec->pieces.size(); i != e; ++i) {
+      if (!isec->pieces[i].live)
+        continue;
+      uint32_t pieceAlign = MinAlign(isec->pieces[i].inSecOff, align);
+      offset = alignTo(offset, pieceAlign);
+      isec->pieces[i].outSecOff = offset;
+      isec->isFinal = true;
+      StringRef string = isec->getStringRef(i);
+      offset += string.size();
+    }
+  }
+  size = offset;
+}
 // Mergeable cstring literals are found under the __TEXT,__cstring section. In
 // contrast to ELF, which puts strings that need different alignments into
 // different sections, clang's Mach-O backend puts them all in one section.
@@ -1176,19 +1215,10 @@ void BitcodeBundleSection::writeTo(uint8_t *buf) const {
 // deduplication of differently-aligned strings.  Finally, the overhead is not
 // huge: using 16-byte alignment (vs no alignment) is only a 0.5% size overhead
 // when linking chromium_framework on x86_64.
-CStringSection::CStringSection()
-    : SyntheticSection(segment_names::text, section_names::cString),
-      builder(StringTableBuilder::RAW, /*Alignment=*/16) {
-  align = 16;
-  flags = S_CSTRING_LITERALS;
-}
+DeduplicatedCStringSection::DeduplicatedCStringSection()
+    : builder(StringTableBuilder::RAW, /*Alignment=*/16) {}
 
-void CStringSection::addInput(CStringInputSection *isec) {
-  isec->parent = this;
-  inputs.push_back(isec);
-}
-
-void CStringSection::finalizeContents() {
+void DeduplicatedCStringSection::finalizeContents() {
   // Add all string pieces to the string table builder to create section
   // contents.
   for (const CStringInputSection *isec : inputs)

lld/MachO/SyntheticSections.h

@@ -518,17 +518,28 @@ private:
   uint64_t xarSize;
 };
 
-class CStringSection final : public SyntheticSection {
+class CStringSection : public SyntheticSection {
 public:
   CStringSection();
   void addInput(CStringInputSection *);
-  uint64_t getSize() const override { return builder.getSize(); }
-  void finalizeContents();
+  uint64_t getSize() const override { return size; }
+  virtual void finalizeContents();
   bool isNeeded() const override { return !inputs.empty(); }
-  void writeTo(uint8_t *buf) const override { builder.write(buf); }
+  void writeTo(uint8_t *buf) const override;
 
   std::vector<CStringInputSection *> inputs;
 
+private:
+  uint64_t size;
+};
+
+class DeduplicatedCStringSection final : public CStringSection {
+public:
+  DeduplicatedCStringSection();
+  uint64_t getSize() const override { return builder.getSize(); }
+  void finalizeContents() override;
+  void writeTo(uint8_t *buf) const override { builder.write(buf); }
+
 private:
   llvm::StringTableBuilder builder;
 };

lld/MachO/Writer.cpp

@@ -1149,7 +1149,11 @@ template <class LP> void macho::writeResult() { Writer().run<LP>(); }
 
 void macho::createSyntheticSections() {
   in.header = make<MachHeaderSection>();
-  in.cStringSection = config->dedupLiterals ? make<CStringSection>() : nullptr;
+  if (config->dedupLiterals) {
+    in.cStringSection = make<DeduplicatedCStringSection>();
+  } else {
+    in.cStringSection = make<CStringSection>();
+  }
   in.wordLiteralSection =
       config->dedupLiterals ? make<WordLiteralSection>() : nullptr;
   in.rebase = make<RebaseSection>();

lld/test/MachO/dead-strip-align.s

@@ -0,0 +1,46 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
+# RUN: %lld -lSystem -o %t.out %t.o -dead_strip
+# RUN: llvm-otool -l %t.out | FileCheck --check-prefix=SECT %s
+# RUN: llvm-otool -vs __TEXT __cstring %t.out | FileCheck %s
+
+# SECT:      sectname __cstring
+# SECT-NEXT: segname __TEXT
+# SECT-NEXT: addr
+# SECT-NEXT: size
+# SECT-NEXT: offset
+# SECT-NEXT: align 2^4 (16)
+
+# CHECK: 0 \303Q043\005\376\334\272\230vT2\020\001
+# CHECK: 8 def
+
+.section __TEXT,__cstring,cstring_literals
+.globl _foo
+_foo:  # Dead. External, has symbol table entry, gets stripped.
+  .asciz "asdf"
+
+.globl _hi
+_hi:
+  .asciz "hi" # External, has symbol table entry.
+
+.p2align 4
+L_internal_aligned_16: # Has no symbol table entry.
+  .asciz "\303Q043\005\376\334\272\230vT2\020\001"
+
+L_internal_nonaligned:
+  .asciz "abc"
+
+.p2align 3
+L_internal_aligned_8:
+  .asciz "def"
+
+.text
+.globl _main
+_main:
+  movq _hi(%rip), %rax
+  movq L_internal_nonaligned(%rip), %rax
+  movq L_internal_aligned_8(%rip), %rax
+  movaps L_internal_aligned_16(%rip), %xmm0
+  retq
+
+.subsections_via_symbols

lld/test/MachO/subsections-section-relocs.s

@@ -1,52 +0,0 @@
-# REQUIRES: x86
-# RUN: rm -rf %t; split-file %s %t
-# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
-
-# RUN: %lld -o %t/test %t/test.o -order_file %t/order-file
-# RUN: llvm-objdump --section-headers -d --no-show-raw-insn %t/test | FileCheck %s
-# CHECK-LABEL: Sections:
-# CHECK:       __cstring {{[^ ]*}} {{0*}}[[#%x, CSTRING_ADDR:]]
-# CHECK-LABEL: Disassembly of section __TEXT,__text:
-## L._str should end up at CSTRING_ADDR + 4, and leaq is 7 bytes long so we
-## have RIP = ADDR + 7
-# CHECK:       [[#%x, ADDR:]]: leaq
-# CHECK-SAME:    [[#%u, CSTRING_ADDR + 4 - ADDR - 7]](%rip), %rsi {{.*}} <_bar_str+0x4>
-
-# RUN: llvm-readobj --string-dump=__cstring %t/test | FileCheck %s --check-prefix=STRINGS
-# STRINGS: bar
-# STRINGS: Private symbol
-# STRINGS: foo
-
-#--- order-file
-_bar_str
-_foo_str
-
-#--- test.s
-.text
-.globl _main, _foo_str, _bar_str
-
-_main:
-  leaq L_.str(%rip), %rsi
-  mov $0, %rax
-  ret
-
-.section __TEXT,__cstring
-_foo_str:
-  .asciz "foo"
-
-_bar_str:
-  .asciz "bar"
-
-## References to this generate a section relocation
-## N.B.: ld64 doesn't actually reorder symbols in __cstring based on the order
-##       file. Our implementation only does does so if --no-literal-merge is
-##       specified. I'm not sure how else to test section relocations that
-##       target an address inside a relocated symbol: using a non-__cstring
-##       section would cause llvm-mc to emit a symbol relocation instead using
-##       the nearest symbol. It might be more consistent for LLD to disable
-##       symbol-based cstring reordering altogether and leave this functionality
-##       untested, at least until we find a real-world use case...
-L_.str:
-  .asciz "Private symbol"
-
-.subsections_via_symbols