[llvm] remove empty __LLVM segment in llvm-bitcode-strip

[rmaz]

Summary

When running llvm-bitcode-strip we want to remove the __LLVM
segment as well as the __bundle section when there are no other
sections in the segment.

[rmaz]

update test comment

[jhenderson]

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove-nonempty-segment.test

4–5

Ideally, we'd use the same invocation to check everything at once. I believe llvm-readobj has an option to dump Mach-O segments. Does that suffice?

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove.test

11

It's usually a bad idea to have a purely negative check like this, because they can rot too easily. Better would be to check all segments and show the one you want isn't in that set.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

1239

its - belonging to it
it's - it is

[rmaz]

Single pass tests

[rmaz]

Use llvm-readobj --macho-segment

[jhenderson]

LGTM, with one change.

By the way, if you haven't already, you've probably done enough to request commit access.

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove.test

14

No need for this line: 1) the --implicit-check-not covers it. 2) This won't catch __LLVM appearing before or between the other segments.

[rmaz]

Remove unnecessary CHECK-NOT

[rmaz]

With linter

llvm/include/llvm/ObjCopy/MachO/MachOConfig.h

struct MachOConfig {
   std::vector<StringRef> RPathToPrepend;
   DenseMap<StringRef, StringRef> RPathsToUpdate;
   DenseMap<StringRef, StringRef> InstallNamesToUpdate;
   DenseSet<StringRef> RPathsToRemove;
 
   // install-name-tool's id option
   Optional<StringRef> SharedLibId;
 
+  // Segments to remove if they are empty
+  DenseSet<StringRef> EmptySegmentsToRemove;
+
   // Boolean options
   bool StripSwiftSymbols = false;
   bool KeepUndefined = false;
 
   // install-name-tool's --delete_all_rpaths
   bool RemoveAllRpaths = false;
 };
 
 } // namespace objcopy
 } // namespace llvm
 
 #endif // LLVM_OBJCOPY_MACHO_MACHOCONFIG_H

llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp

Obj.LoadCommands.insert(Obj.LoadCommands.begin(),
                             buildRPathLoadCommand(RPath));
   }
 
   // Unlike appending rpaths, the indexes of subsequent load commands must
   // be recalculated after prepending one.
   if (!MachOConfig.RPathToPrepend.empty())
     Obj.updateLoadCommandIndexes();
 
+  // Remove any empty segments if required.
+  if (!MachOConfig.EmptySegmentsToRemove.empty()) {
+    auto RemovePred = [&MachOConfig](const LoadCommand &LC) {
+      if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_SEGMENT_64 ||
+          LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_SEGMENT) {
+        return LC.Sections.empty() &&
+               MachOConfig.EmptySegmentsToRemove.contains(
+                   LC.getSegmentName().getValue());
+      }
+      return false;
+    };
+    if (Error E = Obj.removeLoadCommands(RemovePred))
+      return E;
+  }
+
   return Error::success();
 }
 
 static Error dumpSectionToFile(StringRef SecName, StringRef Filename,
                                Object &Obj) {
   for (LoadCommand &LC : Obj.LoadCommands)
     for (const std::unique_ptr<Section> &Sec : LC.Sections) {
       if (Sec->CanonicalName == SecName) {

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove-nonempty-segment.test

-## Test bitcode section removal.
+## Test bitcode segment is not removed when not empty.
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-bitcode-strip -r %t -o %t2
-# RUN: llvm-readobj --sections %t2 | FileCheck --implicit-check-not=Name: %s
+# RUN: llvm-readobj --macho-segment --sections %t2 | FileCheck --implicit-check-not=Name: %s
 
 # CHECK:      Name: __text
 # CHECK-NEXT: Segment: __TEXT
 # CHECK:      Name: __bundle
 # CHECK-NEXT: Segment: __DATA
 # CHECK:      Name: __notbundle
 # CHECK-NEXT: Segment: __LLVM
+# CHECK:      Cmd: LC_SEGMENT_64
+# CHECK-NEXT: Name: __TEXT
+# CHECK:      Cmd: LC_SEGMENT_64
+# CHECK-NEXT: Name: __DATA
+# CHECK:      Cmd: LC_SEGMENT_64
+# CHECK-NEXT: Name: __LLVM
 
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x00000003
   filetype:        0x00000001
-  ncmds:           1
-  sizeofcmds:      392
+  ncmds:           3
+  sizeofcmds:      536
   flags:           0x00002000
   reserved:        0x00000000
 LoadCommands:
   - cmd:             LC_SEGMENT_64
-    cmdsize:         392
-    segname:         ''
+    cmdsize:         152
+    segname:         __TEXT
     vmaddr:          0
-    vmsize:          16
-    fileoff:         424
-    filesize:        16
+    vmsize:          4
+    fileoff:         568
+    filesize:        4
     maxprot:         7
     initprot:        7
-    nsects:          4
+    nsects:          1
     flags:           0
     Sections:
       - sectname:        __text
         segname:         __TEXT
         addr:            0x0000000000000000
         content:         'AABBCCDD'
         size:            4
-        offset:          424
+        offset:          568
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x80000400
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __DATA
+    vmaddr:          4
+    vmsize:          4
+    fileoff:         572
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
       - sectname:        __bundle
         segname:         __DATA
         addr:            0x0000000000000004
         content:         'DDAADDAA'
         size:            4
-        offset:          428
+        offset:          572
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x00000000
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         232
+    segname:         __LLVM
+    vmaddr:          8
+    vmsize:          8
+    fileoff:         576
+    filesize:        8
+    maxprot:         7
+    initprot:        7
+    nsects:          2
+    flags:           0
+    Sections:
       - sectname:        __bundle
         segname:         __LLVM
         addr:            0x0000000000000008
         content:         'EEFFEEFF'
         size:            4
-        offset:          432
+        offset:          576
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x00000000
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
       - sectname:        __notbundle
         segname:         __LLVM
         addr:            0x0000000000000008
         content:         'EEFFEEFF'
         size:            4
-        offset:          436
+        offset:          580
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x00000000
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000

llvm/test/tools/llvm-objcopy/MachO/bitcode-strip-remove.test

-## Test bitcode section removal.
+## Test bitcode section and segment removal.
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-bitcode-strip -r %t -o %t2
-# RUN: llvm-readobj --sections %t2 | FileCheck --implicit-check-not=Name: %s
+# RUN: llvm-readobj --macho-segment --sections %t2 | FileCheck --implicit-check-not=Name: %s
 
 # CHECK:      Name: __text
 # CHECK-NEXT: Segment: __TEXT
 # CHECK:      Name: __bundle
 # CHECK-NEXT: Segment: __DATA
-# CHECK:      Name: __notbundle
-# CHECK-NEXT: Segment: __LLVM
+# CHECK:      Cmd: LC_SEGMENT_64
+# CHECK-NEXT: Name: __TEXT
+# CHECK:      Cmd: LC_SEGMENT_64
+# CHECK-NEXT: Name: __DATA
 
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x00000003
   filetype:        0x00000001
-  ncmds:           1
-  sizeofcmds:      392
+  ncmds:           3
+  sizeofcmds:      456
   flags:           0x00002000
   reserved:        0x00000000
 LoadCommands:
   - cmd:             LC_SEGMENT_64
-    cmdsize:         392
-    segname:         ''
+    cmdsize:         152
+    segname:         __TEXT
     vmaddr:          0
-    vmsize:          16
-    fileoff:         424
-    filesize:        16
+    vmsize:          4
+    fileoff:         488
+    filesize:        4
     maxprot:         7
     initprot:        7
-    nsects:          4
+    nsects:          1
     flags:           0
     Sections:
       - sectname:        __text
         segname:         __TEXT
         addr:            0x0000000000000000
         content:         'AABBCCDD'
         size:            4
-        offset:          424
+        offset:          488
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x80000400
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __DATA
+    vmaddr:          4
+    vmsize:          4
+    fileoff:         492
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
       - sectname:        __bundle
         segname:         __DATA
         addr:            0x0000000000000004
         content:         'DDAADDAA'
         size:            4
-        offset:          428
+        offset:          492
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x00000000
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
-      - sectname:        __bundle
-        segname:         __LLVM
-        addr:            0x0000000000000008
-        content:         'EEFFEEFF'
-        size:            4
-        offset:          432
-        align:           0
-        reloff:          0x00000000
-        nreloc:          0
-        flags:           0x00000000
-        reserved1:       0x00000000
-        reserved2:       0x00000000
-        reserved3:       0x00000000
-      - sectname:        __notbundle
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    vmaddr:          8
+    vmsize:          4
+    fileoff:         496
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __bundle
         segname:         __LLVM
         addr:            0x0000000000000008
         content:         'EEFFEEFF'
         size:            4
-        offset:          436
+        offset:          496
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x00000000
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

 //===- ObjcopyOptions.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ObjcopyOptions.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/ObjCopy/CommonConfig.h"
 #include "llvm/ObjCopy/ConfigManager.h"
+#include "llvm/ObjCopy/MachO/MachOConfig.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/CRC.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"

 }
 
 Expected<DriverConfig>
 objcopy::parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr,
                                   function_ref<Error(Error)> ErrorCallback) {
   DriverConfig DC;
   ConfigManager ConfigMgr;
   CommonConfig &Config = ConfigMgr.Common;
+  MachOConfig &MachOConfig = ConfigMgr.MachO;
   BitcodeStripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     printHelp(T, errs(), ToolType::BitcodeStrip);
     exit(1);
if (!InputArgs.hasArg(BITCODE_STRIP_output)) {
     return createStringError(errc::invalid_argument,
                              "-o is a required argument");
   }
   Config.OutputFilename = InputArgs.getLastArgValue(BITCODE_STRIP_output);
 
   if (!InputArgs.hasArg(BITCODE_STRIP_remove))
     return createStringError(errc::invalid_argument, "no action specified");
 
-  // We only support -r for now, which removes all bitcode sections.
+  // We only support -r for now, which removes all bitcode sections and
+  // the __LLVM segment if it's now empty.
   cantFail(Config.ToRemove.addMatcher(NameOrPattern::create(
       "__LLVM,__bundle", MatchStyle::Literal, ErrorCallback)));
+  MachOConfig.EmptySegmentsToRemove.insert("__LLVM");
 
   DC.CopyConfigs.push_back(std::move(ConfigMgr));
   return std::move(DC);
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.