<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 6, 2014 at 5:25 PM, Delesley Hutchins <span dir="ltr"><<a href="mailto:delesley@google.com" target="_blank">delesley@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It doesn't surprise me that temporaries are creating a problem; that<br>
code was always a bit flaky.  I think Chris assumed that a temporary<br>
would always be local to a single block, so he wanted to kill them<br>
eagerly as an optimization.  In general, having multiple CFG blocks<br>
between the generation of a value and its destructor should not be an<br>
issue, but temporaries are not handled like ordinary values.  That<br>
should probably be changed.<br>
<br>
Let me take a look, and see what I can do.  What's your time frame on<br>
this?  If this is blocking you, then just kill the clearTemporaries()<br>
and put a FIXME comment next to it.  There's another memory leak and a<br>
few other fixes to consumed analysis that I have to do anyway, so I'll<br>
sit down and see if I can restore sanity to the code.<br></blockquote><div><br></div><div>Cool, I'll do that if it blocks us. For now, I wanted to make sure I understand it correctly. This breaks with some in-progress patch I have to fix temp dtor handling in the CFG, and I wanted to make sure I'm not designing this patch into a direction that violates invariants we need to have in the CFG. (My current patch unconditionally inserts a new block for the temp dtor, which breaks existing tests in the consumed analysis).</div>
<div><br></div><div>Thanks for you help!</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
  -DeLesley<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On Tue, May 6, 2014 at 3:14 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> /On Tue, May 6, 2014 at 10:01 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>><br>
>> On Tue, May 6, 2014 at 12:56 AM, Delesley Hutchins <<a href="mailto:delesley@google.com">delesley@google.com</a>><br>
>> wrote:<br>
>>><br>
>>> Both thread safety analysis and consumed analysis just use the CFG;<br>
>>> they are not part of the static analyzer, and they maintain their own<br>
>>> notion of "state".  Moreover, the line you're looking at is not<br>
>>> "killing state".  The initial state of a block is determined from the<br>
>>> end state of its immediate predecessors, and the first step when<br>
>>> starting to analyze a new block is to calculate the initial state for<br>
>>> that block.  Blocks are traversed in topological order, so the<br>
>>> previously processed block has no relationship to the one we're just<br>
>>> starting to process.<br>
>>><br>
>>> I don't understand what you mean by "brittle" here.  This algorithm<br>
>>> works for any "layout" in which the CFG captures control flow, which<br>
>>> is, after all, the whole point of the CFG.<br>
>><br>
>><br>
>> It seems like it has problems with temporary destructors in the face of<br>
>> control flow:<br>
>> If 'Status' is the class taken from<br>
>> test/SemaCXX/warn-consumed-analysis.cpp, but I add an "operator bool()<br>
>> const;" to it (which is not annotated in any way), the following happens:<br>
>> Status g();<br>
>> void f() {<br>
>>   bool b = g();<br>
>> }<br>
>> Everything works as expected:<br>
>> /tmp/t7.cc:39:12: warning: invalid invocation of method '~Status' on a<br>
>> temporary object while it is in the 'unconsumed' state<br>
>>   bool b = g();<br>
>><br>
>> Now if I change the code to:<br>
>> Status g();<br>
>> bool coin();<br>
>> void f() {<br>
>>   bool b = coin() && g();<br>
>> }<br>
>> I don't get any errors, but did not change the consumed state of the<br>
>> returned temporary, if I'm not mistaken.<br>
>><br>
>> The main difference is that with temporary destructors, we have control<br>
>> flow, and thus multiple CFG blocks in between the generation of the<br>
>> temporary and the destructor call.<br>
>><br>
>> I debugged into this and found the Visitor.reset(CurrStates) to be the<br>
>> point where the information that the status is not consumed yet is lost. I<br>
>> apparently jumped to the wrong conclusion (which is kinda obvious when I<br>
>> think about patterns like "{ Status s = g(); if (...) {} }", which of course<br>
>> works). Sorry about that.<br>
>><br>
>> Any insight in how this could be fixed (or where my mistake is) would be<br>
>> highly appreciated.<br>
><br>
><br>
> Ok, so the problem seems to be the unconditional call to<br>
> CurrStates->clearTemporaries(); in line 1439. Could we instead remove the<br>
> temps from the map just above that call when we visit the TemporaryDtor?<br>
> Also, where are other elements removed? ConsumedStateMap::remove doesn't<br>
> seem to have any references (I'd guess we can safely remove stuff when the<br>
> dtor is called?).<br>
><br>
> Just removing line 1439 (the clearTemporaries() call) "fixes" all problems I<br>
> can identify, but I assume it's not the right solution :)<br>
><br>
> Thanks,<br>
> /Manuel<br>
><br>
>><br>
>><br>
>> Cheers,<br>
>> /Manuel<br>
>><br>
>><br>
>><br>
>><br>
>>><br>
>>><br>
>>>   -DeLesley<br>
>>><br>
>>><br>
>>> On Mon, May 5, 2014 at 10:38 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>>> > On Mon, May 5, 2014 at 7:29 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>><br>
>>> > wrote:<br>
>>> >><br>
>>> >><br>
>>> >> On May 5, 2014, at 10:27 , Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>>> >><br>
>>> >> On Mon, May 5, 2014 at 6:49 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>><br>
>>> >> wrote:<br>
>>> >>><br>
>>> >>> Neither of these are part of the analyzer; they're part of the<br>
>>> >>> analysis-based warnings in Clang. libClangAnalysis is the common base<br>
>>> >>> between the two, which is why we have to be careful about the CFG,<br>
>>> >>> but the<br>
>>> >>> full analyzer infrastructure isn't going to be the answer.<br>
>>> >><br>
>>> >><br>
>>> >> What would be "full analyzer infrastructure" here? The<br>
>>> >> REGISTER_MAP_WITH_PROGRAMSTATE part?<br>
>>> >><br>
>>> >><br>
>>> >> ProgramState at all. The analysis-based warnings track their own state<br>
>>> >> in<br>
>>> >> more lightweight structures, and aren't usually following paths<br>
>>> >> anyway.<br>
>>> ><br>
>>> ><br>
>>> > The consumed analysis seems to have to? Otherwise, how would it figure<br>
>>> > out<br>
>>> > that something was not consumed in all paths?<br>
>>> ><br>
>>> >>> "A new block may only be started if there is control flow" probably<br>
>>> >>> is<br>
>>> >>> not enforced, but I'm not sure why we wouldn't do that. ("control<br>
>>> >>> flow"<br>
>>> >>> doesn't have to mean a C/C++ notion of control flow; it can be<br>
>>> >>> something<br>
>>> >>> like the static variable DeclStmt that clients of the CFG can use to<br>
>>> >>> decide<br>
>>> >>> where execution goes next.)<br>
>>> >><br>
>>> >><br>
>>> >> I'm aware control flow doesn't necessarily mean C/C++ control flow.<br>
>>> >> Here's my reasons why we might not want to have that invariant.<br>
>>> >> 1. if I remember correctly I have already seen blocks that don't have<br>
>>> >> terminators that just flow into a single other block (I'll try to fish<br>
>>> >> one<br>
>>> >> out if you're curious); so I was surprised to see other places<br>
>>> >> basically<br>
>>> >> assert in a comment that this is an assumed invariant<br>
>>> >> 2. it seems that sometimes having the CFG be more uniform regarding<br>
>>> >> different related patterns might be nice (although I see that it is a<br>
>>> >> very<br>
>>> >> very weak argument)<br>
>>> >> 3. it might just be too hard to assert the invariant, in which case<br>
>>> >> I'd<br>
>>> >> say it's better to explicitly say it's *not* an invariant than to let<br>
>>> >> checks<br>
>>> >> or cfg-based compiler checks rely on it<br>
>>> >><br>
>>> >><br>
>>> >> I guess it's not. After an if-statement, both branches have to<br>
>>> >> transition<br>
>>> >> to the same block, of course. I'm not sure what I was thinking, nor am<br>
>>> >> I<br>
>>> >> sure what the comment was getting at.<br>
>>> ><br>
>>> ><br>
>>> > Well, I thought "incoming" control flow (at the end of an if) is just<br>
>>> > part<br>
>>> > of control flow.<br>
>>> ><br>
>>> > And I think I caused some confusion, as the invariant proposal I made<br>
>>> > was<br>
>>> > not actually a citation of the code (but might have looked like I<br>
>>> > implied<br>
>>> > that).<br>
>>> > The comment is:<br>
>>> > // Note also that we are restricting<br>
>>> > // to looking at return statements in the same CFGBlock,<br>
>>> > // so this will intentionally not catch cases where the<br>
>>> > // return statement contains nested control-flow.<br>
>>> ><br>
>>> > Which seems to imply that invariant, though?<br>
>>> ><br>
>>> > Cheers,<br>
>>> > /Manuel<br>
>>><br>
>>><br>
>>><br>
>>> --<br>
>>> DeLesley Hutchins | Software Engineer | <a href="mailto:delesley@google.com">delesley@google.com</a> |<br>
>>> <a href="tel:505-206-0315" value="+15052060315">505-206-0315</a><br>
>><br>
>><br>
><br>
<br>
<br>
<br>
--<br>
DeLesley Hutchins | Software Engineer | <a href="mailto:delesley@google.com">delesley@google.com</a> | <a href="tel:505-206-0315" value="+15052060315">505-206-0315</a><br>
</div></div></blockquote></div><br></div></div>