release/20.x: [llvm-objcopy] Apply encryptable offset to first segment, not section (#130517)

[llvmbot]

Backport 8413f4d

Requested by: @nikic

[llvmbot]

@drodriguez What do you think about merging this PR to the release branch?

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (llvmbot)

Changes

Backport 8413f4d

Requested by: @nikic

---

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

2 Files Affected:

• (modified) llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp (+8-5)
• (modified) llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test (+9-3)

diff --git a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
index d4eb6a9b9fc0b..8ecd669e67178 100644
--- a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
@@ -116,11 +116,10 @@ uint64_t MachOLayoutBuilder::layoutSegments() {
   const bool IsObjectFile =
       O.Header.FileType == MachO::HeaderFileType::MH_OBJECT;
   uint64_t Offset = IsObjectFile ? (HeaderSize + O.Header.SizeOfCmds) : 0;
-  if (O.EncryptionInfoCommandIndex) {
-    // If we are emitting an encryptable binary, our load commands must have a
-    // separate (non-encrypted) page to themselves.
-    Offset = alignToPowerOf2(HeaderSize + O.Header.SizeOfCmds, PageSize);
-  }
+  // If we are emitting an encryptable binary, our load commands must have a
+  // separate (non-encrypted) page to themselves.
+  bool RequiresFirstSectionOutsideFirstPage =
+      O.EncryptionInfoCommandIndex.has_value();
   for (LoadCommand &LC : O.LoadCommands) {
     auto &MLC = LC.MachOLoadCommand;
     StringRef Segname;
@@ -174,6 +173,10 @@ uint64_t MachOLayoutBuilder::layoutSegments() {
         if (!Sec->hasValidOffset()) {
           Sec->Offset = 0;
         } else {
+          if (RequiresFirstSectionOutsideFirstPage) {
+            SectOffset = alignToPowerOf2(SectOffset, PageSize);
+            RequiresFirstSectionOutsideFirstPage = false;
+          }
           Sec->Offset = SegOffset + SectOffset;
           Sec->Size = Sec->Content.size();
           SegFileSize = std::max(SegFileSize, SectOffset + Sec->Size);
diff --git a/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test b/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test
index 19b06b1ec02c8..2b2bd670613de 100644
--- a/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test
+++ b/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test
@@ -1,13 +1,19 @@
 # RUN: rm -rf %t && mkdir %t
 # RUN: yaml2obj %s -o %t/original
 # RUN: llvm-strip --strip-all %t/original -o %t/stripped
-# RUN: llvm-readobj --macho-segment %t/stripped | FileCheck %s
+# RUN: llvm-readobj --macho-segment --section-headers %t/stripped | FileCheck %s
+
+# CHECK-LABEL: Sections [
+# CHECK:      Index: 0
+# CHECK-NEXT: Name: __text
+# CHECK-NEXT: Segment: __TEXT
+# CHECK:      Offset: 16384
 
 # CHECK-LABEL: Name: __PAGEZERO
-# CHECK:       fileoff: 16384
+# CHECK:       fileoff: 0
 
 # CHECK-LABEL: Name: __TEXT
-# CHECK:       fileoff: 16384
+# CHECK:       fileoff: 0
 
 # The YAML below is the following code
 # int main(int argc, char **argv) { return 0; }

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[drodriguez]

@drodriguez What do you think about merging this PR to the release branch?

@nikic I imagine this is you asking and not a sentient robot? :D

I think the fix is safe enough for cherry-picking into the release branch. A couple of people in the issue, PR, and related issues I have found had confirmed it works for them. Not sure if it classifies as important enough for cherry-picking into the release branch, but it is true that creates binaries that might be rejected from some tooling (but not the dynamic loaded itself in my experience).

In summary: not opposed to cherry-picking into the release branch, but also I will not be offended if this is not considered important enough.

[TimNN]

but not the dynamic loaded itself in my experience

We actually found this because the resulting binaries fail to launch on iOS: rust-lang/rust#138212

[drodriguez]

That's why I added "in my experience". I never got a failing binary, but there's yours and another report in the original PR. I never got a failing binary, but I cannot give you a reason why. The dyld_info analysis is pretty clear, but it is sometimes more strict than dyld itself (again, in my experience).

I will accept to unblock. Whoever needs to decide if this is release-worthy can make an informed decision.

[github-actions]

@nikic (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.