Page MenuHomePhabricator

[Disassembler][llvm-readobj] ELF note dumper abstraction
Needs RevisionPublic

Authored by tpr on Oct 3 2018, 1:27 AM.

Details

Summary

This commit introduces an abstraction to allow knowledge of how to dump
an ELF note record into the target that it is specific to.

llvm-readobj's ELF dumper uses the new abstraction, although no target
yet implements it. A subsequent change will move dumping of the
AMDGPU-specific note records out of llvm-readobj and into
lib/Target/AMDGPU.

Change-Id: Ib42c861bfda0ad38f0ceb2f61658215326c8fcaf

Diff Detail

Event Timeline

tpr created this revision.Oct 3 2018, 1:27 AM

See D52822 for AMDGPU implementing ELFNoteDumper.

See also D52823 for MCELFNoteDisassembler, another new abstraction in TargetRegistry.

tpr added a reviewer: kzhuravl.Oct 3 2018, 1:57 AM
tpr added subscribers: dstuttard, timcorringham.
compnerd requested changes to this revision.Oct 4 2018, 10:24 AM
compnerd added inline comments.
include/llvm/Object/ELFNoteDumper.h
28

Why not use = default?

32

We can use the destructor as the key function rather than emitting the data globally.

tools/llvm-readobj/ELFDumper.cpp
3811

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 revision now requires changes to proceed.Oct 4 2018, 10:24 AM
tpr added inline comments.Oct 5 2018, 1:08 AM
tools/llvm-readobj/ELFDumper.cpp
3811

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?

compnerd added inline comments.Oct 5 2018, 9:58 AM
tools/llvm-readobj/ELFDumper.cpp
3811

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.

tpr added inline comments.Oct 5 2018, 12:06 PM
tools/llvm-readobj/ELFDumper.cpp
3811

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.

compnerd added inline comments.Oct 9 2018, 9:37 AM
tools/llvm-readobj/ELFDumper.cpp
3811

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.

tpr added inline comments.Oct 10 2018, 10:51 PM
tools/llvm-readobj/ELFDumper.cpp
3811

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.

compnerd added inline comments.Oct 14 2018, 10:33 AM
tools/llvm-readobj/ELFDumper.cpp
3811

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?

tpr added a comment.Oct 17 2018, 10:28 PM

This particular family of notes is target specific because it contains information on how the driver should program the hardware in order to be able to run the shaders in this graphics pipeline. So I think that justifies treating the notes as target specific.

tpr added a comment.Oct 31 2018, 6:51 AM

Ping.

It would be good to have comments from other reviewers re the whole idea of having a TargetRegistry interface to allow readobj-style dumping of a target-specific note type to be inside the target.