<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 class="h5"><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><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><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>
<br>Cheers,<br>Mark<br><br></div></div></div></div>