[llvm-commits] [PATCH] Enhance ARMDisassembler to report UNPREDICTABLE insns as soft errors
Owen Anderson
resistor at mac.com
Wed Aug 17 10:45:39 PDT 2011
Thanks James!
Applied in r137830.
--Owen
On Aug 17, 2011, at 4:54 AM, James Molloy wrote:
> Hi,
>
> New patch attached. Changelog from previous:
>
> * DecodeStatus promoted to MCDisassembler.h. "Unpredictable" renamed to "SoftFail" to be more target-agnostic.
> * FixedLenDecoderEmitter now is not parameterised by return type, although the guards and OK/Fail parameters remain.
> * llvm-mc adapted to now print a warning above the disassembly on soft fail. Example:
>
> $ /work/llvm-cmake2/build/bin/./llvm-mc --disassemble /work/llvm-cmake2/test/MC/Disassembler/ARM/invalid-LDRB_POST-arm.txt -triple=arm-apple-darwin9 [12:06]
> /work/llvm-cmake2/test/MC/Disassembler/ARM/invalid-LDRB_POST-arm.txt:10:1: warning: potentially undefined instruction encoding
> 0x05 0x70 0xd7 0xe6
> ^
> ldrb r7, [r7], r5
>
> * invalid-LDRB_POST-arm.txt -> unpredictable-LDRB_POST-arm.txt and updated to look for the soft fail warning.
>
> Hopefully this should be ready for committing. Should apply cleanly on ToT at now: r137824.
>
> Cheers,
>
> James
>
>> -----Original Message-----
>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
>> bounces at cs.uiuc.edu] On Behalf Of James Molloy
>> Sent: 16 August 2011 19:56
>> To: Owen Anderson
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Enhance ARMDisassembler to report
>> UNPREDICTABLE insns as soft errors
>>
>> Hi Owen,
>>
>> Thanks. That does seem the most logical thing to do - I suppose I'm
>> just used to open source projects where changing an abstraction can
>> lead to violent uproar on mailing lists :)
>>
>> I'll wait for anyone to pipe up about the MCDisassembler change then
>> adapt my patch.
>>
>> Thanks,
>>
>> James
>> ________________________________________
>> From: Owen Anderson [resistor at mac.com]
>> Sent: 16 August 2011 19:44
>> To: James Molloy
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Enhance ARMDisassembler to report
>> UNPREDICTABLE insns as soft errors
>>
>> 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<mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> -- IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose the
>> contents to any other person, use it for any purpose, or store or copy
>> the information in any medium. Thank you.
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.<unpredictable.v2.patch>
More information about the llvm-commits
mailing list