Skip to content

Conversation

ellishg
Copy link
Contributor

@ellishg ellishg commented Sep 29, 2025

Move llvm::countr_zero() into a helper function to reduce code duplication between CStringSection and DeduplicatedCStringSection. More importantly, this moves a giant comment to that helper function since it pertains to both classes.

@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2025

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-macho

Author: Ellis Hoag (ellishg)

Changes

Move llvm::countr_zero() into a helper function to reduce code duplication between CStringSection and DeduplicatedCStringSection. More importantly, this moves a giant comment to that helper function since it pertains to both classes.


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

1 Files Affected:

  • (modified) lld/MachO/SyntheticSections.cpp (+29-28)
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index 228b84db21c2a..d38b6c9e00157 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -1685,31 +1685,7 @@ void CStringSection::writeTo(uint8_t *buf) const {
   }
 }
 
-void CStringSection::finalizeContents() {
-  uint64_t offset = 0;
-  // TODO: Call buildCStringPriorities() to support cstring ordering when
-  // deduplication is off, although this may negatively impact build
-  // performance.
-  for (CStringInputSection *isec : inputs) {
-    for (const auto &[i, piece] : llvm::enumerate(isec->pieces)) {
-      if (!piece.live)
-        continue;
-      // See comment above DeduplicatedCStringSection for how alignment is
-      // handled.
-      uint32_t pieceAlign = 1
-                            << llvm::countr_zero(isec->align | piece.inSecOff);
-      offset = alignToPowerOf2(offset, pieceAlign);
-      piece.outSecOff = offset;
-      isec->isFinal = true;
-      StringRef string = isec->getStringRef(i);
-      offset += string.size() + 1; // account for null terminator
-    }
-  }
-  size = offset;
-}
-
-// Mergeable cstring literals are found under the __TEXT,__cstring section. In
-// contrast to ELF, which puts strings that need different alignments into
+// 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.
 // Strings that need to be aligned have the .p2align directive emitted before
 // them, which simply translates into zero padding in the object file. In other
@@ -1744,8 +1720,33 @@ void CStringSection::finalizeContents() {
 // requires its operand addresses to be 16-byte aligned). However, there will
 // typically also be other cstrings in the same file that aren't used via SIMD
 // and don't need this alignment. They will be emitted at some arbitrary address
-// `A`, but ld64 will treat them as being 16-byte aligned with an offset of `16
-// % A`.
+// `A`, but ld64 will treat them as being 16-byte aligned with an offset of
+// `16 % A`.
+static uint8_t getStringPieceAlignment(const CStringInputSection *isec,
+                                     const StringPiece &piece) {
+  return llvm::countr_zero(isec->align | piece.inSecOff);
+}
+
+void CStringSection::finalizeContents() {
+  uint64_t offset = 0;
+  // TODO: Call buildCStringPriorities() to support cstring ordering when
+  // deduplication is off, although this may negatively impact build
+  // performance.
+  for (CStringInputSection *isec : inputs) {
+    for (const auto &[i, piece] : llvm::enumerate(isec->pieces)) {
+      if (!piece.live)
+        continue;
+      uint32_t pieceAlign = 1 << getStringPieceAlignment(isec, piece);
+      offset = alignToPowerOf2(offset, pieceAlign);
+      piece.outSecOff = offset;
+      isec->isFinal = true;
+      StringRef string = isec->getStringRef(i);
+      offset += string.size() + 1; // account for null terminator
+    }
+  }
+  size = offset;
+}
+
 void DeduplicatedCStringSection::finalizeContents() {
   // Find the largest alignment required for each string.
   for (const CStringInputSection *isec : inputs) {
@@ -1754,7 +1755,7 @@ void DeduplicatedCStringSection::finalizeContents() {
         continue;
       auto s = isec->getCachedHashStringRef(i);
       assert(isec->align != 0);
-      uint8_t trailingZeros = llvm::countr_zero(isec->align | piece.inSecOff);
+      uint8_t trailingZeros = getStringPieceAlignment(isec, piece);
       auto it = stringOffsetMap.insert(
           std::make_pair(s, StringOffset(trailingZeros)));
       if (!it.second && it.first->second.trailingZeros < trailingZeros)

Copy link

github-actions bot commented Sep 29, 2025

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

@ellishg ellishg merged commit ccf1fb0 into main Sep 30, 2025
9 checks passed
@ellishg ellishg deleted the users/ellishg/lld-count-zero-helper branch September 30, 2025 16:44
ellishg added a commit that referenced this pull request Sep 30, 2025
Use `llvm::Align` instead of directly storing the shift amount for
clarity. Also remove the `DeduplicatedCStringSection::StringOffset` in
favor of simply storing the `uint64_t` offset since `trailingZeros` is
not used outside of `finalizeContents()`. These two changes allow us to
refactor `finalizeContents()`.


No function change intended.

Depends on #161241.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Sep 30, 2025
…(#161253)

Use `llvm::Align` instead of directly storing the shift amount for
clarity. Also remove the `DeduplicatedCStringSection::StringOffset` in
favor of simply storing the `uint64_t` offset since `trailingZeros` is
not used outside of `finalizeContents()`. These two changes allow us to
refactor `finalizeContents()`.

No function change intended.

Depends on llvm/llvm-project#161241.
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Oct 3, 2025
Move `llvm::countr_zero()` into a helper function to reduce code
duplication between `CStringSection` and `DeduplicatedCStringSection`.
More importantly, this moves a giant comment to that helper function
since it pertains to both classes.
mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Oct 3, 2025
Use `llvm::Align` instead of directly storing the shift amount for
clarity. Also remove the `DeduplicatedCStringSection::StringOffset` in
favor of simply storing the `uint64_t` offset since `trailingZeros` is
not used outside of `finalizeContents()`. These two changes allow us to
refactor `finalizeContents()`.


No function change intended.

Depends on llvm#161241.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants