<div dir="ltr">Here's an updated patch.<div><br></div><div>I don't have commit rights: Eli, could you commit for me? :-)</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jan 29, 2013 at 5:46 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi JF,<div><br></div><div>This looks fine to me, with a could of very minor nits:</div>
<div><br></div><div><div>+ BuildMI(TrapBB, dl, TII->get(</div><div>+ Subtarget->isThumb() ? ARM::tTRAP :</div><div>+ (Subtarget->emitNaClTrap() ? ARM::TRAPNaCl : ARM::TRAP)));</div><div><br></div><div>When 80-column mucks with things this much, I prefer to put the conditional testing on a local variable and then just reference that in the get() call.</div>
<div>unsigned NewOpc = Subtarget->isThumb() ….</div><div>… TII->get(NewOpc) …</div><div><br></div><div><div>+def EmitNaClTrap : Predicate<"Subtarget->emitNaClTrap()">,</div><div>+ AssemblerPredicate<"FeatureNaClTrap", "NaCl">;</div>
<div>+def DontEmitNaClTrap : Predicate<"!Subtarget->emitNaClTrap()">;</div></div><div><br></div><div>For things like this, the predicates are typically named with "Use" and "DontUse" rather than "Emit".</div>
<div><br></div><div>Good to commit with those changes.</div><div><br></div><div>-Jim</div><div><br></div><div><div><div class="h5"><div>On Jan 23, 2013, at 1:35 PM, JF Bastien <<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>> wrote:</div>
<br></div></div><blockquote type="cite"><div><div class="h5"><div dir="ltr">Hi Jim,<div><br></div><div>I updated this and I now don't think it's so ugly anymore, thanks to some guidance from Eli. The one dodgy thing is in ARM_MC::ParseARMTriple: I think it should be using Triple, but that's better done in a separate change.</div>
<div><br></div><div>Please take a look.</div><div><br></div><div>Thanks,</div><div><br></div><div>JF</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jan 22, 2013 at 4:28 PM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Jim,<div><br></div><div>Here's an updated patch.</div><div><br></div><div>I'm not sure it's quite right yet and would appreciate any suggestions you may have, especially when it comes to reconciling ISel and MC: they seem to get information slightly differently, the former from triple and the latter from CPU features (mostly mattr). I may be mistaken, but I believe that I need a SubTargetFeature for MC. The oddness I refer to comes out in the test files that I changed.</div>
<div><br></div><div>One suggestion from Derek was to create a separate trap_nacl mnemonic at the assembly level. The backend up to ISel would still do lowering decisions for @llvm.trap based on the presence of the NaCl triple, but MC would be oblivious to this since the mnemonic stands alone. Thoughts on this?</div>
<div><br></div><div>Thanks,</div><div><br></div><div>JF</div></div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jan 21, 2013 at 2:42 PM, JF Bastien <span dir="ltr"><<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I see, that's what I was trying to avoid without realizing that it was the only way to go :-)<div><br>
</div><div>I'll send an updated patch tomorrow, at least so I can pretend to take MLK off.</div>
</div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jan 21, 2013 at 10:43 AM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi JF,<div><br></div><div>That's the ugly part. You need two totally separate definitions, including a different instruction name.</div>
<div><br></div><div>// The new definition.</div><div>def TRAPnacl: …. Requires<[IsARM, IsPNaCL]>;</div><div><div>// The old definition. Updated "Requires" clause.</div><div>def TRAP: …. Requires<[IsARM, IsNotPNaCL]>;</div>
<div><br></div><div>Any .cpp code which references ARM::TRAP will also need to be updated.</div><span><font color="#888888"><div><br></div><div>-Jim</div></font></span><div><div><br></div><div>
<div>On Jan 18, 2013, at 11:27 AM, JF Bastien <<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_default">I've tried a few different things and tablegen eludes me. How do I do something like this:</div>
<div class="gmail_default"><div class="gmail_default"> let isBarrier = 1, isTerminator = 1 in</div>
<div class="gmail_default"> def TRAP : AXI<(outs), (ins), MiscFrm, NoItinerary,</div><div class="gmail_default"> "trap", [(trap)]>,</div><div class="gmail_default"> Requires<[IsARM]> {</div>
<div class="gmail_default"> bits<16> imm16 = [{</div><div class="gmail_default"> if (IsNaCl)</div><div class="gmail_default"> return 0b1110110111100000;</div><div class="gmail_default"> return 0b1111110111101110;</div>
<div class="gmail_default"> }];</div><div class="gmail_default"> let Inst{31-20} = 0b111001111111;</div><div class="gmail_default"> let Inst{19-8} = imm16{15-4};</div><div class="gmail_default"> let Inst{7-4} = 0b1111;</div>
<div class="gmail_default"> let Inst{3-0} = imm16{3-0};</div><div class="gmail_default"> }</div><div>?</div><div><br></div><div>Multiclasses+Requires doesn't quite work because it would create different names for the NaCl/non-NaCl versions.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 16, 2013 at 5:25 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On Jan 16, 2013, at 4:58 PM, JF Bastien <<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr"><div>The sneaky part is indeed our original intent: we want to mitigate mode-switching attacks so we sketched out a mitigation that's kind of like constant blinding, it makes the final code somewhat unpredictable to the attacker. I do think that it's possible to bias instruction selection without incurring an undue performance hit while still messing with known-code creation.</div>
</div></blockquote><div><br></div></div><div>With clever help from register allocation, that's quite possible. The low bits are often enough mostly register and immediate values that they can be twiddled creatively. I'm curious what you come up with there. That could be quite interesting!</div>
<div><br><blockquote type="cite"><div dir="ltr"><div> The one issue we had was that we needed to freeze our ARM ABI, and that implied freezing all the trap/halt/debug instructions that the NaCl "OS" recognizes, hence this change.</div>
<div><br></div><div>The mitigation is something we'll implement later, once PNaCl reaches its performance goals. We've already branched NaCl ARM with a frozen ABI for Chrome M25, and although the current backend is GCC we'll keep the same ABI once we migrate to PNaCl.</div>
<div><br></div><div>In the meantime we'd like to reduce PNaCl's localmods, so while this change on its own is quite silly, we think it's critical as part of a whole system.</div>
<div><br></div><div>Given this, would it make sense to commit this change, once I specialize it for the NaCl ARM target?</div></div><div class="gmail_extra"><br></div></blockquote><div><br></div></div><div>Yeah, that's fine. I'll probably grumble a bit about the added complexity, but it's the right thing to do. :)</div>
<span><font color="#888888"><div><br></div><div>-Jim</div></font></span><div><br><blockquote type="cite"><div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Jan 16, 2013 at 4:33 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word">Hi JF,<div><br></div><div>Assuming there's real security benefits to be had, yes. However, I'm reticent to add complexity to the code on a purely theoretical benefit. Can you elaborate a bit more on why this is worth it?<div>
<br></div><div>In particular, I'm skeptical of benefits to an overlapping ARM/Thumb TRAP instruction. Now, I can definitely see benefit if you could find a way to get ARM ISel to more frequently have Thumb2 undefined bitpatterns in the bitstream (as the low-order bits of normal ARM instructions, that is). That would be quite clever and downright sneaky. Also likely pretty hard to do it w/o completely crushing performance…</div>
<span><font color="#888888"><div><br></div><div>-Jim</div></font></span><div><br><div><div>On Jan 16, 2013, at 4:27 PM, JF Bastien <<a href="mailto:jfb@google.com" target="_blank">jfb@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">As discussed over IRC: it then makes sense to only change the encoding for the NaCl triple (which is effectively its own OS).</div><div class="gmail_extra">
<br><br><div class="gmail_quote">
On Wed, Jan 16, 2013 at 4:17 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div><div>On Jan 16, 2013, at 3:45 PM, Renato Golin Linaro <<a href="mailto:renato.golin@linaro.org" target="_blank">renato.golin@linaro.org</a>> wrote:</div><br>
<blockquote type="cite"><div dir="ltr">On 16 January 2013 22:39, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word">The entire encoding is semantically significant on Darwin. I suspect that's true on other platforms, too, but I don't know for sure.</div></blockquote><div><br></div><div>I'm not sure either. I agree with Bastien that it *should* trap on both ARM and Thumb, but it also depends on what catch routine is installed and other hard-to-know problems.</div>
<div><br></div></div></div></div></blockquote><div><br></div></div><div>Both instructions will trap; however, how they trap is also important. In this case, it's the difference between the user program terminating with SIGILL vs. SIGTRAP.</div>
<span><font color="#888888"><div><br></div><div>-Jim</div></font></span><div><div><br></div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Bastien, Have you tested in which platforms?<br>
</div><div><br></div><div>cheers,</div><div>--renato</div><div><br></div><div><br></div></div></div></div>
</blockquote></div></div><br></div>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div></blockquote></div><br></div>
</blockquote></div></div><br></div></blockquote></div><br></div></div>
</blockquote></div><br></div></div></div></blockquote></div><br></div>
</div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div><span><arm-halt.diff></span></blockquote></div><br></div></div></blockquote></div><br></div>