[llvm] add -r functionality to llvm-bitcode-strip

[rmaz]

Summary

This diff adds functionality to the llvm-bitcode-strip tool for
stripping of LLVM bitcode sections.

[jhenderson]

Please split this patch in two, one patch for each option. I'll then review the two independently.

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

1–2

This comment doesn't line up with what the test is doing.

[rmaz]

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

1–2

Whoops, should have removed

[rmaz]

split out -o into separate diff D120731

[jhenderson]

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

6–9

Usually, we'd have one test file per option, i.e. in one test, you'd test -o, and another test, you'd test what the -r operation does.

14

I don't think you need to dump the entire section data. Sufficient should be to check the Name: and possibly Segment: fields (and then only the actual names, not the raw values you're currently checking too).

Example:

# RUN: ... | FileCheck %s --implicit-check-not=Name

# CHECK:      Name: __text
# CHECK-NEXT: Segment: __TEXT
# CHECK:      Name: __data
# CHECK-NEXT: Segment: __DATA

llvm/tools/llvm-objcopy/BitcodeStripOpts.td

27

Please normalise the help text to be consistent with ending with a period or not. I prefer not for help text.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

1220–1223

No braces for single-line if statements. I take it in the future, other options will provide different actions, and you just have to request one (or more) actions from the available set?

1225–1229

Period at end of sentences. Also, no braces for single-line if statements.

I think you need additional sections in your test case with a section in the __LLVM segment with a different name, and a __bundle section in a non-LLVM segment, to show that it's only the combination of the two that is removed.

It may also be a good idea to show what happens (in a test case) when the error callback is triggered in this code path.

[rmaz]

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

1220–1223

Yes, there are 2 other output options that I have not yet added:

-m     Remove the bitcode from the __LLVM segment, leaving behind a marker.

-l     Remove all of the native executable code, leaving the LLVM bitcode behind. In
       this case, bitcode_strip will take care to preserve the (__TEXT,__info_plist)
       section while removing the rest of the __TEXT segment.

[rmaz]

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

6–9

The reason for using a single test is that you cannot use -o without -r, so all the -o test could do would be to test you get an error without -r.

[rmaz]

• add test case for LLVM section with not bundle name
• add test case for __bundle section in other segment
• address formatting comments

[rmaz]

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

1225–1229

I don't believe you can trigger the error callback on this code path as we are using a MatchStyle::Literal.

[jhenderson]

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

6–9

Thanks. When I made this and related comments, I didn't realise that you wanted to make -r a cumpolsory argument. Given that other commands are planned, I'd suggest the following test file breakdown between this and D120731:
bitcode-strip-basic.test - contains tests that show the -o option is required and that at least one command is required. You don't need to show what the behaviour of that command is in this test, other than it suppresses the error.
bitcode-strip-r.test (or similar longer name spelling out what -r does) - contains tests that show the behaviour of -r.

8

Nit: comments should end with a period. Also, you can delete "output flag and" from this comment: the previous test case covers this point, so no need for this test case to explicitly mention that too.

I'd also suggest deleting the blank line between comment and run command, since the two are tied together, rather than the comment being a general comment about the whole test file.

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

1225–1229

Thanks. Can the if statement ever fail though? (I'm not sure if the ErrorCallback and returned Error are tied together or not). If not, I'd change it to cantFail.

[rmaz]

• split test into basic and remove
• cantFail instead of checking error that cannot occur

[rmaz]

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

1225–1229

The if statement can only fail if the Matcher fails to create, which for a literal matcher cannot happen. Updated to cantFail.

[jhenderson]

LGTM.

[jhenderson]

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

4 ↗

(On Diff #412731)

Actually, you should add a colon to the end of the --implicit-check-not command. Otherwise, this might accidentally match a part of someone's local directory tree.

[rmaz]

add : to implicit check not

[rmaz]

with linter

[rmaz]

update test for windows

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

 ## Test output flag is required.
 
 # RUN: yaml2obj %s -o %t
 # RUN: not llvm-bitcode-strip %t 2>&1 | FileCheck --check-prefix=MISSING-ARG %s
-# RUN: llvm-bitcode-strip %t -o %t2
-# RUN: cmp %t %t2
 
 # MISSING-ARG: llvm-bitcode-strip: error: llvm-bitcode-strip expects a single output path specified with -o
 
+## Test output flag and action flag are required
+
+## Test bitcode section removal.
+
+# RUN: llvm-bitcode-strip -r %t -o %t2
+# RUN: llvm-readobj --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
+
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x00000003
   filetype:        0x00000001
   ncmds:           1
-  sizeofcmds:      152
+  sizeofcmds:      392
   flags:           0x00002000
   reserved:        0x00000000
 LoadCommands:
   - cmd:             LC_SEGMENT_64
-    cmdsize:         152
-    segname:         __TEXT
+    cmdsize:         392
+    segname:         ''
     vmaddr:          0
-    vmsize:          4
-    fileoff:         184
-    filesize:        4
+    vmsize:          16
+    fileoff:         424
+    filesize:        16
     maxprot:         7
     initprot:        7
-    nsects:          1
+    nsects:          4
     flags:           0
     Sections:
       - sectname:        __text
         segname:         __TEXT
         addr:            0x0000000000000000
         content:         'AABBCCDD'
         size:            4
-        offset:          184
+        offset:          424
         align:           0
         reloff:          0x00000000
         nreloc:          0
         flags:           0x80000400
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
+      - sectname:        __bundle
+        segname:         __DATA
+        addr:            0x0000000000000004
+        content:         'DDAADDAA'
+        size:            4
+        offset:          428
+        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
+        segname:         __LLVM
+        addr:            0x0000000000000008
+        content:         'EEFFEEFF'
+        size:            4
+        offset:          436
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x00000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000

llvm/tools/llvm-objcopy/BitcodeStripOpts.td

 
 def version : Flag<["--"], "version">,
               HelpText<"Print the version and exit">;
 
 def V : Flag<["-"], "V">,
         Alias<version>,
         HelpText<"Alias for --version">;
 
+def remove : Flag<["-"], "r">,
+        HelpText<"Remove the __LLVM bitcode segment entirely">;
+
 def output : JoinedOrSeparate<["-"], "o">, HelpText<"Write output to <file>">,
              MetaVarName<"<file>">;

llvm/tools/llvm-objcopy/ObjcopyOptions.h

 // If a help flag is set then ParseInstallNameToolOptions will print the help
 // messege and exit.
 Expected<DriverConfig>
 parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr);
 
 // ParseBitcodeStripOptions returns the config and sets the input arguments.
 // If a help flag is set then ParseBitcodeStripOptions will print the help
 // messege and exit.
-Expected<DriverConfig> parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr);
+Expected<DriverConfig>
+parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr,
+                         llvm::function_ref<Error(Error)> ErrorCallback);
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit. ErrorCallback is used to handle recoverable errors. An Error returned
 // by the callback aborts the parsing and is then returned by this function.
 Expected<DriverConfig>
 parseStripOptions(ArrayRef<const char *> ArgsArr,
                   llvm::function_ref<Error(Error)> ErrorCallback);
 } // namespace objcopy
 } // namespace llvm
 
 #endif // LLVM_TOOLS_LLVM_OBJCOPY_OBJCOPYOPTIONS_H

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/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"
objcopy::parseInstallNameToolOptions(ArrayRef<const char *> ArgsArr) {
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[0];
 
   DC.CopyConfigs.push_back(std::move(ConfigMgr));
   return std::move(DC);
 }
 
 Expected<DriverConfig>
-objcopy::parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr) {
+objcopy::parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr,
+                                  function_ref<Error(Error)> ErrorCallback) {
   DriverConfig DC;
   ConfigManager ConfigMgr;
   CommonConfig &Config = ConfigMgr.Common;
   BitcodeStripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
objcopy::parseBitcodeStripOptions(ArrayRef<const char *> ArgsArr,
 
   if (!InputArgs.hasArg(BITCODE_STRIP_output)) {
     return createStringError(
         errc::invalid_argument,
         "llvm-bitcode-strip expects a single output path specified with -o");
   }
   Config.OutputFilename = InputArgs.getLastArgValue(BITCODE_STRIP_output);
 
+  if (!InputArgs.hasArg(BITCODE_STRIP_remove))
+    return createStringError(errc::invalid_argument,
+                             "llvm-bitcode-strip has no action specified");
+
+  // We only support -r for now, which removes all bitcode sections.
+  if (Error E = Config.ToRemove.addMatcher(NameOrPattern::create(
+          "__LLVM,__bundle", MatchStyle::Literal, ErrorCallback)))
+    return std::move(E);
+
   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.
 Expected<DriverConfig>
 objcopy::parseStripOptions(ArrayRef<const char *> RawArgsArr,

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

auto Is = [=](StringRef Tool) {
     // powerpc64-unknown-freebsd13-objcopy -> objcopy
     // llvm-install-name-tool -> install-name-tool
     auto I = Stem.rfind_insensitive(Tool);
     return I != StringRef::npos &&
            (I + Tool.size() == Stem.size() || !isAlnum(Stem[I + Tool.size()]));
   };
 
   if (Is("bitcode-strip") || Is("bitcode_strip"))
-    return parseBitcodeStripOptions(Args);
+    return parseBitcodeStripOptions(Args, reportWarning);
   else if (Is("strip"))
     return parseStripOptions(Args, reportWarning);
   else if (Is("install-name-tool") || Is("install_name_tool"))
     return parseInstallNameToolOptions(Args);
   else
     return parseObjcopyOptions(Args, reportWarning);
 }