On Jul 15, 2013, at 12:58 PM, Alon Zakai <[email protected]> wrote: > ----- Original Message ----- >> From: "Mark Seaborn" <[email protected]> >> To: "John McCall" <[email protected]> >> Cc: [email protected], "Derek Schuff" <[email protected]>, "Eli >> Bendersky" <[email protected]>, >> [email protected] >> Sent: Monday, July 15, 2013 12:49:09 PM >> Subject: Re: [PATCH] Use ARM-style representation for C++ method pointers >> under PNaCl >> >> On 8 July 2013 19:29, John McCall <[email protected]> wrote: >> >>> On Jul 8, 2013, at 7:16 PM, Mark Seaborn <[email protected]> wrote: >>> >>> On 8 July 2013 15:42, John McCall <[email protected]> wrote: >>> >>>> On Jul 8, 2013, at 3:38 PM, Mark Seaborn <[email protected]> wrote: >>>> >>>> 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. >>>> >>>> >>>> So, PNaCl cannot be used to generate interoperable i386 code; you always >>>> have to emit a separate i386 slice? >>>> >>> >>> That's right. At least, interoperability with native ABIs is limited >>> unless you pass an architecture-specific target triple to Clang when >>> compiling. >>> >>> >>> Okay, then I agree that you need to at least check the arch. >>> >>> I'm not familiar with the LLVM history, but the comment in Triple.h >>> suggests that there are multiple platforms on le32. So either (1) check >>> specifically for your platform, which is the combination of le32 and nacl, >>> or (2) convince me (shouldn't take much) that all the platforms either want >>> this or explicitly don't care. >>> >> >> Yes, "le32" covers PNaCl and Emscripten. The comment in Triple.h is: >> >> le32, // le32: generic little-endian 32-bit CPU (PNaCl / Emscripten) >> >> I think both platforms want this change. Emscripten contains a workaround >> for making C++ method pointers work when Clang generates code using the >> Itanium/x86 ABI. Emscripten allocates every function a small integer ID, >> which is an index into a function table. Currently, Emscripten ensures >> that every function gets an even-numbered ID by inserting dummy entries >> into the function table. If my Clang change were committed, and if it were >> enabled for Emscripten, Emscripten could remove this workaround. >> >> Specifically, the workaround is implemented by the following line in >> Emscripten's modules.js: >> this.nextIndex += 2; // Need to have indexes be even numbers, see >> |polymorph| test >> (https://github.com/kripken/emscripten/blob/master/src/modules.js) >> >> That said, I don't work on Emscripten -- CC'ing Emscripten maintainer Alon >> Zakai in case he wants to comment. >> > > Thanks, yeah, that is 100% accurate. Emscripten would benefit from the change > as well.
Okay, thanks. In that case, I'm fine with basing this purely on le32. Please reference the fact that both PNaCl and Emscripten want this in your commit message. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
