Any comments? <br><br><div class="gmail_quote">On Thu, May 3, 2012 at 10:51 AM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>[resurrecting an old thread...]</div><div><br></div><div>Chris, </div><div><br></div><div>today we use a function attribute (AddressSafety) to mark a function for instrumentation with AddressSanitizer. </div><div>We also need a similar thing for ThreadSanitizer. </div>
<div>Currently these attributes are checked in the instrumentation passes (asan, tsan) and in GVN, where we need to disable {asan,tsan}-hostile transformation. </div><div><br></div><div>You suggested to use <span>module flags metadata instead of function attributes. </span></div>
<div>Is this what you mean (named metadata node that contains a list of functions)?</div><div><div> !llvm.address_safety_analysis = !{!0, !1, !2}</div><div> !0 = metadata !{i32 (i16*)* @Foo}</div><div> !1 = metadata !{i32 (i8*)* @Bar}</div>
<div> !2 = metadata !{i32 (i32*)* @Buz}</div></div><div><br></div><div>With this approach the attribute lookup will be a bit more expensive (either linear search, or metadata sort + bsearch, or an auxiliary hash table).</div>
<div>Not a great deal though. </div><div>One way is to add sort(), isSorted() and binarySearch() methods to NamedMDNode. </div><div><br></div><div>Thanks, </div><div><br></div><div>--kcc </div><div class="HOEnZb"><div class="h5">
<div><br></div><div><br></div>
<div><br></div><div><br></div><div><br><br><div class="gmail_quote">On Wed, Mar 21, 2012 at 10:59 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I wonder if we can use a function-level metadata instead of module-level metadata (and instead of function attributes). <div>
(I don't see such, so we may need to add and document the notion of function-level metadata, similar to module-level).</div>
<div>WDYT?</div><div><br></div><div>OTOH, we may add one more attribute and reconsider the problem later. </div><div>Neither ASAN nor TSAN require backward compatibility with LLVM IR, so these two LLVM function attributes (AddressSafety, ThreadSafety)</div>
<div>can be easily replaced with function-level metadata once we have it. </div><div><br></div><div>--kcc </div><div><div><div><br></div><div><br></div><div><br><div class="gmail_quote">On Fri, Mar 16, 2012 at 5:42 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.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="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div class="gmail_quote">
<div>Also, if I use metadata for tsan and an attribute for asan, it will look weird. </div><div>Would you suggest to use metadata for asan as well (instead of AddressSafety attribute)? </div></div></blockquote><div><br></div>
</div><div>Yes please! :)</div><div><br></div></div></div></blockquote><div><br></div></div><div>Ok (once we find out what's the right way). </div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div class="gmail_quote">
<div>The AddressSafety attribute is checked at least in one place outside of asan code (lib/Analysis/MemoryDependenceAnalysis.cpp).</div><div>The ThreadSafety attribute (or equivalent metadata) will need to be checked in the same place too:<br>
</div><div>struct X {</div><div> uint64_t for_alignment;</div><div> char a, b, c, d;</div><div>};</div><div><br></div><div>Thread1(X *x) {</div><div> ... = X.a + X.d</div><div>}</div><div><br></div><div>Thread2(X *x) {</div>
<div> X.b = ...</div><div>}<br></div><div><br></div><div>If GVN widens X.a + X.d, tsan will start reporting a non-existing race. </div></div></blockquote><br></div></div><div>I never agreed with this patch, but let it go in to avoid blocking forward progress.</div>
</div></blockquote><div><br></div></div><div>Thanks!</div><div>I agree that the patch is not perfect (all problems that you mentioned, especially with inlining). </div><div><br></div><div><shameless plug></div><div>
BTW, this particular patch unblocked the progress for mozilla (e.g. <a href="http://www.mozilla.org/security/announce/2012/mfsa2012-14.html" target="_blank">http://www.mozilla.org/security/announce/2012/mfsa2012-14.html</a>)</div>
<div>and clang+asan bootstrap (found ~5 clang/llvm bugs already, more are in my queue)</div><div></shameless plug><br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div>However, now that you ask :), GVN should never have checked for a "asan" attribute. If you want an attribute to disable specific transformations, we should design this properly, and it should be a property of the load or store, not a property of the function. </div>
</div></blockquote><div><br></div></div><div>I don't see how to attach the property to loads/stores.</div><div>If we use instruction-level metadata, it may be lost (as you just explained) above. </div><div>If we add an instruction-level attribute (are there such?), what value should be used if some phase creates a new load/store? </div>
<div>Again, what should be done when some code is copied (inlined, unrolled, etc)?</div><div><br></div><div> </div><div>--kcc </div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br>