<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 8 July 2013 19:29, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="h5"><div><div>On Jul 8, 2013, at 7:16 PM, Mark Seaborn <<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>> wrote:</div><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 8 July 2013 15:42, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div>On Jul 8, 2013, at 3:38 PM, Mark Seaborn <<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>> wrote:</div><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 8 July 2013 13:51, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div><div>On Jul 8, 2013, at 12:56 PM, Mark Seaborn <<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>> wrote:</div><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 19 June 2013 22:43, Mark Seaborn <span dir="ltr"><<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>On 19 June 2013 15:20, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div>On Jun 19, 2013, at 3:17 PM, Mark Seaborn <<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>> wrote:</div><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 19 June 2013 13:17, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div>On Jun 19, 2013, at 1:05 PM, Mark Seaborn <<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>> wrote:</div><blockquote type="cite">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 19 June 2013 13:01, Mark Seaborn <span dir="ltr"><<a href="mailto:mseaborn@chromium.org" target="_blank">mseaborn@chromium.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Use ARM-style representation for C++ method pointers under PNaCl<br><br>Before this change, Clang uses the x86 representation for C++ method<br>
pointers when generating code for PNaCl. However, the resulting code<br>
will assume that function pointers are 0 mod 2. This assumption is<br>not safe for PNaCl, where function pointers could have any value<br>(especially in future sandboxing models).<br><br>So, switch to using the ARM representation for PNaCl code, which makes<br>
no assumptions about the alignment of function pointers.<br><br>See: <a href="https://code.google.com/p/nativeclient/issues/detail?id=3450" target="_blank">https://code.google.com/p/nativeclient/issues/detail?id=3450</a><br>
</div></blockquote><div><br></div><div>Oops, I meant to send this to cfe-commits rather than llvm-commits.<br></div></div></div></div></blockquote></div><br></div><div>I do not think you should just unconditionally opt in to random ARM</div>
<div>behavior. In particular, ARM uses 32-bit guard variables because that's</div><div>the size of a pointer on ARM; PNaCl needs to be able to efficiently</div><div>support 64-bit platforms as well.</div></div></blockquote>
<div><br></div><div>The code does always use 64-bit guard variables on 64-bit systems. It does this:<br><br> // Guard variables are 64 bits in the generic ABI and size width on ARM<br> // (i.e. 32-bit on AArch32, 64-bit on AArch64).<br>
guardTy = (IsARM ? CGF.SizeTy : CGF.Int64Ty);<br><br></div><div>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". :-)<br>
</div></div></div></div></blockquote></div><br></div><div>Interesting, okay.</div><div><br></div><div>I still do not want PNaCl to claim to be ARM. Abstract the code so that</div><div>you can opt into the specific behaviors you want without pretending to</div>
<div>be ARM.<br></div></div></blockquote><div><br></div></div><div>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.<br>
<br></div><div>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.<br>
</div></div></div></div></blockquote><div><br></div><div>I've updated the patch to also add a test for C++ guard variable usage under PNaCl. Is this OK to commit?<br></div></div></div></div></blockquote></div><br></div>
</div><div><div>+ bool UseARMMethodPtrABI;</div><div>+ bool UseARMGuardVarABI;</div><div> </div></div><div>Good enough.</div><div><br></div><div><div>+ if (CGM.getContext().getTargetInfo().getTriple().getArch()</div>
<div>+ == llvm::Triple::le32) {</div><div><br></div><div>I think testing the OS would be more reasonable here.<br></div></div></div></blockquote><div><br></div><div>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.<br>
</div></div></div></div></blockquote></div><br></div><div>So, PNaCl cannot be used to generate interoperable i386 code; you always have to emit a separate i386 slice?<br></div></div></blockquote><div><br></div>That's right. At least, interoperability with native ABIs is limited unless you pass an architecture-specific target triple to Clang when compiling.<br>
</div></div></div></blockquote></div><br></div></div><div>Okay, then I agree that you need to at least check the arch.</div><div><br></div><div>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.<br>
</div></div></blockquote><div><br></div><div>Yes, "le32" covers PNaCl and Emscripten. The comment in Triple.h is:<br><br> le32, // le32: generic little-endian 32-bit CPU (PNaCl / Emscripten)<br><br></div>
<div>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.<br>
<br>Specifically, the workaround is implemented by the following line in Emscripten's modules.js:<br> this.nextIndex += 2; // Need to have indexes be even numbers, see |polymorph| test<br>(<a href="https://github.com/kripken/emscripten/blob/master/src/modules.js">https://github.com/kripken/emscripten/blob/master/src/modules.js</a>)<br>
<br></div><div>That said, I don't work on Emscripten -- CC'ing Emscripten maintainer Alon Zakai in case he wants to comment.<br><br>Cheers,<br>Mark<br><br></div></div></div></div>