<div class="gmail_extra">Hello again,</div><div class="gmail_extra"><br></div><div class="gmail_extra">I've updated according to most of the solutions, and restructured a lot of the poisoning code.</div><div class="gmail_extra">
Performance looks great, but there's still a bit of work to convert my hand-written tests to the ASan output_tests format.</div><div class="gmail_extra">I'm hoping to get this patch accepted with initialization order off by default, then submit a separate patch to include all of my test cases.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Details on previous issues follow, patch is attached, and rietveld issues are at</div><div class="gmail_extra">llvm: <a href="http://codereview.appspot.com/6432065/">http://codereview.appspot.com/6432065/</a></div>
<div class="gmail_extra">compiler-rt: <a href="http://codereview.appspot.com/6419070/">http://codereview.appspot.com/6419070/</a></div>
<div class="gmail_extra"><br></div><div class="gmail_extra">Thanks again,</div><div class="gmail_extra">Reid</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul 19, 2012 at 2:28 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">
<br><br><div class="gmail_quote"><div>On Thu, Jul 19, 2012 at 3:16 AM, Reid Watson <span dir="ltr"><<a href="mailto:reidw@google.com" target="_blank">reidw@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks again for reviewing this -- I've implemented the suggested<br>
changes, uploaded to Rietveld(<a href="http://codereview.appspot.com/6425049" target="_blank">http://codereview.appspot.com/6425049</a>),<br></blockquote></div><div>Thanks! (although you've uploaded only half of the patch). </div>
<div><br></div><div>Comments on the LLVM part: </div><div><br></div><div>+static bool HasDynamicInitializer(GlobalVariable *G) { </div>
<div><br></div><div>I am not sure how reliable this function is. </div><div>Can we get this information from clang in a form of metadata? </div><div>I don't insists to fix this now, but this will need a fix eventually. </div>
<div><br></div><div>+ if (CurrentFunction->getName().find("__cxx_global_var_init") == 0) { </div>
<div>Shouldn't this be _GLOBAL__I_a?</div><div><br></div></div></blockquote><div> </div><div>_GLOBAL__I_a will call each of the __cxx_global_var_init methods. </div><div>This seems to work for now, but yes, a more robust method of checking this would be nice, and I'll look into this.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div></div><div><br></div><div>+ // size_t poison_during_initialization; </div>
<div>I'd call this has_dynamic_initializer</div><div><br></div><div><br></div><div>Please end the comments with a period. </div><div><br></div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div></div><div>Comments on the compiler-rt part: </div><div><br></div><div><div>+ ScopedLock lock(&mu_for_globals); </div>
<div>+ if (n == 0) </div><div>+ return; </div>
</div><div><br></div><div>Move if (n == 0) before the lock. </div>
<div><br></div><div>+ while (l->g->beg != last->beg) { </div><div><br></div><div>Do you really need these 3 loops? </div><div>At the point where you call this, the globals of the current TU are in the head of the loop. </div>
<div>So, you probably can do just one loop starting from 'first'.</div><div><br></div><div><div>+ while (l) { </div>
<div>+ if (l->g->poison_during_initialization) </div></div><div><br></div><div><br></div><div>I am worried about the performance of this thing. </div><div>I would prefer to have two different lists of globals, one containing only those globals that "has_dynamic_initializer".</div>
<div>You can do this change later if you prefer, but before enabling the option by default. </div></div></blockquote><div><br></div><div>I've restructured the way poisoning works in the patch attached to this email, simplifying this code and improving performance quite a bit.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><br></div><div>Same for tests: you will need to add tests, not necessary in the first commit, but before flipping the asan-initialization-order flag. </div>
<div>I will ask you for at least these kinds of tests: </div><div> - llvm-style tests in the form of .ll file with CHECK statements. </div><div> - "output" style tests (see projects/compiler-rt/lib/asan/output_tests) with the actual bugs. </div>
<div> - stress test: a shell script that auto-generates a program with N C++ modules each containing M1 globals with dynamic init and M2 globals with static init. </div><div> I want to see how O(N*M1) looks like in practice. We should not see O(N*(M1+M2)). </div>
<div>
<div><br></div></div></div></blockquote><div><br></div><div>I'm still in the process of converting my own test cases into the ASan output_test format. </div><div>Almost all of my tests require multiple files, so I may need to tweak test_output.sh a little bit.</div>
<div>As far as performance is concerned, things look great.</div><div>I'm working on finishing up the stress test, and will include it with the testing patch.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
and attached the new patches to this email.<br>
<br>
I left in the extra check which forces access to global scalar<br>
variables to be implemented. It remains necessary to catch the first<br>
case in my previous email,</blockquote><div><br></div></div><div>Please remind me that case. </div><div>I think we need something like this: </div><div><br></div>if (ClOpt && ClOptGlobals) </div><div class="gmail_quote">
if(GlobalVariable *G = dyn_cast<GlobalVariable>(Addr) && !HasDynamicInitializer(G)) { ... </div><div class="gmail_quote"><div><br></div><div>where HasDynamicInitializer is fast (e.g. reads metadata)</div>
<div>
<div><br></div></div></div></blockquote><div><br></div><div>Yes, the decision to restrict what we catch to only variables with dynamic initialization solves this.<br>The initial implementation attempted to catch cases where two initializers in different modules modified a global variable in a third distinct module.</div>
<div>The order of modification to that global would be unspecified.</div><div>An example of this sort of bug would be calling strerror, (or a user defined function which operated in a similar manner) inside of two initializers in different TUs.</div>
<div>However, catching this error required a fair bit of complexity (synchronization could be use, so we would effectively be stuck trying to write a less useful version of ThreadSanitizer) and introduced a number of false positives.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
and all initialization order problems in<br>
which the value being initialized is some simple type. If the<br>
performance hit is unacceptable though, </blockquote><div><br></div></div><div>Performance drop might be acceptable, but I don't want to disable a useful optimization for no reason. </div><div><br></div><div>--kcc </div>
<div><div><div> </div></div></div></div></blockquote></div></div>