<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 19 February 2014 10:11, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><br>
On 02/18/2014 09:27 PM, Nick Lewycky wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
LLVM's hand-rolled RTTI differs from C++'s in that it allows you to call the construction method on a base class and then based on the dynamic arguments decide to construct a more derived type and return a pointer to that. This looks like calling CallInst::Create and getting back something you can cast<MemSetInst>(). However, we implement this by new'ing a CallInst, which breaks C++ aliasing rules.<br>


<br>
The attached patch fixes this undefined behaviour by making CallInst forward to IntrinsicInst to construct intrinsics, which in turn uses private constructors on the most derived types. There is some runtime cost, as the implementation of CallInst::Create moved from the header to the .cpp file. If this is a serious problem, let me know and I can probably fix that, I just think the result will result in less much readable code.<br>


<br>
Please review!<br>
</blockquote></div>
My only correctness concern would be the removal of:<br>
void operator=(const IntrinsicInst&) LLVM_DELETED_FUNCTION;<br>
from IntrinsicInst.  That looks suspicious.<br>
<br>
I agree with David on the yuck factor of that much duplicated code though.  Templates, or other copy removal would be appreciated, but not required.<br>
<br>
Another point: is there any way to detect if someone adds a new subclass of Instrinsic which needs added to your switch?  Maybe an assert in the protected constructor which is only satisfied coming from Create(...)?  Seems a bit hacky, but might save someone a lot of debugging time.  Again, appreciated, not required.<br>

</blockquote><div><br></div><div>I don't think that would work either? IntrinsicInst is sometimes the dynamic type. If somebody adds a new type, and doesn't update the switch in Create then we end up falling through to creating an IntrinsicInst instead of their new type. We can't assert on that because we can't tell whether this intrinsic is supposed to be created as an IntrinsicInst or if there happens to exist a wrapper type we needed to use.</div>

<div></div></div><br></div><div class="gmail_extra">Nick</div></div>