<br><br><div class="gmail_quote">On Mon, Jul 2, 2012 at 11:34 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">
Hi Reid, <div><br></div><div>+// This flag may need to be replaced with -fasan-initializers.  </div><div>Ideally, this phase should be on-by default when asan is enabled. </div><div>Assuming it will be robust and have no false reports, of course. </div>

<div>For now, -mllvm -asan-initializers is ok for me.</div><div><br></div><div><br></div><div><div><div>   size_t n = GlobalsToChange.size();                                                                                                                                                               </div>

<div>+  NumGlobals = n;    </div></div></div><div><br></div><div>Please remove 'n'.</div><div><br></div><div>+    if (ClInitializers && F->getName() == "_GLOBAL__I_a")                                                                                                                                          </div>

<div><br></div><div>This is wrong. We should not identify global init functions using the name. </div><div>There should be some other way (attribute? metadata?)</div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>+  // Add a call to poison all external globals before the given function starts                                                                                                                    </div>

<div><div><br></div><div>Ouch. This will make the algorithm quadratic by the number of global variables. </div><div>Essentially, we poison all globals each time we initialize a global. </div><div>Instead, we need to poison all globals of all modules (except for the current) when we start handling a given module. </div>

<div>This will make it quadratic by the number of modules, which is tolerable. </div></div></blockquote><div><br></div><div>I am wrong here, you seem to be inserting the callbacks into the right place. </div><div>Than another comment: </div>
<div><br></div><div>+  SmallVector<ReturnInst*, 8> ReturnInstructions;                                                                                                                                                  </div>
<div>Do you need this temp? Maybe have one loop instead? </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br></div><div>Besides, I don't see how you distinguish between globals that have run-time init and linker-initialized-globals. </div>

<div>In particular, the following is sad: </div><div>+  if (isa<GlobalVariable>(Addr) && ClOpt && ClOptGlobals && !ClInitializers) {                                                                                                                     </div>

<div>Here you disable a performance optimization that (last time I measured) gave ~1% of speedup. </div><div>Meanwhile, we still don't need to touch linker-initialized globals. </div><div><br></div><div>May I ask you to send next asan patches using rietveld (in addition to attaching the patch to the message)? </div>

<div>I know that LLVM community does not require that, but my code review  productivity drops when I look at raw patches. </div><div><br></div><div>Thanks! </div><div><br></div><div><br></div><div><br></div><div><br><br>
<div class="gmail_quote"><div><div class="h5">
On Sat, Jun 30, 2012 at 1:36 AM, Reid Watson <span dir="ltr"><<a href="mailto:reidw@google.com" target="_blank">reidw@google.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">
Hello,<br>
<br>
I've attached a patch to add basic support for detecting problems with<br>
initialization order in C++ to AddressSanitizer.<br>
This patch relies on changes to the runtime library, and I've sent a<br>
patch for this to cfe-commits.<br>
This is definitely a first draft, and it leans toward false positives<br>
(function local statics, in particular), but it does detect the most<br>
basic cases of the "static initialization order fiasco".<br>
For now, I'd like to get a working baseline/infrastructure committed,<br>
in order to avoid any monster commits.<br>
<br>
All the best,<br>
Reid<br>
<br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>
</blockquote></div><br>