[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:
> > 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D52821