r162565 (the patch itself) and r162566 (file renaming)<div>Thanks! </div><div><br></div><div>--kcc <br><br><div class="gmail_quote">On Thu, Aug 23, 2012 at 9:26 PM, 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!  The reason for expanding the blacklist is somewhat unfortunate:<br>
<br>
Deploying initialization order checking on deployed code has found a<br>
surprising number of examples of undefined behavior.  Worse, fixing<br>
them brings up some unfortunate coding politics problems.<br>
<br>
The C++11 standard states that access to "any non-static member or<br>
base class of the object before the constructor begins results in<br>
undefined behavior" [12.7.1].  The initialization order checking tool<br>
recently patched in catches examples of this resulting from the<br>
unspecified order of initialization of variables in different TUs.<br>
<br>
However, some existing code exploits knowledge of the compiler/linker<br>
to avoid initialization order problems using code that contains<br>
undefined behavior.  For example, some classes have an explicit, one<br>
parameter constructor which is implicitly required not to change the<br>
object's memory.  This, together with zero initialization offers a<br>
means to "solve" the initialization order problem on objects that can<br>
be used if they are zero initialized.<br>
<br>
This would be a perfect use case for constexpr, but that's still<br>
relatively new, so expanding the blacklist is the simplest solution<br>
for now.  I completely agree that it would be better to just get rid<br>
of the undefined behavior.  Currently though, expanding the blacklist<br>
lets me:<br>
* Skip known UB without blacklisting a function which may actually be<br>
causing other problems when called on a different global.<br>
* Collect a list of all variables which have this issue in currently<br>
deployed code.<br>
<br>
All the best,<br>
--Reid<br>
<div class="HOEnZb"><div class="h5"><br>
On Thu, Aug 23, 2012 at 3:41 AM, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
> Looks good in general, I'll commit later unless someone objects.<br>
><br>
> Just curious, what is your use case?<br>
> We sometimes use the blacklists for asan/msan/tsan during initial deployment<br>
> in a particular source base,<br>
> but long term we always try to fix the code (and mark the tricky code with<br>
> attributes) and get rid of the blacklist.<br>
><br>
> Thanks!<br>
><br>
> --kcc<br>
><br>
><br>
> On Wed, Aug 22, 2012 at 11:00 PM, Reid Watson <<a href="mailto:reidw@google.com">reidw@google.com</a>> wrote:<br>
>><br>
>> Also, for ease of review, here's a version with no renaming of files<br>
>> involved.  It preserves the FunctionBlackList name, though that should<br>
>> probably be changed if this is committed.<br>
>><br>
>> On Wed, Aug 22, 2012 at 11:08 AM, Reid Watson <<a href="mailto:reidw@google.com">reidw@google.com</a>> wrote:<br>
>> > Hi all,<br>
>> ><br>
>> > First off, sincere thanks to reviewers(kcc especially) for helping<br>
>> > with the initial initialization order patch.<br>
>> ><br>
>> > I'm now looking to expand the blacklisting functionality of<br>
>> > AddressSanitizer to include the ability to blacklist global variables.<br>
>> >  This is currently handled in FunctionBlackList.h, which is renamed to<br>
>> > BlackList.h to reflect the fact that it can store more than functions.<br>
>> ><br>
>> > I've attached the patch and uploaded it to<br>
>> > Rietveld(<a href="http://codereview.appspot.com/6473048/" target="_blank">http://codereview.appspot.com/6473048/</a>).  Review is<br>
>> > appreciated as always.<br>
>> ><br>
>> > All the best,<br>
>> > Reid<br>
><br>
><br>
</div></div></blockquote></div><br></div>