<html><head><base href="x-msg://380/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi James,<div><br></div><div>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.</div><div><br></div><div>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?</div><div><br></div><div>Other than that decision, which we should resolve one way or another before applying it, you patch looks great.</div><div><br></div><div>--Owen</div><div><br><div><div>On Aug 16, 2011, at 2:18 AM, James Molloy wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div lang="EN-GB" link="blue" vlink="purple"><div class="WordSection1" style="page: WordSection1; "><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Hi,<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">As discussed on llvm-dev recently, this patch adds support for “soft-failing” on disassembly of certain ARM instructions.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">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.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">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.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">The major changelist is:<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>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”.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>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;”.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Parameterise FixedLenDecoderEmitter to take as extra (optional) arguments:<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 72pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: 'Courier New'; "><span>o<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>The return value of the functions to emit (default “bool”)<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 72pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: 'Courier New'; "><span>o<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span> “Guard” prefix and postfix to wrap any function call that may fail (default “if (!” and “) return false;” respectively)<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 72pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: 'Courier New'; "><span>o<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Value to return if everything went OK (default “true”)<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 72pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: 'Courier New'; "><span>o<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Value to return on failure (default “false”)<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 72pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: 'Courier New'; "><span>o<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Any extra local variables to add (default “”)<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>These defaults maintain the current behaviour of FixedLenDecoderEmitter. ARM/Thumb has been special cased in DisassemblerEmitter.cpp.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">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.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">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.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">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.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">I can see three options, none of which being particularly elegant:<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Make ARMDisassembler.h a public header so the user can cast<ARMDisassembler> and access an alternate getInstruction() function.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Add a bool* parameter to getInstruction() for UNPREDICTABLE; default is NULL and the other backends ignore it.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>Add an "wasInsnUnpredictable()" function to MCDisassembler, which returns false for all other backends and only the ARM backend deals with. Nastily non-reentrant.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 36pt; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; text-indent: -18pt; "><span style="font-family: Symbol; "><span>·<span style="font: normal normal normal 7pt/normal 'Times New Roman'; "> <span class="Apple-converted-space"> </span></span></span></span>??? An alternative.<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">What would you suggest?<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Cheers,<o:p></o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin-top: 0cm; margin-right: 0cm; margin-left: 0cm; margin-bottom: 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">James<o:p></o:p></div></div><span><unpredictable.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: blue; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="color: blue; text-decoration: underline; ">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></span></blockquote></div><br></div></body></html>