[llvm-commits] [PATCH] Enhance ARMDisassembler to report UNPREDICTABLE insns as soft errors

[Owen Anderson]

Hi James,

My 2¢ are that I think it makes sense to expose DecodeStatus as part of MCDisassembler.  I don't know that any other targets use it offhand, but a "plausible but illegal" soft failure is at least a conceptually target-independent result.  Then we could just change the return type of getInstruction() to return a DecodeStatus.  At worst, ARM can be the only target that returns an Unpredictable result in practice.

I suspect going this way will simplify the implementation (you'll get rid of at least one of the FixedLenDecoderEmitter parameters) without complicating other targets.   Does anyone else with an interest in the MCDisassembler care?

Other than that decision, which we should resolve one way or another before applying it, you patch looks great.

--Owen

On Aug 16, 2011, at 2:18 AM, James Molloy wrote:

> Hi,
>  
> As discussed on llvm-dev recently, this patch adds support for “soft-failing” on disassembly of certain ARM instructions.
>  
> Certain instructions are marked as “UNPREDICTABLE” in the ARMARM – the disassembly of these instructions may still have value however, especially to a debugger or debugging code generated by another toolchain.
>  
> This patch promotes the return value of all Decode* operations in the ARM disassembly to a ternary value (Success, Unpredictable, Fail) and touches a lot of the error handling code to perform status demotion from Success to Unpredictable, and returning early on Fail. It also touches the FixedLenDecoderEmitter to facilitate the same changes in autogenerated code.
>  
> The major changelist is:
> ·         Add a new type “DecodeStatus”, local to ARMDisassembler.cpp, and add a macro CHECK(S, X) that will possibly demote a DecodeStatus S based on a function X’s return value, and exit early if the value becomes “Fail”.
> ·         Modify the return value of Decode* from bool to “DecodeStatus”, modify “if(!X()) return false;” to “CHECK(S, X());”, assuming a local accumulator “DecodeStatus S = Success;”.
> ·         Parameterise FixedLenDecoderEmitter to take as extra (optional) arguments:
> o   The return value of the functions to emit (default “bool”)
> o    “Guard” prefix and postfix to wrap any function call that may fail (default “if (!” and “) return false;” respectively)
> o   Value to return if everything went OK (default “true”)
> o   Value to return on failure (default “false”)
> o   Any extra local variables to add (default “”)
> ·         These defaults maintain the current behaviour of FixedLenDecoderEmitter. ARM/Thumb has been special cased in DisassemblerEmitter.cpp.
>  
> The patch as is does not change the user-seen behaviour of the disassembler at all. I’m still not sure the best way to expose this information to the user.
>  
> AFAIK, Intel or most other architectures don't have the same concept as ARM of a valid but unpredictable instruction. I'm therefore fearful of changing MCDisassembler.h to cope with it. At the moment I have an alternate getInstruction() function that also takes a bool& to which it writes if the insn was predictable or not.
>  
> The problem with this obviously is that it requires the user to #include <ARMDisassembler.h>, which is impossible in the general case as it is a hidden header.
>  
> I can see three options, none of which being particularly elegant:
>  
> ·         Make ARMDisassembler.h a public header so the user can cast<ARMDisassembler> and access an alternate getInstruction() function.
> ·         Add a bool* parameter to getInstruction() for UNPREDICTABLE; default is NULL and the other backends ignore it.
> ·         Add an "wasInsnUnpredictable()" function to MCDisassembler, which returns false for all other backends and only the ARM backend deals with. Nastily non-reentrant.
> ·         ??? An alternative.
>  
> What would you suggest?
>  
> Cheers,
>  
> James
> <unpredictable.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110816/c5e832ad/attachment.html>