[llvm-commits] [PATCH] improve ARM halt encoding
JF Bastien
jfb at google.com
Tue Jan 29 19:56:26 PST 2013
Here's an updated patch.
I don't have commit rights: Eli, could you commit for me? :-)
On Tue, Jan 29, 2013 at 5:46 PM, Jim Grosbach <grosbach at apple.com> wrote:
> Hi JF,
>
> This looks fine to me, with a could of very minor nits:
>
> + BuildMI(TrapBB, dl, TII->get(
> + Subtarget->isThumb() ? ARM::tTRAP :
> + (Subtarget->emitNaClTrap() ? ARM::TRAPNaCl : ARM::TRAP)));
>
> 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.
> unsigned NewOpc = Subtarget->isThumb() ….
> … TII->get(NewOpc) …
>
> +def EmitNaClTrap : Predicate<"Subtarget->emitNaClTrap()">,
> + AssemblerPredicate<"FeatureNaClTrap",
> "NaCl">;
> +def DontEmitNaClTrap : Predicate<"!Subtarget->emitNaClTrap()">;
>
> For things like this, the predicates are typically named with "Use" and
> "DontUse" rather than "Emit".
>
> Good to commit with those changes.
>
> -Jim
>
> On Jan 23, 2013, at 1:35 PM, JF Bastien <jfb at google.com> wrote:
>
> Hi Jim,
>
> 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.
>
> Please take a look.
>
> Thanks,
>
> JF
>
>
> On Tue, Jan 22, 2013 at 4:28 PM, JF Bastien <jfb at google.com> wrote:
>
>> Hi Jim,
>>
>> Here's an updated patch.
>>
>> 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.
>>
>> 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?
>>
>> Thanks,
>>
>> JF
>>
>>
>> On Mon, Jan 21, 2013 at 2:42 PM, JF Bastien <jfb at google.com> wrote:
>>
>>> I see, that's what I was trying to avoid without realizing that it was
>>> the only way to go :-)
>>>
>>> I'll send an updated patch tomorrow, at least so I can pretend to take
>>> MLK off.
>>>
>>>
>>> On Mon, Jan 21, 2013 at 10:43 AM, Jim Grosbach <grosbach at apple.com>wrote:
>>>
>>>> Hi JF,
>>>>
>>>> That's the ugly part. You need two totally separate definitions,
>>>> including a different instruction name.
>>>>
>>>> // The new definition.
>>>> def TRAPnacl: …. Requires<[IsARM, IsPNaCL]>;
>>>> // The old definition. Updated "Requires" clause.
>>>> def TRAP: …. Requires<[IsARM, IsNotPNaCL]>;
>>>>
>>>> Any .cpp code which references ARM::TRAP will also need to be updated.
>>>>
>>>> -Jim
>>>>
>>>> On Jan 18, 2013, at 11:27 AM, JF Bastien <jfb at google.com> wrote:
>>>>
>>>> I've tried a few different things and tablegen eludes me. How do I do
>>>> something like this:
>>>> let isBarrier = 1, isTerminator = 1 in
>>>> def TRAP : AXI<(outs), (ins), MiscFrm, NoItinerary,
>>>> "trap", [(trap)]>,
>>>> Requires<[IsARM]> {
>>>> bits<16> imm16 = [{
>>>> if (IsNaCl)
>>>> return 0b1110110111100000;
>>>> return 0b1111110111101110;
>>>> }];
>>>> let Inst{31-20} = 0b111001111111;
>>>> let Inst{19-8} = imm16{15-4};
>>>> let Inst{7-4} = 0b1111;
>>>> let Inst{3-0} = imm16{3-0};
>>>> }
>>>> ?
>>>>
>>>> Multiclasses+Requires doesn't quite work because it would create
>>>> different names for the NaCl/non-NaCl versions.
>>>>
>>>>
>>>> On Wed, Jan 16, 2013 at 5:25 PM, Jim Grosbach <grosbach at apple.com>wrote:
>>>>
>>>>>
>>>>> On Jan 16, 2013, at 4:58 PM, JF Bastien <jfb at google.com> wrote:
>>>>>
>>>>> 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.
>>>>>
>>>>>
>>>>> 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!
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>>
>>>>> Given this, would it make sense to commit this change, once I
>>>>> specialize it for the NaCl ARM target?
>>>>>
>>>>>
>>>>> Yeah, that's fine. I'll probably grumble a bit about the added
>>>>> complexity, but it's the right thing to do. :)
>>>>>
>>>>> -Jim
>>>>>
>>>>>
>>>>> On Wed, Jan 16, 2013 at 4:33 PM, Jim Grosbach <grosbach at apple.com>wrote:
>>>>>
>>>>>> Hi JF,
>>>>>>
>>>>>> 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?
>>>>>>
>>>>>> 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…
>>>>>>
>>>>>> -Jim
>>>>>>
>>>>>> On Jan 16, 2013, at 4:27 PM, JF Bastien <jfb at google.com> wrote:
>>>>>>
>>>>>> As discussed over IRC: it then makes sense to only change the
>>>>>> encoding for the NaCl triple (which is effectively its own OS).
>>>>>>
>>>>>>
>>>>>> On Wed, Jan 16, 2013 at 4:17 PM, Jim Grosbach <grosbach at apple.com>wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jan 16, 2013, at 3:45 PM, Renato Golin Linaro <
>>>>>>> renato.golin at linaro.org> wrote:
>>>>>>>
>>>>>>> On 16 January 2013 22:39, Jim Grosbach <grosbach at apple.com> wrote:
>>>>>>>
>>>>>>>> The entire encoding is semantically significant on Darwin. I
>>>>>>>> suspect that's true on other platforms, too, but I don't know for sure.
>>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> -Jim
>>>>>>>
>>>>>>>
>>>>>>> Bastien, Have you tested in which platforms?
>>>>>>>
>>>>>>> cheers,
>>>>>>> --renato
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
> <arm-halt.diff>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130129/d9ed7d8b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-halt.diff
Type: application/octet-stream
Size: 10117 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130129/d9ed7d8b/attachment.obj>
More information about the llvm-commits
mailing list