<br><br><div class="gmail_quote">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>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><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>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><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><br></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>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><br></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>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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">then I'd be open to adding an<br>
extra flag to control whether or not to check initialization order on<br>
these types of globals.<br>
<br>
All the best,<br>
Reid<br>
<div class="HOEnZb"><div class="h5"><br>
On Tue, Jul 17, 2012 at 6:31 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
><br>
><br>
> On Sat, Jul 14, 2012 at 12:50 AM, Reid Watson <<a href="mailto:reidw@google.com">reidw@google.com</a>> wrote:<br>
>><br>
>> Ok, I've updated the code to reflect the suggested changes, except for<br>
>> two issues still up in the air. Once these are solved I'll email out<br>
>> an updated patch.<br>
>><br>
>> 1. Disabling ClOptGlobals, detecting linker initialized globals<br>
>> The code which disables the optimization of access to global scalar<br>
>> values is necessary for catching cases where the initialization order<br>
>> happens on a dynamically initialized global scalar value. The<br>
>> following example prints different output depending on the order the<br>
>> input files are given to the compiler, and requires ClOptGlobals to be<br>
>> ignored at one point for this patch to catch it:<br>
><br>
><br>
> Consider you have a C++ program<br>
><br>
> int A; // linker-initialized by zero<br>
> int B = foo(); // initialized at start-up.<br>
><br>
> int bar() { return A + B; }<br>
><br>
> Currently, with ClOptGlobals=1 (i.e. by default) both accesses in bar() are<br>
> ignored.<br>
> With your change, both accesses will be instrumented.<br>
> I think we still need to ignore the access to A.<br>
><br>
><br>
><br>
>><br>
>><br>
>> //File x.cpp<br>
>> #incldue <cstdio><br>
>> extern int y;<br>
>> int f() {<br>
>> printf("using 'y' (which is %d)\n", y);<br>
>> return 3*y+7;<br>
>> }<br>
>> int x = f();<br>
>> int main(){}<br>
>><br>
>> //File y.cpp<br>
>> #include <cstdio><br>
>> int g() {<br>
>> puts("initializing 'y'");<br>
>> return 5;<br>
>> }<br>
>> int y = g();<br>
>><br>
>> Access to linker initialized globals can still result in<br>
>> initialization order problems.<br>
>> Consider this example, which outputs 42 or 84 depending on the order<br>
>> of arguments to the compiler.<br>
>> //FIle setcode.cpp<br>
>> static int code;<br>
>> int getCode(){ return code;}<br>
>> void setCode(int _code){ code = _code;}<br>
>><br>
>> //File x.cpp<br>
>> #include <cstdio><br>
>> void setCode(int);<br>
>> int getCode();<br>
>><br>
>> int initializeX(){<br>
>> setCode(42);<br>
>> return 0;<br>
>> }<br>
>> int x = initializeX();<br>
>><br>
>> int main() {<br>
>> printf("getCode returns %d\n", getCode());<br>
>> }<br>
>><br>
>> //File y.cpp<br>
>> void setCode(int);<br>
>><br>
>> int initializeY(){<br>
>> setCode(84);<br>
>> return 0;<br>
>> }<br>
>><br>
>> int y = initializeY();<br>
>><br>
>><br>
>> 2. Finding the global init function<br>
>> Right now, _GLOBAL__I_a is the hardcoded name of the function which<br>
>> will begin dynamic initialization. It's possible to add metadata<br>
>> identifying it as that function, but the same hard-coding would still<br>
>> be in place.<br>
><br>
><br>
> Ok. Let this name be hardcoded, unless someone else has a better idea.<br>
><br>
> --kcc<br>
><br>
>><br>
>> Thanks,<br>
>> Reid<br>
>><br>
>> On Fri, Jul 13, 2012 at 10:02 AM, Reid Watson <<a href="mailto:reidw@google.com">reidw@google.com</a>> wrote:<br>
>> > Kostya,<br>
>> ><br>
>> > I completely missed that (caught in a poorly written email filter).<br>
>> > Thanks for reviewing it for me, and I'll use rietveld when submitting<br>
>> > the update d version.<br>
>> > I'm looking at the code now, and I'll update when I've implemented the<br>
>> > suggested changes.<br>
>> ><br>
>> > --Reid<br>
>> ><br>
>> > On Fri, Jul 13, 2012 at 4:41 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>><br>
>> > wrote:<br>
>> >> Reid,<br>
>> >><br>
>> >> Have you seen my replies to your previous patches?<br>
>> >> <a href="http://comments.gmane.org/gmane.comp.compilers.clang.scm/53381" target="_blank">http://comments.gmane.org/gmane.comp.compilers.clang.scm/53381</a><br>
>> >> <a href="http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/116782" target="_blank">http://permalink.gmane.org/gmane.comp.compilers.llvm.cvs/116782</a><br>
>> >><br>
>> >> --kcc<br>
>> >><br>
>> >> On Thu, Jul 12, 2012 at 9:19 PM, Reid Watson <<a href="mailto:reidw@google.com">reidw@google.com</a>> wrote:<br>
>> >>><br>
>> >>> Hello,<br>
>> >>><br>
>> >>> Attached are two patches which add support for detecting<br>
>> >>> initialization order problems in AddressSanitizer.<br>
>> >>> Issues with initialization order are checked by poisoning the shadow<br>
>> >>> memory of global variables which are not guaranteed to have been<br>
>> >>> initialized before each modules initializers run.<br>
>> >>> This approach catches the most obvious issues with initialization<br>
>> >>> order, and correctly ignores access to global variables which should<br>
>> >>> be accessible during initializers (function local statics, etc.).<br>
>> >>><br>
>> >>> All the best,<br>
>> >>> Reid<br>
>> >>><br>
>> >>> _______________________________________________<br>
>> >>> llvm-commits mailing list<br>
>> >>> <a href="mailto:llvm-commits@cs.uiuc.edu">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>
>> >><br>
><br>
><br>
</div></div></blockquote></div><br>