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

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked an inline comment as done.
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:
> 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.
OK, I'll propose it in a separate patch.

(I've been wanting to try this feature. It's neat.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541