This is an archive of the discontinued LLVM Phabricator instance.

[MC] Changes to help improve target specific symbol disassembly
ClosedPublic

Authored by rochauha on May 25 2020, 2:43 AM.

Details

Summary

This commit slightly modifies the MCDisassembler, and llvm-objdump to allow targets to also decode entire symbols.

WebAssembly uses the onSymbolStart hook it to decode preludes. WebAssembly partially
disassembles the symbol in its target specific way; and then falls back to the normal
flow of llvm-objdump.

AMDGPU needs it to decode kernel descriptors entirely, and move to the next symbol.

This commit is to split the above task into 2.

  • Changes to llvm-objdump and MC-layer without breaking WebAssembly code [ this commit ]
  • AMDGPU's implementation of onSymbolStart that decodes kernel descriptors. [ https://reviews.llvm.org/D80713 ]

PS:
The last 6 bytes of AMDGPU kernel descriptor are reserved and hence 0.
Right now 4 bytes of it are trimmed in llvm-objdump, which makes the kernel descriptor invalid.
I have commented out those AMDGPU specific parts in this commit.
The nop-data.ll test failure in AMDGPU tests is because of this.

Diff Detail

Event Timeline

rochauha created this revision.May 25 2020, 2:43 AM
rochauha edited the summary of this revision. (Show Details)May 25 2020, 2:53 AM

This commit slightly modifies the MCDisassembler, and llvm-objdump to allow targets to also decode entire symbols.

What does "decode entire symbols" mean?

llvm/tools/llvm-objdump/llvm-objdump.cpp
1414

This breaks test/CodeGen/AMDGPU/nop-data.ll

What does "decode entire symbols" mean?

It means to disassemble all bytes of a particular symbol.

(Not got time to look at the patch yet, but one comment I wanted to make)

What does "decode entire symbols" mean?

It means to disassemble all bytes of a particular symbol.

Are you aware of the --disassemble-symbols switch which disassembles the bytes for a specified symbol already?

Are you aware of the --disassemble-symbols switch which disassembles the bytes for a specified symbol already?

Yes, but that disassembles the bytes of a specified symbol as instructions.
This commit allows the target to handle certain symbols as part of --disassemble-all; in a way that the target requires.

For example - AMDGPU needs to disassemble kernel descriptors as assembler directives rather than instructions.

bcain added a subscriber: bcain.May 26 2020, 6:27 AM
rochauha updated this revision to Diff 266304.EditedMay 26 2020, 12:49 PM

Changes to MCDisassembler and llvm-objdump to allow targets to also decode entire symbols.

Not all symbols need to be disassembled as instructions.
Targets can handle certain symbols as part of --disassemble-all; in a way that they require.

WebAssembly uses the onSymbolStart hook to handle preludes.
This commit tries to generalize onSymbolStart so that other targets can also use it (if they want to)

Now the entire symbol can be disassembled in a target specific way. We then move to the next
symbol or fall back (only if the symbol is not disassembled entirely) to decoding remaining bytes as instructions.

AMDGPU needs it to decode kernel descriptors as assembler directives rather than instructions.

This commit is to split the above task into 2.

  • Changes to llvm-objdump and MC-layer without breaking WebAssembly code [ this commit ]
  • AMDGPU's implementation of onSymbolStart that decodes kernel descriptors. [ https://reviews.llvm.org/D80713 ]

Edit:

  • Uncommented lines in llvm-objdump that broke test/CodeGen/AMDGPU/nop-data.ll (as pointed out by MaskRay)
  • Small change to the new helper functions to read bytes
  • Changes to this summary
jhenderson requested changes to this revision.May 27 2020, 1:25 AM

Am I right in thinking that this change is now NFC? If so, please add that to the title.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
78–103

Seems like this whole comment needs updating if you are adding a new enum value.

130–137

Looks like this comment might need reflowing - "particular" at least looks like it belongs on the previous line.

132

';' -> ','

133–134

I don't think this last sentence is a good comment to have, as it implies that WebAssembly and AMDGPU are the only two cases that make use of this, which might be true now, but probably won't be true in the future. I think it's simplest to say "This is used for example by AMDGPU to decode kernel descriptors."

152

Nit: missing trailing full stop.

As it's a TODO, perhaps this whole comment should not use doxygen style comment markers?

175

Nit: missing trailing full stop (feel free to add to the comment below too).

176–183

Variable names should all be UpperCamelCase (index -> Index, doubleWord -> DoubleWord etc). Also applies to code body.

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
26

There is already a class DataExtractor in Support for reading a series of bytes of various sizes. I think it would make more sense to use that than reinvent the wheel with these methods. It also has error handling, which can be used to detect failures to read and handles endianness.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1401

If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.

1411–1412

This also appears to be unrelated to this patch.

This revision now requires changes to proceed.May 27 2020, 1:25 AM
rochauha updated this revision to Diff 266517.May 27 2020, 6:29 AM
rochauha marked 9 inline comments as done.

Changes as per review by jhenderson.

aardappel accepted this revision.May 28 2020, 11:39 AM
aardappel added inline comments.
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
133

Maybe retain the original comment that it is also used for WebAssembly?

scott.linder added inline comments.May 28 2020, 2:08 PM
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
78–103

It seems like beyond the need to just update the comment all the uses of DeocdeStatus need to be audited to possibly consider the new variant. The big one is the CHECK macro which relies on the bitwise-AND reduction property of the current encoding, which Ignore seems to break (and maybe it just doesn't really fit in to begin with; what should Ignore & Success mean? It currently means Fail, which doesn't seem particularly helpful).

It seems like what we want instead is to have two orthogonal statuses: a boolean one to indicate whether any decoding is possible and was attempted, and then the ternary status for the decode in the case that it happens. Maybe we just want to leave DecodeStatus alone and have onSymbolStart return an Optional<onSymbolStart> where None indicates no decoding was attempted?

88

This wording seems ambiguous; when I first read it I took it to mean that Ignore and SoftFail are equivalent. As the above describes SoftFail already can this just describe Ignore here? I also don't know what you mean when you say SoftFail can be used for partial disassembly; I thought from the description it just meant the decoded instruction is architecturally invalid, but is a decodeable form?

The comment also seems to use Unpredictable and SoftFail interchangeably, can this all just be changed to refer to SoftFail?

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
131

I'm a little confused here on what the intended semantics for this impl are, but it doesn't seem like the intention would be to Ignore here, right? Since llvm-objdump.cpp ignores the return value completely it isn't clear what was really intended (i.e. this could have returned anything, yet chose to return different values for different cases).

Maybe @aardappel can describe the intent a bit better?

156

This seems to be overloading what SoftFail means. It seems like the original intent of SoftFail was to capture a Success which is "poisoned" by architectural meaningless-ness. Thus Success & SoftFail == SoftFail but SoftFail & Fail == Fail.

It seems like Success is the right thing to return here still. Can't the callee know whether or not the "whole" symbol was consumed by just considering the Size value that is returned along with the status?

llvm/tools/llvm-objdump/llvm-objdump.cpp
1436

Going along with the comments I left above, I don't think we need to subvert Success and SoftFail here to support "partial" decoding. I would vote to just use Size to detect when the "entire" symbol was decoded.

1439

I would prefer this just fall out of the code below, rather than need to be explicit here. Things like onSymbolEnd should still be considered even when the whole symbol is decoded, right?

jhenderson added inline comments.May 29 2020, 1:45 AM
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
16

Unrelated formatting change. Please revert.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1401

Actually, why was this line modified at all?

1411–1412

If it wasn't clear what I meant before, since this change is unrelated to this patch (it's just a pure formatting change and you aren't touching this bit of code), I think it needs reverting.

rochauha updated this revision to Diff 267186.May 29 2020, 5:07 AM
rochauha marked 3 inline comments as done.

Changes as per review by @aardappel and @jhenderson

jhenderson added inline comments.May 29 2020, 6:26 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1401

My question hasn't been answered. Please revert the changes to this line, or explain why you wish to modify it.

rochauha marked 2 inline comments as done.EditedMay 29 2020, 6:41 AM

@scott.linder, the DecodeStatus enum seems to mainly be concerned with instructions.
But I felt thought that we could probably use it with symbols too. Just that a symbol SoftFail would not mean the same thing as an instruction SoftFail.

SoftFail and Ignore are different. When target returns SoftFail or Success, the Size variable must be updated by the target. When returning Ignore, value of Size must be 0.

The idea of using Optional<> and Size value with the status sounds good. Comments from others regarding this would also be helpful.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
78–103

I believe Ignore should not be used with the other 3 decode statuses.

rochauha marked an inline comment as done.EditedMay 29 2020, 6:48 AM

@jhenderson, following was your previous comment on line 1399
> If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.

So I modified the line when making revisions to the commit.

@jhenderson, following was your previous comment on line 1399
> If you're going to modify this line, please also add the missing full stop and "skip" -> "Skip". Seems like it's unrelated to this patch.

So I modified the line when making revisions to the commit.

Right, but the modification prior to my comment there was just to remove a space in the comment. I said "if you are going to modify this line", but on closer look, I'm not sure why you wanted to modify the line in the first place, hence my question.

@jhenderson apologies for the misunderstanding from my side. I'll undo that change.
That entire chunk of code is commented out in the child revision anyways.

I'd like to understand the blockers that need to be worked on for this review to be accepted.

jhenderson accepted this revision.Jun 1 2020, 12:45 AM

No more additional comments from me, but I don't know enough about this code to give any further feedback on the actual behaviour changes. @scott.linder should take the lead from here. I'm approving now, but only to clear my blockage. It is NOT an approval of the patch in general.

This revision is now accepted and ready to land.Jun 1 2020, 12:45 AM
MaskRay added inline comments.Jun 1 2020, 11:17 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1442

The comments should probably be moved before if.

You can say that most targets return Ignore, and state how AMDGPU and WebAssembly are different and why they do continue.

rochauha updated this revision to Diff 267839.Jun 2 2020, 4:16 AM
rochauha marked 5 inline comments as done.

Changes as per reviews by @scott.linder, @jhenderson and @MaskRay

rochauha updated this revision to Diff 268248.Jun 3 2020, 10:18 AM

To have round trippable disassembly, we fall back to decoding the
remaining bytes as instructions.

If there is a failure, we disassemble the 'failed' region as bytes
before falling back.

  • This means that on failure the target must print whatever it disassembled so far as comments? Requires discussion.
  • Otherwise we would have decoded some bytes twice. Once by the target (until it failed) and then again as bytes.

If there is Success or SoftFail i.e no 'real' failure, we skip over
Size bytes before falling back.
So if the entire symbol is 'eaten' by the target:

Start += Size  // Now Start = End and we will never decode as instructions

Right now, most targets return None i.e ignore to treat a symbol
separately. But WebAssembly decodes preludes for some symbols.

rochauha updated this revision to Diff 268255.Jun 3 2020, 10:30 AM

Run git-clang-format.

scott.linder accepted this revision.Jun 11 2020, 1:36 PM

LGTM, sorry for the delay in reviewing. My feeling is that we should just keep the simple rules you have outlined in the docs and require the callback implementation ensure things are not decoded twice by buffering their output (if needed) and setting Size correctly.

llvm/lib/MC/MCDisassembler/MCDisassembler.cpp
22–23

Can this be removed? The contract is this value can't be read anyway.

23

I think the comment here is overkill, the comment for the function itself already describes the meaning.

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
132

Same as above, I think the comment on each return is overkill and likely to just become out of date at some point in the worst case.

146

In the original implementation, I'm lost on what was intended here. The ".local" has already made it to the output stream by this point. I don't know how to review the change without knowing what the intention was (if this is actually a bug; maybe I just don't understand at all). The behavior is not changing, though, so I don't think we need to wait on understanding the intention to land this change.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1444

I think we either need to require the callback buffer output so it can avoid detecting an error in the middle of output, or we need to factor out that buffering by passing in a buffered stream and only appending it to the disassembly output in the Success case.

rochauha updated this revision to Diff 270324.Jun 12 2020, 1:45 AM
rochauha marked 6 inline comments as done.

Minor changes as per review by @scott.linder.

This revision was automatically updated to reflect the committed changes.