[PATCH] D130357: [MC,llvm-objdump,ARM] Target-dependent disassembly resync policy.

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added inline comments.


================
Comment at: llvm/lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:353
 
+uint64_t AArch64Disassembler::suggestBytesToSkip(ArrayRef<uint8_t> Bytes,
+                                                 uint64_t Address) const {
----------------
simon_tatham wrote:
> DavidSpickett wrote:
> > We're assuming that Address is already aligned, I assume that's safe given that we're either:
> > * disassembling from the start of a fn, which would be aligned
> > * have just disassembled an instruction, which would have been aligned, and 4 bytes in size.
> > 
> > Correct?
> > 
> > I guess someone could give MC a misaligned function start but garbage in garbage out in that case? Or would you want to align up to the nearest 4 bytes.
> Mmmm. I did wonder about specifying `suggestBytesToSkip` so that you could give it a totally arbitrary alignment and it would do something sensible. But I didn't like the idea that every single implementation (other than the default 'skip 1 byte because anything's a valid start position') would end up having to contain very similar boilerplate. That struck me as a sign of having put the API boundary in the wrong place.
> 
> In this patch there are only two implementations of `suggestBytesToSkip` (or two and a half if you count the Arm/Thumb branches of the AArch32 one). But I expect that most non-x86 targets will end up wanting to do something sensible in here – surely a lot of targets are fixed-alignment RISC style, and even m68k has an alignment constraint if I remember my Amiga-owning days correctly. So there'd end up being a lot of copies of that code!
Thinking about this a bit more ... another problem with trying to use `suggestBytesToSkip` to handle //existing// alignment problems is that it's called in the wrong place in the disassembly loop. Currently, the expected usage is that you have some starting address, and you call `getInstruction` to see if you can disassemble an instruction starting there. If you can, you advance by its width; otherwise, you call this new `suggestBytesToSkip` function and advance by that many instead.

But if the user tries to //start// disassembly at an address that's invalid due to misalignment, then `suggestBytesToSkip` can't rescue them anyway, because by the time it's first called, it's too late to prevent an initial nonsense instruction from having been decoded at the misaligned starting location. So then you'd get a resynchronization between that first instruction and the next one, which seems even more nonsensical to me!

So I think it is right that `suggestBytesToSkip` restricts itself to not //creating new// alignment problems, and doesn't check whether there's an existing one. The latter (if we think it needs doing at all) would be the job of some other API function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130357/new/

https://reviews.llvm.org/D130357