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

Jordan Rupprecht via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 21 13:09:02 PDT 2019


rupprecht marked 2 inline comments as done.
rupprecht added a comment.

LGTM, once a MachO reviewer signs off. No action needed for my two 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())
----------------
seiya wrote:
> 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).
> 
Aside: woah, didn't realize phab could display cool tabbed codeblocks like that.

Yes, that's a decent sketch for how it might work. CopyConfig itself should be returning uninterpreted StringRefs instead of ELF/MachO-specific things.


================
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));
----------------
seiya wrote:
> 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`?
> 
Yeah, `stable_partition`, good catch.

In retrospect, I think the use of ELF/removeSections actually may not need to use `stable_partition`, and switching to `remove_if` may be faster. IIUC the difference is that stable_partition guarantees the things past the partition point are also in stable order, whereas remove_if does not (and is faster for not having to). So disregard my original suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541





More information about the llvm-commits mailing list