[PATCH] D120669: [llvm] add -r functionality to llvm-bitcode-strip

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.
Herald added a project: All.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:7-10
+## Test bitcode section removal
+
+# RUN: llvm-bitcode-strip -r %t -o %t2
+# RUN: llvm-readobj --sections %t2 | FileCheck %s
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:15
+# CHECK-NEXT:    Index: 0
+# CHECK-NEXT:    Name: __text (5F 5F 74 65 78 74 00 00 00 00 00 00 00 00 00 00)
+# CHECK-NEXT:    Segment: __TEXT (5F 5F 54 45 58 54 00 00 00 00 00 00 00 00 00 00)
----------------
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
```


================
Comment at: llvm/tools/llvm-objcopy/BitcodeStripOpts.td:27
+def remove : Flag<["-"], "r">,
+        HelpText<"Remove the __LLVM bitcode segment entirely.">;
+
----------------
Please normalise the help text to be consistent with ending with a period or not. I prefer not for help text.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:1220-1223
+  if (!InputArgs.hasArg(BITCODE_STRIP_remove)) {
+    return createStringError(errc::invalid_argument,
+                             "llvm-bitcode-strip has no action specified");
+  }
----------------
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?


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:1225-1229
+  // 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);
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120669/new/

https://reviews.llvm.org/D120669