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

[Seiya Nuta via Phabricator via llvm-commits]

seiya added inline comments.


================
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())
----------------
rupprecht wrote:
> 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.
I think so too. What complicates the problem is that we don't know the file format until we parse the command line options and decode the input file. How about splitting the command-line parsing into two stages?

```
lang=cpp, name=CopyConfig.h

struct UnparsedNewSymbolInfo {
  StringRef Bind; // Don't parse it for now.
  // ...
};

class CopyConfig {
  // Format-independent options.
  StringRef InputFilename;
  FileFormat InputFormat;
  MachineInfo BinaryArch;

  // ... 

  // Format-specific options.
  UnparsedNewSymbolInfo NewSymbolInfo;
};
```

```
lang=cpp, name=ELFCopyConfig.h

struct NewSymbolInfo {
  uint8_t Type = ELF::STB_GLOBAL;
  // ...
};

class ELFCopyConfig : public CopyConfig {
  ELFCopyConfig(CopyConfig &Config) {
    // We should cache the result in case there're many input files to process.
    parse(Config);
  }
};
```

```
lang=cpp, name=ELFObjcopy.cpp
Error executeObjcopyOnBinary(const CopyConfig &UnparsedConfig, ...) {
  ELFCopyConfig Config(UnparsedConfig);
  // ...
}
```

In the first stage (CopyConfig), we parse only format-independent options. In second stage (ELFCopyConfig), we parse format-specific options (the `Bind` field, in this example).



================
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));
----------------
rupprecht wrote:
> 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.
> 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).

Unlike ELF, in MachO, SymTable/StrTable are not sections, but are load commands. Thus, we never remove them by this `removeSections`.

> Also, you might want to use stable_sort to remove sections -- see ELF's implementation.
Perhaps you meant `stable_partition`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541