[llvm-objdump] Create ObjectFile specific dumpers

[MaskRay]

Summary

We pay the one-off boilerplate overhead to create *Dumper classes that derive
from objdump::Dumper a la llvm-readobj. This has two primary advantages.

First, a lot object file format specific code can be moved from
llvm-objdump.cpp to *Dump.cpp files. Refactor printPrivateHeaders as
an example.

Second, with the introduction of ELFDumper<ELFT>, we can simplify
a few dispatch functions in ELFDump.cpp.

In addition, the ObjectFile specific dumpers contains a ObjectFile specific
reference so that we can remove a lot of cast<*ObjectFile>(Obj).

[MaskRay]

drop an unneeded change. Change an instance in MachoDump.cpp

[mtrofin]

lgtm, some nits.

llvm/tools/llvm-objdump/COFFDump.cpp

40

(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.

llvm/tools/llvm-objdump/ELFDump.cpp

44

does this buy much - looks like objdump::createELFDumper below would just look about the same, and (subjectively) even maybe a bit more clear?

[MaskRay]

Thanks for the review. There are recently a lot of XCOFF llvm-objdump patches and I unfortunately don't have much time reading them, but a pattern I noticed is that llvm-objdump.cpp gets more and more object file format specific functions, which is probably not a good sign.

I'll land this patch soon to hopefully help object file format specific functions reside in *Dump.cpp` files. I think @jhenderson would agree with the direction and am happy to address his post-commit suggestions.

llvm/tools/llvm-objdump/COFFDump.cpp

40

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.

llvm/tools/llvm-objdump/ELFDump.cpp

44

This function is entirely used for the template argument deduction, so that I don't need to do return createDumper<Dumper<ELF32LE>>(*O); below, with the risk of making a template argument mistake...

[jhenderson]

I just got back from camping for the past week or so. Will take a look at this in the next few days.

[jhenderson]

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.

llvm/tools/llvm-objdump/COFFDump.cpp

40

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.

[MaskRay]

In D155045#4532319, @jhenderson wrote:

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.

Thank you for the comment! D.printPrivateHeaders(FirstPrivateHeader); is indeed hackish to get things done. I'll check how it can be improved.