On Jun 19, 2013, at 10:43 PM, Mark Seaborn <[email protected]> wrote: > On 19 June 2013 15:20, John McCall <[email protected]> wrote: > On Jun 19, 2013, at 3:17 PM, Mark Seaborn <[email protected]> wrote: >> On 19 June 2013 13:17, John McCall <[email protected]> wrote: >> On Jun 19, 2013, at 1:05 PM, Mark Seaborn <[email protected]> wrote: >>> On 19 June 2013 13:01, Mark Seaborn <[email protected]> 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
