[PATCH] D52821: [Disassembler][llvm-readobj] ELF note dumper abstraction

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3811
+        }
+      }
+      if (!Dumped)
----------------
tpr wrote:
> compnerd wrote:
> > tpr wrote:
> > > compnerd wrote:
> > > > tpr wrote:
> > > > > compnerd wrote:
> > > > > > I'm not sure if I really like this approach.  We already have the name that identifies who should interpret the note.  There is nothing that prevents the use of the `aarch64-unknown-windows-msvc-elf` to use the "AMD" vendor namespace to add a note.  However, I don't think that any target registered will have that triple.  How would that work?
> > > > > This code just asks each registered (i.e. compiled in) target if it understands the note, and ignores the triples. So if the aarch64 target is compiled in and it decides to dump an AMD note type, it can.
> > > > > 
> > > > > We have the vendor name from the note, but that does not help identify the target.
> > > > > 
> > > > > Are you objecting to the whole point of this patch, to be able to delegate target-specific notes into targets and move the knowledge of them out of generic code in lib/BinaryFormat?
> > > > Yeah, thinking a bit more about it, I think that I'm not sure if the target-delegation is useful.  However, moving the note processing code out of readobj I think is a great idea.
> > > > 
> > > > I'm trying to understand what the benefits are of the additional machinery to dispatch to a target (particularly when the notes can be used by a target that doesn't normally get regular consideration like ELF on Windows).  But, moving the logic into a different location seems like something we should do.
> > > The note type I'm particularly interested in is NT_AMD_AMDGPU_PAL_METADATA, and its forthcoming replacement NT_AMD_AMDGPU_PAL_METADATA_MSGPACK. You will see that readobj currently dumps that by calling AMDGPU::PALMD::toString() in lib/Support/AMDGPUMetadata.cpp (in a slightly ad-hoc way that could be tidied).
> > > 
> > > What I am planning to do is encapsulate that metadata in a class, to include reading, writing, assembling, disassembling, dumping and code generating. That class could go in lib/Support, where the other AMDGPU metadata stuff is currently. But it will be a substantial chunk of code, and it is specific to the AMDGPU target, so I think it would be better in lib/Target/AMDGPU. To do that, I need the target dispatch thing just to be able to get to it from readobj. Then the code is present only if you enable the AMDGPU target at build time, and it is not left cluttering up lib/Support.
> > > 
> > > D52823 has the analogous abstraction for disassembling the note into a TargetStreamer.
> > What do you think of putting it in `lib/Object/ELF`?  The notes are ELF specific, and, although there may be more code, I think keeping it target agnostic is better.
> I don't see the argument for keeping the knowledge of these target-specific ELF notes somewhere target agnostic.
> 
> Have you looked at D52823? That adds another interface to target registry, this time for disassembling these target-specific ELF notes. That really does need to be in the target, because the disassembly code will interact with the target-specific TargetStreamer. If I go ahead and do that, but keep the ELF note dumping somewhere target agnostic, then the logic will be split between two places, which doesn't seem right.
Disassembly for a specific target will require the target of course.  But, I don't understand why the note is target specific.  The notes are completely agnostic to what target the output is meant for.  I would say that the split that you are describing with the note reading and emitting assembly for makes sense (one simply reads the target agnostic data, one emits target dependent data).  Am I still not understanding something about what you are trying to accomplish?


Repository:
  rL LLVM

https://reviews.llvm.org/D52821