<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Daniel Berlin" <dberlin@dberlin.org><br><b>To: </b>"Jeroen Dobbelaere" <jeroen.dobbelaere@gmail.com><br><b>Cc: </b>reviews+D8056+public+7b63cedb34d51bb1@reviews.llvm.org, "Hal Finkel" <hfinkel@anl.gov>, cfe-commits@cs.uiuc.edu, "cfe-dev@cs.uiuc.edu Developers" <cfe-dev@cs.uiuc.edu><br><b>Sent: </b>Tuesday, March 17, 2015 4:24:01 PM<br><b>Subject: </b>Re: [PATCH] Fix for bug 21725: wrong results with union and strict-aliasing<br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 17, 2015 at 2:03 PM, Jeroen Dobbelaere <span dir="ltr"><<a href="mailto:jeroen.dobbelaere@gmail.com" target="_blank">jeroen.dobbelaere@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div><div><div class="h5"><div class="gmail_quote">On Tue, Mar 17, 2015 at 7:16 PM, <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</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;"><span>In <a href="http://reviews.llvm.org/D8056#141899" target="_blank">http://reviews.llvm.org/D8056#141899</a>, @jeroen.dobbelaere wrote:<br>
<br>
> Further investigation shows that llvm/lib/Analysis/TypeBasedAliasAnalysis.cpp
is not ready to handle struct types where multiple members have the
same (0) offset. The clang fix only makes sense once that issue is
resolved.<br>
<br>
<br>
</span>Can you work on this? If not (or even if you can), can you file a bug report with an example?<br>
<div><div><br>
<br>
<a href="http://reviews.llvm.org/D8056" target="_blank">http://reviews.llvm.org/D8056</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br></div></div></blockquote><div> </div></div></div></div>Yes, I am looking into it. I have a version now where 'most things' work, but it seems that the tbaa information that clang produces is not expressive enough when the struct/union has array members. For example:<br></div><div><br>union U03 {<br> short mS[4];<br> int mI;<br>};<br><br>results in:<br><br>!5 = !{!"omnipotent char", !6, i64 0}<br>!6 = !{!"Simple C/C++ TBAA"}<br>!7 = !{!8, !8, i64 0}<br>!8 = !{!"int", !5, i64 0}<br>!9 = !{!10, !10, i64 0}<br>!10 = !{!"short", !5, i64 0}<br>!40 = !{!41, !8, i64 0}<br>!41 = !{!"_ZTS3U03", !5, i64 0, !8, i64 0}<br><br></div><div>Note that In !41, we loose the information about the 'short' member.<br><br></div></div></blockquote><div>I would start here :)<br></div><div><br></div><div id="DWT5655">You should make it produce info about all the members.</div></div></div></div></blockquote><br>I agree.<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div></div><div>A function like:<br><br>void test_u03b(union U03* a, union U03* b)<br>{<br> a->mS[0]=1;<br> b->mI=2;<br><br> // CHECK: Function: _Z9test_u03b{{.*}}<br> //FIXME: CHECK: MayAlias: store i32 2, i32* %{{.*}}, align 4, !tbaa !{{.*}} <-> store i16 1, i16* %{{.*}}, align 2, !tbaa !{{.*}}<br>}<br><br></div><div>results in wrong information: an access to mS[0] results in a 'short' (!10) access. The access to mI results in '!40'.<br></div><div>Note that for the short '!10' access, we loose the information about the associated union.<br></div></div></blockquote><div> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div></div><div>Because !41 refers to char '!5' and int '8', but not to short '!10', we cannot detect that there might <br>be overlap between '!40' and '!10'.<br></div></div></blockquote><div><br></div><div>RIght, but this is clearly a bug in the info above.</div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div></div><div>When we have two arrays of different type, the problem becomes even worse as we now have no<br>connection to the union any more from the two accesses...<br></div></div></blockquote><div><br></div><div>I went through this on the whiteboard with folks when they designed this, and pointed this out - it's pretty easy to get into situations where you have two random pieces of memory, and they were part of a union, and you don't know this.</div><div><br></div><div>You can make them as easy or as hard as you like.</div><div><br></div><div id="DWT5656">The answer I got was "we're fine with people having to make this explicit".</div></div></div></div></blockquote><br>I don't understand what this means. How should they do that? The language has a defined set of semantics, and we need to follow them. At the very least, if we don't want to fully represent the TBAA hierarchy for some types (I'm not sure why we'd not do this, but...) then we should omit the metadata all together.<br><br> -Hal<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div><br></div><div>One issue with getting array accesses right that I see, is that the 'offset' field can suddenly be variable.<br></div><div><br></div></div></blockquote><div><br></div><div>Not really. You can't say where it points, so the range would be "everything" anyway.</div><div>You should just say it accesses the entire array (but not anything else).</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div>Maybe a good enough (and correct) fallback that we should consider is to make such _union_ accesses alias with everything... (omnipotent char) in stead of mapping it to the basic type.<br></div></div></blockquote><div><br></div><div>It is possible, with the sets we have, to get the aliasing right :)</div><div>I know this because GCC does it.</div><div> </div><div>This design is nearly identical to GCC's TBAA and subvariable design :)</div><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div dir="ltr"><div><br></div><div>Any thoughts ?<span class="HOEnZb"><font color="#888888"><br></font></span></div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Jeroen Dobbelaere<br><br></div></font></span></div>
</blockquote></div><br></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>