[llvm-commits] [PATCH] improve ARM halt encoding

[Eli Bendersky]

On Tue, Jan 29, 2013 at 7:56 PM, JF Bastien <jfb at google.com> wrote:
> Here's an updated patch.
>
> I don't have commit rights: Eli, could you commit for me? :-)
>

Done in r173943.


>
> 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>
>>
>>
>