On 8 July 2013 13:51, John McCall <[email protected]> wrote: > On Jul 8, 2013, at 12:56 PM, Mark Seaborn <[email protected]> wrote: > > On 19 June 2013 22:43, 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'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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
