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

[John McCall]

On Jun 19, 2013, at 10:43 PM, 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'm sorry, did I ever respond to this?  It LGTM.

John.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130719/9f5e8d7b/attachment.html>