<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">On Wed, Nov 14, 2012 at 8:14 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br>
<div class="gmail_quote"><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 Wed, Nov 14, 2012 at 7:37 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
> On Wed, Nov 14, 2012 at 7:17 PM, John McCall <<a href="mailto:rjmccall@apple.com">rjmccall@apple.com</a>> wrote:<br>
>><br>
>> On Nov 14, 2012, at 5:04 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>><br>
>> On Mon, Aug 27, 2012 at 4:18 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>><br>
>>> On Tue, Jun 19, 2012 at 2:49 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>>><br>
>>>> On Fri, Jun 8, 2012 at 6:41 PM, John McCall <<a href="mailto:rjmccall@apple.com">rjmccall@apple.com</a>> wrote:<br>
>>>>><br>
>>>>> On Jun 8, 2012, at 5:47 PM, Richard Trieu wrote:<br>
>>>>><br>
>>>>> On Fri, Jun 8, 2012 at 4:29 PM, John McCall <<a href="mailto:rjmccall@apple.com">rjmccall@apple.com</a>> wrote:<br>
>>>>>><br>
>>>>>> On Jun 8, 2012, at 4:24 PM, Richard Trieu wrote:<br>
>>>>>> > Add a bit to VarDecl that is set to true when its address is taken<br>
>>>>>> > or it gets passed by non-const reference.<br>
>>>>>><br>
>>>>>> Just out of curiosity, is this bit set for variables of class type<br>
>>>>>> that are used as an operand of a trivial copy constructor or copy assignment<br>
>>>>>> operator?<br>
>>>>>><br>
>>>>>> John.<br>
>>>>><br>
>>>>><br>
>>>>> No, the bit is not currently set for those uses.<br>
>>>>><br>
>>>>><br>
>>>>> Then please document the desired semantics more accurately, like "the<br>
>>>>> address is taken or bound to a reference other than either operand of a<br>
>>>>> trivial copy operation" or whatever semantics we actually settle on.<br>
>>>>><br>
>>>>> Please document that this is only accurate when all possible accesses<br>
>>>>> have been seen. For example, it is only accurate for local variables after<br>
>>>>> their scope ends, and it is only accurate for global variables at the end of<br>
>>>>> the translation unit and only if they have internal linkage. It is also<br>
>>>>> basically meaningless within templates.<br>
>>>>><br>
>>>>> Please document that this is only meant to be a conservative<br>
>>>>> approximation of whether the address is used; it may be true in situations<br>
>>>>> when the address is actually statically provable to not escape. Alas, your<br>
>>>>> patch does not implement a conservative approximation. Among other things,<br>
>>>>> you're only setting this bit in direct uses, but you really do need to be<br>
>>>>> looking through anything that propagates reference-ness, including member<br>
>>>>> accesses, base/derived casts, l-value reinterpreting casts, comma operators,<br>
>>>>> and conditional operators; I do not claim that this list is exhaustive.<br>
>>>>> Other things which take references to objects are lambda by-reference<br>
>>>>> captures and member calls on the object; this, too, I do not claim to be<br>
>>>>> exhaustive.<br>
>>>>><br>
>>>>> That leads into the very interesting question of how we should validate<br>
>>>>> the correctness of this bit. We really can't rely on it until we have some<br>
>>>>> sort of validation story.<br>
>>>>><br>
>>>>> Arguably, this bit should never be set on a variable of reference type.<br>
>>>>><br>
>>>>> + /// \brief Whether this variable has its address taken or is<br>
>>>>> referenced.<br>
>>>>> + unsigned HasReference : 1;<br>
>>>>><br>
>>>>> + /// Whether this variable has its address taken or is referenced.<br>
>>>>> + bool hasReference() const { return VarDeclBits.HasReference; }<br>
>>>>> + void setReference(bool ref) { VarDeclBits.HasReference = ref; }<br>
>>>>><br>
>>>>> This is a poor choice of names; it is confusable with both the concept<br>
>>>>> of a variable of reference type and the concept of whether a declaration has<br>
>>>>> been referenced in the translation unit. I would go with isAddressTaken /<br>
>>>>> setAddressTaken, with the C++ behavior with references being understood (and<br>
>>>>> documented).<br>
>>>>><br>
>>>>> Also, the semantics you're actually tracking are quite a bit more<br>
>>>>> subtle than whether the address it taken, because you're specifically<br>
>>>>> ignoring initializations of const references. That's quite suspect and<br>
>>>>> quite a bit less generically useful. Also note that it is not illegal to<br>
>>>>> bind a const reference to something and then modify it through that<br>
>>>>> reference, either due to a mutable field or due to the constness being cast<br>
>>>>> away.<br>
>>>>><br>
>>>>> setAddressTaken can default its argument to true.<br>
>>>>><br>
>>>>> You are not initializing this field to false.<br>
>>>>><br>
>>>>> You are not serializing and deserialization this field. The<br>
>>>>> ParmVarDecl abbreviation should assume this bit is 'false', I guess.<br>
>>>>><br>
>>>>> --- lib/Sema/SemaInit.cpp (revision 158223)<br>
>>>>> +++ lib/Sema/SemaInit.cpp (working copy)<br>
>>>>> @@ -6211,6 +6211,12 @@<br>
>>>>> Expr *InitE = Init.get();<br>
>>>>> assert(InitE && "No initialization expression?");<br>
>>>>><br>
>>>>> + QualType QT = Entity.getType();<br>
>>>>> + if (QT->isReferenceType() &&<br>
>>>>> !QT.getNonReferenceType().isConstQualified())<br>
>>>>> + if (DeclRefExpr *DRE =<br>
>>>>> dyn_cast<DeclRefExpr>(InitE->IgnoreParens()))<br>
>>>>> + if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()))<br>
>>>>> + VD->setReference(true);<br>
>>>>><br>
>>>>> This is not an appropriate place for this check; not everything goes<br>
>>>>> through PerformCopyInitialization. This needs to happen in<br>
>>>>> InitializationSequence::Perform for a reference-binding step.<br>
>>>>><br>
>>>>> + if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(op))<br>
>>>>> + if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()))<br>
>>>>> + VD->setReference(true);<br>
>>>>><br>
>>>>> You're not even looking through dyn_casts here. You should extract out<br>
>>>>> a function which walks an expression and marks variables that are bound in<br>
>>>>> this way. Alternatively, you may be able to piggy-back on the C++11 ODR-use<br>
>>>>> code.<br>
>>>>><br>
>>>>> John.<br>
>>>><br>
>>>><br>
>>>> Another shot at adding an AddressTaken bit. A warning was created to<br>
>>>> show which part of the code construct was responsible for setting the<br>
>>>> AddressTaken bit. At the point where a reference (const or not) is bound,<br>
>>>> or an address is taken, a recursive function is called that will traverse<br>
>>>> the expression and set this bit for matching VarDecls. Also, the<br>
>>>> ASTReaderDecl and ASTWriterDecl have been updated for this bit.<br>
>>>><br>
>>>> Currently, the analysis doesn't go into class method calls and is one of<br>
>>>> the unhandled ways for the address to leak. Classes weren't important to my<br>
>>>> original use of this bit, but I'm open to suggestions on how to handle them.<br>
>>><br>
>>><br>
>>> Ping?<br>
>><br>
>><br>
>> Ping again.<br>
>><br>
>><br>
>> I still don't know what this bit is actually trying to record.<br>
>><br>
>> John.<br>
><br>
><br>
> The bit specifies if the variable is accessible from somewhere else in the<br>
> program. The two ways to determine this are either the address of the<br>
> variable is taken or there is a reference to the variable.<br>
><br>
> This bit will be used to augment -Wloop-analysis to check while loops.<br>
> There is a common pattern with threaded code that currently gives a false<br>
> positive.<br>
><br>
> bool done = false;<br>
> StartWork(&done);<br>
> while (!done)<br>
> ;<br>
<br>
</div></div>That can't be the actual pattern; LLVM (and I assume gcc) will<br>
optimize this to an infinite loop.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Eli<br>
</font></span></blockquote></div><br><div>Normally, there would be a sleep statement or some other work in the loop. The point is that the loop could be interrupted by StartWork() changing the value of done, thus not an infinite loop.</div>
</div>