Page MenuHomePhabricator

[AMDGPU] Improve disassembler error handling
ClosedPublic

Authored by timcorringham on Mar 20 2018, 9:05 AM.

Details

Summary

llvm-objdump now disassembles unrecognised opcodes as data, using
the .long directive. We treat unrecognised opcodes as being 32 bit
values, so move along 4 bytes rather than the single byte which
previously resulted in a cascade of bogus disassembly following an
unrecognised opcode.

While no solution can always disassemble code that contains
embedded data correctly this provides a significant improvement.

Fixes bug in https://llvm.org/bugs/show_bug.cgi?id=36347

Diff Detail

Repository
rL LLVM

Event Timeline

timcorringham created this revision.Mar 20 2018, 9:05 AM

Adding Dmitry.

Does this resolve any bugzilla?

This implements Bug 36347

We have to look into it. Meanwhile, could you please update the patch description with bugzilla id so it will get into svn when checked in. Thanks.

timcorringham edited the summary of this revision. (Show Details)Mar 20 2018, 9:47 AM
artem.tamazov added inline comments.Mar 20 2018, 10:19 AM
tools/llvm-objdump/llvm-objdump.cpp
611 ↗(On Diff #139139)

What if there are less than 4 bytes left? Let's make sure that we are not getting into undefined behavior in such a case.

timcorringham added inline comments.Mar 21 2018, 7:55 AM
lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
250 ↗(On Diff #139139)

This should return the minimum of 4 or the number of bytes remaining if the disassembly failed.

tools/llvm-objdump/llvm-objdump.cpp
611 ↗(On Diff #139139)

Currently there is code in DisassembleObject() - line 1475 - that truncates the section to a multiple of 4 bytes, so we won't encounter the case of having fewer than 4 bytes left here.
However, maybe we should rethink that behaviour as it would be possible for data to be deliberately added to the end of a section that resulted in a non-multiple-of-4 length, and we could cope with that in the disassembly by using .byte directives.

dp added inline comments.Mar 23 2018, 5:08 AM
tools/llvm-objdump/llvm-objdump.cpp
611 ↗(On Diff #139139)

Could you add an assert?

assert(Bytes.size() % sizeof(U32) == 0);

With this assert the assumption about section length will be clear. A relevant comment would be helpful too.

dp accepted this revision.Mar 23 2018, 5:09 AM

Otherwise look fine.

This revision is now accepted and ready to land.Mar 23 2018, 5:09 AM
artem.tamazov added inline comments.Mar 23 2018, 6:46 AM
tools/llvm-objdump/llvm-objdump.cpp
611 ↗(On Diff #139139)

I am second to this. Let's make our assumptions (specifically, an assumption that DisassembleObject() truncates the section to a multiple of 4 bytes) expliicit.

Added support for disassembly of arbitrary size sections

The text section size is no longer truncated to a multiple
of 4 bytes, and the .byte directive is used to disassemble
any remaining bytes at the end of a section.

artem.tamazov accepted this revision.Mar 26 2018, 8:42 AM

Looks good.

This revision was automatically updated to reflect the committed changes.