[PATCH] D155045: [llvm-objdump] Create ObjectFile specific dumpers

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

This is definitely a big step in the right direction for llvm-objdump, in my opinion, and I look forward to further work in this area. My one concern with this patch is the Mach-O-specific parameter being passed into `printPrivateHeaders`, as that feels a bit ugly to me. I wonder if we could create the Mach-O dumper somehow with that parameter set in some manner? It would then no longer be a parameter of `printPrivateHeaders` that impacts all tools. Alternatively, there could be some other mechanism to give the dumper subclasses access to various configuration parameters.



================
Comment at: llvm/tools/llvm-objdump/COFFDump.cpp:40
 
-class COFFDumper {
+class COFFDumper : public Dumper {
 public:
----------------
MaskRay wrote:
> mtrofin wrote:
> > (Comment for discussion, not blocking this patch) should `ObjDumper` be reused by both readobj and objdump - this was prompted by my going to initially suggest - also for a potential future patch - renaming `Dumper` to something more specific (like `ObjDumper`) - then realized the name was taken. Or maybe one could be `ObjDumper` and the other `ReadObjDumper`?
> > 
> > Anyway, this can go offline on discourse.
> Thank you for bring up the topic. I prefer a different name `Dumper` for llvm-objdump because:
> 
> * `llvm::ObjDumper` is taken by llvm-readobj.
> * We cannot reuse `llvm::ObjDumper` for llvm-objdump, so a separate class is needed.
> * `llvm::objdump::ObjDumper` is probably not a good idea, considering that we do `using namespace llvm;`
> 
> Therefore, I picked `llvm::objdump::Dumper` for now.
> 
> Classes like `ELFDumper` in an unnamed namespace do not suffer ODR violation problems, so name collision is fine.
> 
> I agree that renaming `llvm::ObjDumper` to be something like `llvm::ReadObjDumper` or adding to a new namespace makes sense. That change should be a separate refactoring for llvm-readobj.
I'd love for there to be a way to share more llvm-readobj and llvm-objdump code. However, my suspicion is that it isn't that easy to do because the two have different outputs for most (all?) of their options, at least for ELF. Perhaps sharing a virtual interface, but having app-specific dumpers (which are then further specialized in some manner by file format) would be a nice thing, but I'm not sure how much it would give us.

Either way, `llvm::objdump::Dumper` seems like a reasonable name for me. We could make the llvm-readobj version `llvm::readobj::Dumper` and create a `readobj` namespace for similarity, but I don't think that bit is necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155045