[PATCH] Use ARM-style representation for C++ method pointers under PNaCl

[Mark Seaborn]

On 8 July 2013 13:51, John McCall <rjmccall at apple.com> wrote:

> On Jul 8, 2013, at 12:56 PM, Mark Seaborn <mseaborn at chromium.org> wrote:
>
> On 19 June 2013 22:43, Mark Seaborn <mseaborn at chromium.org> wrote:
>
>> On 19 June 2013 15:20, John McCall <rjmccall at apple.com> wrote:
>>
>>> On Jun 19, 2013, at 3:17 PM, Mark Seaborn <mseaborn at chromium.org> wrote:
>>>
>>> On 19 June 2013 13:17, John McCall <rjmccall at apple.com> wrote:
>>>
>>>> On Jun 19, 2013, at 1:05 PM, Mark Seaborn <mseaborn at chromium.org>
>>>> wrote:
>>>>
>>>> On 19 June 2013 13:01, Mark Seaborn <mseaborn at chromium.org> wrote:
>>>>
>>>>> Use ARM-style representation for C++ method pointers under PNaCl
>>>>>
>>>>> Before this change, Clang uses the x86 representation for C++ method
>>>>> pointers when generating code for PNaCl.  However, the resulting code
>>>>> will assume that function pointers are 0 mod 2.  This assumption is
>>>>> not safe for PNaCl, where function pointers could have any value
>>>>> (especially in future sandboxing models).
>>>>>
>>>>> So, switch to using the ARM representation for PNaCl code, which makes
>>>>> no assumptions about the alignment of function pointers.
>>>>>
>>>>> See: https://code.google.com/p/nativeclient/issues/detail?id=3450
>>>>>
>>>>
>>>> Oops, I meant to send this to cfe-commits rather than llvm-commits.
>>>>
>>>>
>>>> I do not think you should just unconditionally opt in to random ARM
>>>> behavior.  In particular, ARM uses 32-bit guard variables because that's
>>>> the size of a pointer on ARM;  PNaCl needs to be able to efficiently
>>>> support 64-bit platforms as well.
>>>>
>>>
>>> The code does always use 64-bit guard variables on 64-bit systems.  It
>>> does this:
>>>
>>>     // Guard variables are 64 bits in the generic ABI and size width on
>>> ARM
>>>     // (i.e. 32-bit on AArch32, 64-bit on AArch64).
>>>     guardTy = (IsARM ? CGF.SizeTy : CGF.Int64Ty);
>>>
>>> Having said that, PNaCl is 32-bit-only:  PNaCl programs assume a 32-bit
>>> address space.  We don't support 64-bit pointers in PNaCl.  In Clang,
>>> targeting PNaCl is identified by "le32" being in the triple, and I assume
>>> there's no way to get 64-bit pointers with "le32". :-)
>>>
>>>
>>> Interesting, okay.
>>>
>>> I still do not want PNaCl to claim to be ARM.  Abstract the code so that
>>> you can opt into the specific behaviors you want without pretending to
>>> be ARM.
>>>
>>
>> OK, I have changed the patch to split IsARM into two separate fields.  I
>> called the fields UseARMMethodPtrABI and UseARMGuardVarABI out of a lack of
>> imagination.
>>
>> I've changed the patch to use the non-ARM guard variable ABI for PNaCl.
>> Having looked at the code more carefully, I see it's inlining a different
>> guard variable check on ARM, which we don't necessarily want to use for
>> PNaCl; it's not just a different guard var size.
>>
>
> I've updated the patch to also add a test for C++ guard variable usage
> under PNaCl.  Is this OK to commit?
>
>
> +  bool UseARMMethodPtrABI;
> +  bool UseARMGuardVarABI;
>
> Good enough.
>
> +    if (CGM.getContext().getTargetInfo().getTriple().getArch()
> +        == llvm::Triple::le32) {
>
> I think testing the OS would be more reasonable here.
>

Actually, we do want to test the architecture field here.  If we use the
triple "i686-unknown-nacl", we want to get the x86 ABI for C++ method
pointers, so that the generated code is interoperable with the NaCl GCC
toolchain.  If Clang tested the OS field here, that would break this use
case.

Cheers,
Mark
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130708/c7c1bf28/attachment.html>