<br><br><div class="gmail_quote">On Tue, Aug 21, 2012 at 11:36 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">
submitted the clang part: r162259.<div>Looking at the rest (there are minor problems, I may be able to fix them myself). <br></div></blockquote><div><br></div><div>LLVM part went as r<span style="background-color:rgb(255,255,255);color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px">162268 with minor changes: </span></div>
<div><span style="background-color:rgb(255,255,255);color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="background-color:rgb(255,255,255);color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px">1. Don't call </span><font color="#222222" face="arial, sans-serif">createInitializerPoisonCalls if there are no globals with dynamic init in the current TU. </font></div>
<div><font color="#222222" face="arial, sans-serif">2. Fix test instrument_initializer_metadata.ll to have correct metadata name </font></div><div><font color="#222222" face="arial, sans-serif">3. removed instrument_initializer.ll -- I did not understand why this test was needed. Please comment. </font></div>
<div><font color="#222222" face="arial, sans-serif"><br></font></div><div><font color="#222222" face="arial, sans-serif">Looking at the compiler-rt part, where I don't like some parts. </font></div><div><font color="#222222" face="arial, sans-serif"><br>
</font></div><div><font color="#222222" face="arial, sans-serif">--kcc </font></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>
<br></div><div>--kcc <div><div class="h5"><br><br><div class="gmail_quote">On Tue, Aug 21, 2012 at 12:52 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">OK, I re-reviewed what was contained in the patches.<br>
I had a few non-reviewed experiments accidentally included in the<br>
previous patches.<br>
I've attached the correct versions, containing only the properly<br>
reviewed changes.<br>
Sincere apologies for the trouble!<br>
<div><div><br>
On Mon, Aug 20, 2012 at 1:14 PM, Reid Watson <<a href="mailto:reidw@google.com" target="_blank">reidw@google.com</a>> wrote:<br>
> Sorry, minor problem with the compiler-rt patch.<br>
><br>
> Please commit the attached version of the compiler-rt patch instead of<br>
> the previous one.<br>
><br>
> On Mon, Aug 20, 2012 at 10:57 AM, Reid Watson <<a href="mailto:reidw@google.com" target="_blank">reidw@google.com</a>> wrote:<br>
>> I've incorporated all the review comments in the attached patches.<br>
>> The tool is off by default, and enabled by adding -mllvm<br>
>> -asan-initialization-order to clang.<br>
>><br>
>> I don't have commit access, so I'd appreciate it if someone could<br>
>> commit these for me.<br>
>><br>
>> All the best,<br>
>> Reid<br>
>><br>
>> On Thu, Aug 16, 2012 at 6:09 PM, Reid Watson <<a href="mailto:reidw@google.com" target="_blank">reidw@google.com</a>> wrote:<br>
>>> I'll add in the prefix before the final version for commit.<br>
>>><br>
>>> The goal is to detect globals with dynamic initialization per the C++<br>
>>> standard, so that ASan knows which globals can only be accessed during<br>
>>> . It's valid to access a statically initialized global inside of an<br>
>>> initializer, even if that global resides in a different TU. Thus,<br>
>>> LLVM/ASan need to be able to distinguish between<br>
>>> dynamically/statically initialized globals. I'm not certain this is<br>
>>> optimal, but there haven't been any unexpected false positives with<br>
>>> it. We may be missing a few case, and the optimizer can certainly end<br>
>>> up leaving us with false negatives, but those are much better than<br>
>>> false positives.<br>
>>><br>
>>> On Thu, Aug 16, 2012 at 5:27 PM, Eric Christopher <<a href="mailto:echristo@apple.com" target="_blank">echristo@apple.com</a>> wrote:<br>
>>>><br>
>>>><br>
>>>> On Aug 16, 2012, at 1:05 AM, Kostya Serebryany <<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>> wrote:<br>
>>>><br>
>>>>> +llvm-commits<br>
>>>>><br>
>>>>> Reid,<br>
>>>>><br>
>>>>> The LLVM and compiler-rt patches look good.<br>
>>>>> Please fix the remaining small issues (see my code review comments) and commit.<br>
>>>>> Hold on with the output tests for a bit since Alexey Samsonov is migrating them to cmake (please coordinate with him and commit as a separate patch).<br>
>>>>><br>
>>>>> The stress test should contain X files, Y linker initialized globals and Z dynamically initialized globals.<br>
>>>>> Such test only makes sense where all 3 numbers are large.<br>
>>>>> I guess you can commit a single .sh script into compiler-rt/lib/asan/scripts<br>
>>>><br>
>>>> The metadata should at least be prefixed with something like llvm.asan.<whatever> instead of just the name. That way it's more identifiable.<br>
>>>><br>
>>>> What's the idea behind the metadata use anyhow?<br>
>>>><br>
>>>> -eric<br>
</div></div></blockquote></div><br></div></div></div></div>
</blockquote></div><br>