<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:x="urn:schemas-microsoft-com:office:excel" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-GB" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Hi Reid,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I wrote this as part of the normal assembly parsing because I think it would be<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">very intrusive to modify the assembly parser to support parsing a raw inline<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">assembly string.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">At least for ARM, many of the parsed operands are more complex than just a<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">register or immediate. For example, a memory operand has a base register, but<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">might also have an offset immediate or register, a shift, and/or and alignment.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">We would also have to modify the expression parser to allow operand references<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">inside expressions, without needing to know if they are immediates or symbol<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">names.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I think this is a lot easier for MS-style inline assembly, because it only<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">works with a few simple operand types, whereas this checker needs to understand<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">all of the more complex operands (well, excluding the immediate-only ones).<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">That said, I'm not too familiar with how the MS-style inline assembly support<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">works, so I'll investigate that a bit more to see if there's an easier way to<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">do this.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">My approach also has the advantage that it needs only very small changes in the<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">assembly parser, which is different for each architecture, so moving more of<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">the change into the assembly parser means more code needs to be re-written for<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">each backend we want this to work with. I think the target-specific code in my<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">prototype could be reduced to almost nothing if the assembly parser set<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">accurate start _and_ end locations for every operand, but unfortunately that's<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">not currently the case, at least for ARM.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Oliver<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Reid Kleckner [mailto:rnk@google.com]
<br>
<b>Sent:</b> 26 November 2018 20:36<br>
<b>To:</b> Oliver Stannard<br>
<b>Cc:</b> llvm-dev; nd; Renato Golin; Tim Northover; Kristof Beyls; Jim Grosbach; Eric Christopher<br>
<b>Subject:</b> Re: [RFC] Checking inline assembly for validity<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">I was following along until I got to the "way this works" part. :)<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I agree with the premise 100%, writing correct inline assembly is a huge pain. It would be nice to have some middle ground between having the user carry the burden of writing correct constraints as in GCC inline asm, and having the compiler
guess the constraints as in MSVC inline asm.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The way I imagined this would work, before digging in too much, is that we'd parse the GCC inline asm in the frontend, as is done with MS inline asm, so you can get the warnings during semantic analysis. Generally, GNU-as syntax and the
GCC asm constraints should make this much, much easier than it is for Intel syntax. The parser would parse "%N" as some new kind of MCOperand. We'd write generic MC checks that walk the MC instruction stream and leverage InstrInfo to produce warnings.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><br>
That's pretty far from what you had in mind. Does that seem crazy? I think it would help clean up a lot of the ad-hoc inline asm constraint warning logic we have in clang, which has tons of target-specific knowledge. It also seems like a good first step towards
a less string-ly typed inline asm instruction in LLVM IR, which is perhaps a bit out of scope.<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Nov 26, 2018 at 2:54 AM Oliver Stannard <<a href="mailto:Oliver.Stannard@arm.com">Oliver.Stannard@arm.com</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm">
<p class="MsoNormal">GCC-style inline assembly is notoriously hard to write correctly, because it is<br>
the user's responsibility to tell the compiler about the requirements of the<br>
assembly (inputs, output, modified registers, memory access), and getting this<br>
wrong results in silently generating incorrect code. This is also dependent on<br>
register allocation and scheduling decisions made by the compiler, so an inline<br>
assembly statement may appear to work correctly, then silently break when<br>
another change to the code or compiler upgrade causes those decisions to<br>
change.<br>
<br>
I've posted a prototype patch at <a href="https://reviews.llvm.org/D54891" target="_blank">
https://reviews.llvm.org/D54891</a> which tries to<br>
improve this situation by emitting diagnostics when the instructions inside the<br>
inline assembly string do not match the operands to the inline assembly<br>
statement. We can do this because we parse the assembly in the same process as<br>
the compiler, and the MC layer has some knowledge of which registers are read<br>
and written by an assembly instruction.<br>
<br>
For example, this C code, which tries to add 3 integers together, looks OK at<br>
first glance:<br>
<br>
int add3(int a, int b, int c) {<br>
int x;<br>
asm volatile(<br>
"add %0, %1, %2; add %0, %0, %3"<br>
: "=r" (x)<br>
: "r" (a), "r" (b), "r" (c));<br>
return x;<br>
}<br>
<br>
However, the compiler is allowed to allocate x (the output operand) and c (an<br>
input operand) to the same register, as it assumes that c will be read before x<br>
is written. This code might happen to work correctly when first written, but a<br>
later change (either in the source code or compiler) could cause register<br>
allocation to change, and this will start silently generating the "wrong" code.<br>
<br>
With this patch, the compiler emits this warning for the above code:<br>
<br>
test.cpp:4:7: warning: read from an inline assembly operand after first<br>
output operand written, suggest adding early-clobber<br>
modifier to output operand [-Winline-asm]<br>
"add %0, %1, %2; add %0, %0, %3"<br>
^<br>
<inline asm>:1:30: note: instantiated into assembly here<br>
add r0, r0, r1; add r0, r0, r2<br>
^<br>
test.cpp:4:7: note: output operand written here<br>
"add %0, %1, %2; add %0, %0, %3"<br>
^<br>
<inline asm>:1:6: note: instantiated into assembly here<br>
add r0, r0, r1; add r0, r0, r2<br>
^<br>
<br>
The warning is a bit noisy because it prints both the original source code and<br>
final assembly code, and the carets for the original source just point to the<br>
start of the line, but that's a separate issue.<br>
<br>
I've designed this to be independent of register allocation decisions, so the<br>
above diagnostic is always reported, regardless of whether x and c were<br>
allocated in the same register or not. This allows it to catch code which<br>
currently works but might break in future.<br>
<br>
The way this works is:<br>
- When the AsmParser reaches an INLINEASM MachineInstr, it creates an<br>
InlineAsmDataflowChecker object, which tracks all of the information we need<br>
to know about one inline assembly statement.<br>
- The AsmPrinter examines the operands to the MachineInstruction, and calls<br>
functions on the tracking object to record information about each operand to<br>
the inline assembly block, as provided by the user and relied on by the<br>
compiler's optimisations and code-generation.<br>
- While the AsmParser is generating the final assembly string (which involves<br>
expanding operand templates like "$0" into physical register names), it<br>
records the offset from the start of the (output) string at which each<br>
operand expansion appeared.<br>
- The table-generated assembly matcher is modified to record the index of the<br>
MCParsedAsmOperand which resulted in in the creation of each MCOperand. An<br>
MCParsedAsmOperand can create multiple MCOperands (for example, a memory<br>
operand with base and offset), but not the other way round, so this<br>
information is stored in the MCOperand.<br>
- When the AsmParser is running and a tracking object is present (it is only<br>
present if we are parsing inline assembly), it records in each ParsedOperand<br>
the indexes of the inline assembly statement operands which overlap with it.<br>
This is done using the source location of the just-parsed MCParsedAsmOperand<br>
and the string offsets recored in the tracking object by the AsmPrinter.<br>
- Finally, after an instruction has been completely parsed, the AsmParser calls<br>
into the tracking object with the final MCInst and list of<br>
MCParsedAsmOperands. With all of this information, we can match up inline<br>
assembly operands to the MCOperands that were created for them, and check<br>
that they match.<br>
<br>
The reason that I'm posting this as an RFC is that it adds a lot of coupling<br>
between different parts of the backend. Do people think this is an acceptable<br>
cost for making a quite user-hostile feature a bit safer? If people agree that<br>
this is worthwhile, there are some things I still need to do before the patch<br>
is ready for a proper review:<br>
<br>
- Check the memory overhead (during regular compilation) of storing the parsed<br>
operand number in the MCOperand. If this is too high, it could be moved to a<br>
separate data structure only created when parsing inline assmebly.<br>
- There are three different concepts of "operand" here (inline assembly<br>
operand, MCParsedAsmOperand and MCOperand), tidy up the naming so that these<br>
are a bit clearer.<br>
- For checks which depend on the order of instructions (for example, checking<br>
that a clobbered register is only read from if it has previously been written<br>
to), these could give false positives if there is control-flow in the<br>
assembly. We could check if an MCInst affects control flow, buffer these<br>
diagnostics until the end of the assembly block, and only emit them if there<br>
are no control-flow instructions.<o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</body>
</html>