<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><br><br><div class="gmail_quote">On Tue, Nov 20, 2012 at 4:17 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@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="HOEnZb"><div class="h5">On Tue, Nov 20, 2012 at 4:12 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>

><br>
><br>
> On Tue, Nov 20, 2012 at 3:05 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Nov 19, 2012 at 4:46 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Nov 19, 2012 at 4:33 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> FYI, it shouldn't be moved back to a module pass. I'm just suggesting<br>
>> >> the<br>
>> >> logic in doFinalization should be lifted into a module pass.<br>
>> ><br>
>> > You mean split the AddressSanitizer class into two? This will cause some<br>
>> > pain.<br>
>> > There are things computed at doInitialization or at runOnFunction, and<br>
>> > then<br>
>> > used in doFinalization.<br>
>> > E.g. DynamicallyInitializedGlobals will need to be computed twice.<br>
>> > GlobalsCreatedByAsan will have to be replaced with some other way to<br>
>> > mark<br>
>> > globals created by asan (use metadata nodes? match name strings?)<br>
>><br>
>> This is what I meant... I realize it's not trivial, but I think it's<br>
>> the correct thing to do.<br>
>><br>
>> Essentially, transforms on globals themselves should be done as a<br>
>> module pass (much like globalopt is today a module pass), and<br>
>> transforms on the functions as a function pass.The global transforms<br>
>> are also much simpler / more constrained, and so it seems less likely<br>
>> for them to benefit from being a function pass.<br>
><br>
><br>
> Makes sense. (The downside, of course, is increased code complexity.<br>
> Hopefully not too much).<br>
><br>
><br>
>><br>
>><br>
>><br>
>> To address your specific points:<br>
>><br>
>> DynamicallyInitializedGlobals appears to only be created inside the<br>
>> finalization step, even though it is referenced from outside of it...<br>
>> I've no idea how the outside references can work currently.<br>
><br>
><br>
> hah! This is a performance bug in asan-initialization-order mode!<br>
> (The global transform phase was done in doInitialization until recently).<br>
> I'll fix this with a unit test this time ;)<br>
><br>
><br>
>><br>
>> That said,<br>
>> computing this twice (once in the initialization step of the function<br>
>> pass, once in the module pass) doesn't seem terribly costly.<br>
><br>
><br>
> Not too bad, indeed.<br>
><br>
>><br>
>><br>
>> GlobalsCreatedByAsan should indeed be handled in some other way. I'm<br>
>> hopeful that the new attribute system will be generalized to support<br>
>> globals as well as functions, which would allow us to just not mark<br>
>> the ASan globals as instrumented.<br>
><br>
><br>
> But that won't happen for at lest a couple of months, right?<br>
<br>
</div></div>The attribute stuff is going in presently, but still may not want to wait.<br>
<div class="im"><br>
><br>
>><br>
>> I'd also be fine using a particular<br>
>> naming convention, there are already some naming convention stuff done<br>
>> in ASan, so it wouldn't be too weird.<br>
><br>
><br>
> I will probably have to use names for now (e.g. __asan_gen_1234?)<br>
<br>
</div>FWIW, this seems fine...<br>
<div class="im"><br>
><br>
>><br>
>><br>
>> Are there any other bits that would be made particularly awkward by this<br>
>> split?<br>
><br>
><br>
> Nothing awkward, just quite a few duplicated lines.<br>
><br>
>><br>
>><br>
>><br>
>> Is this something you folks have bandwidth for? I can look at it if not.<br>
><br>
><br>
> Depends on what is the ETA. I'll do it, but don't promise to finish it<br>
> immediately.<br>
><br>
> This all is a bit sad.<br>
> Mainly because the behavior of doInitialization is a) not what I expected it<br>
> to be and b) not at all understood (by me).<br>
<br>
</div>One point -- doFinalization is the real problem here. I think ASan's<br>
doInitialization is fine as-is.<br></blockquote><div><br></div><div>I meant doFinalization. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
So what would you want the behavior of doFinalization to be? That is,<br>
what are you looking for in these routines? Just a way to share common<br>
state between a FunctionPass and a ModulePass?<br></blockquote><div><br></div><div>First of all, I want to understand how it works now :) </div><div>I expected that it is</div><div>  a) doInitialization </div><div>  b) for each f: runOnFunction(f)</div>
<div>  c) doFinalization</div><div>Apparently, there is something else in between (other functions passes are added to the mix).</div><div><br></div><div> </div><div>--kcc </div></div><br></div>