<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">/On Tue, May 6, 2014 at 10:01 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div class="">On Tue, May 6, 2014 at 12:56 AM, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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></blockquote><div><br></div></div><div>It seems like it has problems with temporary destructors in the face of control flow:</div><div>If 'Status' is the class taken from test/SemaCXX/warn-consumed-analysis.cpp, but I add an "operator bool() const;" to it (which is not annotated in any way), the following happens:</div>

<div>Status g();</div><div>void f() {</div><div>  bool b = g();</div><div>}</div><div>Everything works as expected:</div><div><div>/tmp/t7.cc:39:12: warning: invalid invocation of method '~Status' on a temporary object while it is in the 'unconsumed' state</div>

<div>  bool b = g();</div></div><div><br></div><div>Now if I change the code to:</div><div>Status g();</div><div>bool coin();</div><div>void f() {</div><div>  bool b = coin() && g();</div><div>}</div><div>I don't get any errors, but did not change the consumed state of the returned temporary, if I'm not mistaken.</div>

<div><br></div><div>The main difference is that with temporary destructors, we have control flow, and thus multiple CFG blocks in between the generation of the temporary and the destructor call.</div><div><br></div><div>
I debugged into this and found the Visitor.reset(CurrStates) to be the point where the information that the status is not consumed yet is lost. I apparently jumped to the wrong conclusion (which is kinda obvious when I think about patterns like "{ Status s = g(); if (...) {} }", which of course works). Sorry about that.</div>

<div><br></div><div>Any insight in how this could be fixed (or where my mistake is) would be highly appreciated.</div></div></div></div></blockquote><div><br></div><div>Ok, so the problem seems to be the unconditional call to CurrStates->clearTemporaries(); in line 1439. Could we instead remove the temps from the map just above that call when we visit the TemporaryDtor?</div>
<div>Also, where are other elements removed? ConsumedStateMap::remove doesn't seem to have any references (I'd guess we can safely remove stuff when the dtor is called?).</div><div><br></div><div>Just removing line 1439 (the clearTemporaries() call) "fixes" all problems I can identify, but I assume it's not the right solution :)</div>
<div><br></div><div>Thanks,</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Cheers,</div><div>/Manuel</div><div><div class="h5"><div><br></div><div><br></div><div><br></div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
  -DeLesley<br>
<div><div><br>
<br>
On Mon, May 5, 2014 at 10:38 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
> On Mon, May 5, 2014 at 7:29 PM, Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:<br>
>><br>
>><br>
>> On May 5, 2014, at 10:27 , Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">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" target="_blank">jordan_rose@apple.com</a>> 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, 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 in<br>
>> more lightweight structures, and aren't usually following paths anyway.<br>
><br>
><br>
> The consumed analysis seems to have to? Otherwise, how would it figure 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 is<br>
>>> not enforced, but I'm not sure why we wouldn't do that. ("control flow"<br>
>>> doesn't have to mean a C/C++ notion of control flow; it can be something<br>
>>> like the static variable DeclStmt that clients of the CFG can use to 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 one<br>
>> out if you're curious); so I was surprised to see other places 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 very<br>
>> very weak argument)<br>
>> 3. it might just be too hard to assert the invariant, in which case I'd<br>
>> say it's better to explicitly say it's *not* an invariant than to let 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 transition<br>
>> to the same block, of course. I'm not sure what I was thinking, nor am 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 part<br>
> of control flow.<br>
><br>
> And I think I caused some confusion, as the invariant proposal I made was<br>
> not actually a citation of the code (but might have looked like I 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>
</div></div><span><font color="#888888">--<br>
DeLesley Hutchins | Software Engineer | <a href="mailto:delesley@google.com" target="_blank">delesley@google.com</a> | <a href="tel:505-206-0315" value="+15052060315" target="_blank">505-206-0315</a><br>
</font></span></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>