This patch splits the command-line parsing into two phases:
First, parse cross-platform options and leave ELF-specific options unparsed.
Second, in the ELF implementation, parse ELF-specific options and construct ELFCopyConfig.
Paths
| Differential D67139
[llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC. ClosedPublic Authored by seiya on Sep 3 2019, 7:22 PM.
Details Summary This patch splits the command-line parsing into two phases: First, parse cross-platform options and leave ELF-specific options unparsed. Second, in the ELF implementation, parse ELF-specific options and construct ELFCopyConfig.
Diff Detail Event TimelineComment Actions just want to mention a slightly different approach: CopyConfig.h: // Forward declarations only. struct COFFCopyConfig; struct ELFCopyConfig; struct MachOCopyConfig; struct CopyConfig { ... Optional<StringRef> NewSymbolVisibility; std::vector<StringRef> SymbolsToAdd; std::unique_ptr<COFFCopyConfig> COFF; std::unique_ptr<ELFCopyConfig> ELF; std::unique_ptr<MachOCopyConfig> MachO; }; ELFObjcopy.h: struct ELFCopyConfig { Optional<uint8_t> NewSymbolVisibility; std::vector<NewSymbolInfo> SymbolsToAdd }; Error parseELFCopyConfig(CopyConfig &C); Error executeObjcopyOnBinary( CopyConfig &C, object::ELFObjectFileBase &In, Buffer &Out); What do you think ? Comment Actions I've got another suggestion: have separate COFFConfig/ELFConfig etc classes that only contain the interpreted versions and lazily construct them prior to calling the corresponding code path. CopyConfig would contain the raw unparsed strings (perhaps rename them to "RawSymbolsToAdd" etc). Perhaps a higher-level Config class could exist just to wrap them and pass them to executeObjcopyOnBinary etc. Rough outline: CopyConfig.h: class CopyConfig { std::vector<StringRef> RawSymbolsToAdd; ... }; class ELFConfig { std::vector<NewSymbolInfo> SymbolsToAdd; ... }; // etc for COFF, Mach-O... llvm-objcopy.cpp: struct Configurations { CopyConfig Common; Optional<ELFConfig> ELF; ... } ... static Error executeObjcopyOnBinary(Configurations &Cfgs, ...) { if (/*isElf*/...) { if (!Cfgs.ELF) Cfgs.ELF.emplace(Cfgs.Common); return elf::executeObjcopyOnBinary(Cfgs.Common, Cfgs.ELF, ...) } ... } On a related note, I think the ELF-specific etc parsing code belongs in a file other than ELFObjcopy.cpp. I feel like that file should be confined to doing the actual objcopy processing, whereas the command-line option parsing etc should be separate, e.g. in a new file called ELFConfig.cpp etc. What do you think? Comment Actions
LGTM. I'll implement the idea.
I agree with this. I'll add ELFConfig.cpp to separate the command-line parsing.
This revision is now accepted and ready to land.Sep 13 2019, 2:28 AM
Comment Actions I don't have a strong feeling about this either way, if I'm honest. I'm happy for alternative approaches, if @alexshap prefers.
Comment Actions
How about this design? I believe moving the CopyConfig is not a big cost. /// Parses CopyConfig and creates a ELFCopyConfig, or returns Error on parse errors. class ELFCopyConfigBuilder { CopyConfig Common; public: ELFCopyConfig(CopyConfig&& Common) : Common(std::move(Common)){}; Expected<ELFCopyConfig> build(); } class ELFCopyConfig : public ELFConfig { public: Optional<uint8_t> NewSymbolVisibility; std::vector<NewSymbolInfo> SymbolsToAdd; ELFCopyConfig(CopyConfig&& Common, Optional<uint8_t> NewSymbolVisibility, /* ... */) : CopyConfig(std::move(Common)), NewSymbolVisibility(NewSymbolVisibility) /* ... */ {} }; Comment Actions
I thought of that, but if I'm not mistaken, that won't help us when we have a mixture of file formats, e.g. ELF and COFF. That could be a problem in some corner cases (e.g. archives containing objects of different formats). I think Seiya's one has the same problem - we can only use the common config once, not multiple times. If it's not a big deal to create copies of CopyConfig, then I don't mind doing it using inheritance: construct an ELFCopyConfig from the CopyConfig, but not std::move it. That would allow the original CopyConfig to continue to be used later. Comment Actions for what it's worth - i was thinking about it for some time, Comment Actions Thanks you for feedbacks. Putting it all together, the @alexshap's suggestion (D67139#inline-607235) looks desirable to me because:
I'll update the patch but comments are still welcome of course. Closed by commit rL372712: [llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC. (authored by seiya). · Explain WhySep 24 2019, 2:37 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 220054 llvm/tools/llvm-objcopy/CMakeLists.txt
llvm/tools/llvm-objcopy/CopyConfig.h
llvm/tools/llvm-objcopy/CopyConfig.cpp
llvm/tools/llvm-objcopy/ELF/ELFConfig.h
llvm/tools/llvm-objcopy/ELF/ELFConfig.cpp
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.h
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
llvm/tools/llvm-objcopy/llvm-objcopy.cpp
|
Since ELFConfig has Config as a member, you could just pass in ELFConfig in this and the other functions here, and use that.