[PATCH] D65541: [llvm-objcopy][MachO] Implement --only-section

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:419
+  if (Name.count(',') != 1)
+    return createStringError(errc::invalid_argument,
+                             "invalid section name (should be formatted as "
----------------
These errors should also include the full `Name` string so the user knows specifically which flag is the culprit.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:35-46
+static Error validateOptions(const CopyConfig &Config) {
+  // TODO: Support section renaming in GNU objcopy for compatibility (see
+  // http://lists.llvm.org/pipermail/llvm-dev/2019-May/132570.html).
+
+  if (!Config.OnlySection.empty()) {
+    for (const NameOrRegex &NR : Config.OnlySection)
+      if (Error E = NR.isMachOCannonicalName())
----------------
Aside: we still have a lot of ELF-specific stuff in CopyConfig, which should theoretically be relatively platform-agnostic -- for example, the `--add-symbol` parsing code creates functors that assign the symbol binding to `ELF::STB_*` values. We should think about how to properly separate ELF/MachO/COFF specific things there.


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.cpp:15-17
+    LC.Sections.erase(std::remove_if(std::begin(LC.Sections),
+                                     std::end(LC.Sections), ToRemove),
+                      std::end(LC.Sections));
----------------
SymTable/StrTable are explicitly referenced in MachO/Object.h, so if either of those are matched by the predicate, those tables should be cleared out too. (And there should be test cases to verify this).

Also, you might want to use stable_sort to remove sections -- see ELF's implementation.

Although specific to --only-section (see `replaceAndRemoveSections`, ELFObjcopy.cpp), StrTable/SymTab should not be implicitly removed, so it's not really observable with just this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541