<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, May 6, 2014 at 7:54 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">The important thing is that you don't break thread safety analysis,<br>
because the build cops will hate you, but the unit tests for that are<br>
pretty extensive. Don't worry about consumed analysis; I'll work<br>
around any changes to the CFG.<br></blockquote><div><br></div><div>I've not run into any problems with thread-safety analysis in my experiments so far - do you think it has a hidden corner-case dependency on how temporary destructors are modeled that we might run into later?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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 9:57 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> On Tue, May 6, 2014 at 5:25 PM, Delesley Hutchins <<a href="mailto:delesley@google.com">delesley@google.com</a>><br>
> wrote:<br>
>><br>
>> 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>
><br>
><br>
> Cool, I'll do that if it blocks us. For now, I wanted to make sure I<br>
> understand it correctly. This breaks with some in-progress patch I have to<br>
> fix temp dtor handling in the CFG, and I wanted to make sure I'm not<br>
> designing this patch into a direction that violates invariants we need to<br>
> have in the CFG. (My current patch unconditionally inserts a new block for<br>
> the temp dtor, which breaks existing tests in the consumed analysis).<br>
><br>
> Thanks for you help!<br>
> /Manuel<br>
><br>
>><br>
>><br>
>> Thanks!<br>
>><br>
>>   -DeLesley<br>
>><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>><br>
>> > wrote:<br>
>> >><br>
>> >> On Tue, May 6, 2014 at 12:56 AM, Delesley Hutchins<br>
>> >> <<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<br>
>> >> 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<br>
>> >> lost. I<br>
>> >> apparently jumped to the wrong conclusion (which is kinda obvious when<br>
>> >> I<br>
>> >> think about patterns like "{ Status s = g(); if (...) {} }", which of<br>
>> >> course<br>
>> >> works). Sorry about that.<br>
>> >><br>
>> >> Any insight in how this could be fixed (or where my mistake is) would<br>
>> >> 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<br>
>> > 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<br>
>> > the<br>
>> > dtor is called?).<br>
>> ><br>
>> > Just removing line 1439 (the clearTemporaries() call) "fixes" all<br>
>> > 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>><br>
>> >>> 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<br>
>> >>> >>> 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<br>
>> >>> >> 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<br>
>> >>> > 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"<br>
>> >>> >>> 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<br>
>> >>> >>> 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<br>
>> >>> >> have<br>
>> >>> >> terminators that just flow into a single other block (I'll try to<br>
>> >>> >> 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<br>
>> >>> >> 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<br>
>> >>> >> 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<br>
>> >>> >> 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<br>
>> >>> > just<br>
>> >>> > part<br>
>> >>> > of control flow.<br>
>> >>> ><br>
>> >>> > And I think I caused some confusion, as the invariant proposal I<br>
>> >>> > 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>
><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>