[PATCH] D80512: [MC] Changes to help improve target specific symbol disassembly

Ronak Chauhan via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 29 07:01:54 PDT 2020


rochauha marked 2 inline comments as done.
rochauha added a comment.

@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.



================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:76-97
   /// Ternary decode status. Most backends will just use Fail and
   /// Success, however some have a concept of an instruction with
   /// understandable semantics but which is architecturally
   /// incorrect. An example of this is ARM UNPREDICTABLE instructions
   /// which are disassemblable but cause undefined behaviour.
   ///
   /// Because it makes sense to disassemble these instructions, there
----------------
scott.linder wrote:
> jhenderson wrote:
> > Seems like this whole comment needs updating if you are adding a new enum value.
> 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?
I believe `Ignore` should not be used with the other 3 decode statuses.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80512





More information about the llvm-commits mailing list