[cfe-dev] Consumed analysis resets its state in between CFG blocks

[Delesley Hutchins]

No.  Thread safety analysis generally ignores temporaries.  You can
trust the unit tests.  :-)

  -DeLesley

On Wed, May 7, 2014 at 1:12 AM, Manuel Klimek <klimek at google.com> wrote:
> On Tue, May 6, 2014 at 7:54 PM, Delesley Hutchins <delesley at google.com>
> wrote:
>>
>> The important thing is that you don't break thread safety analysis,
>> because the build cops will hate you, but the unit tests for that are
>> pretty extensive. Don't worry about consumed analysis; I'll work
>> around any changes to the CFG.
>
>
> 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?
>
>>
>>
>>   -DeLesley
>>
>>
>> On Tue, May 6, 2014 at 9:57 AM, Manuel Klimek <klimek at google.com> wrote:
>> > On Tue, May 6, 2014 at 5:25 PM, Delesley Hutchins <delesley at google.com>
>> > wrote:
>> >>
>> >> It doesn't surprise me that temporaries are creating a problem; that
>> >> code was always a bit flaky.  I think Chris assumed that a temporary
>> >> would always be local to a single block, so he wanted to kill them
>> >> eagerly as an optimization.  In general, having multiple CFG blocks
>> >> between the generation of a value and its destructor should not be an
>> >> issue, but temporaries are not handled like ordinary values.  That
>> >> should probably be changed.
>> >>
>> >> Let me take a look, and see what I can do.  What's your time frame on
>> >> this?  If this is blocking you, then just kill the clearTemporaries()
>> >> and put a FIXME comment next to it.  There's another memory leak and a
>> >> few other fixes to consumed analysis that I have to do anyway, so I'll
>> >> sit down and see if I can restore sanity to the code.
>> >
>> >
>> > 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).
>> >
>> > Thanks for you help!
>> > /Manuel
>> >
>> >>
>> >>
>> >> Thanks!
>> >>
>> >>   -DeLesley
>> >>
>> >>
>> >> On Tue, May 6, 2014 at 3:14 AM, Manuel Klimek <klimek at google.com>
>> >> wrote:
>> >> > /On Tue, May 6, 2014 at 10:01 AM, Manuel Klimek <klimek at google.com>
>> >> > wrote:
>> >> >>
>> >> >> On Tue, May 6, 2014 at 12:56 AM, Delesley Hutchins
>> >> >> <delesley at google.com>
>> >> >> wrote:
>> >> >>>
>> >> >>> Both thread safety analysis and consumed analysis just use the CFG;
>> >> >>> they are not part of the static analyzer, and they maintain their
>> >> >>> own
>> >> >>> notion of "state".  Moreover, the line you're looking at is not
>> >> >>> "killing state".  The initial state of a block is determined from
>> >> >>> the
>> >> >>> end state of its immediate predecessors, and the first step when
>> >> >>> starting to analyze a new block is to calculate the initial state
>> >> >>> for
>> >> >>> that block.  Blocks are traversed in topological order, so the
>> >> >>> previously processed block has no relationship to the one we're
>> >> >>> just
>> >> >>> starting to process.
>> >> >>>
>> >> >>> I don't understand what you mean by "brittle" here.  This algorithm
>> >> >>> works for any "layout" in which the CFG captures control flow,
>> >> >>> which
>> >> >>> is, after all, the whole point of the CFG.
>> >> >>
>> >> >>
>> >> >> It seems like it has problems with temporary destructors in the face
>> >> >> of
>> >> >> control flow:
>> >> >> 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:
>> >> >> Status g();
>> >> >> void f() {
>> >> >>   bool b = g();
>> >> >> }
>> >> >> Everything works as expected:
>> >> >> /tmp/t7.cc:39:12: warning: invalid invocation of method '~Status' on
>> >> >> a
>> >> >> temporary object while it is in the 'unconsumed' state
>> >> >>   bool b = g();
>> >> >>
>> >> >> Now if I change the code to:
>> >> >> Status g();
>> >> >> bool coin();
>> >> >> void f() {
>> >> >>   bool b = coin() && g();
>> >> >> }
>> >> >> I don't get any errors, but did not change the consumed state of the
>> >> >> returned temporary, if I'm not mistaken.
>> >> >>
>> >> >> 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.
>> >> >>
>> >> >> 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.
>> >> >>
>> >> >> Any insight in how this could be fixed (or where my mistake is)
>> >> >> would
>> >> >> be
>> >> >> highly appreciated.
>> >> >
>> >> >
>> >> > 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?
>> >> > 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?).
>> >> >
>> >> > Just removing line 1439 (the clearTemporaries() call) "fixes" all
>> >> > problems I
>> >> > can identify, but I assume it's not the right solution :)
>> >> >
>> >> > Thanks,
>> >> > /Manuel
>> >> >
>> >> >>
>> >> >>
>> >> >> Cheers,
>> >> >> /Manuel
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>>   -DeLesley
>> >> >>>
>> >> >>>
>> >> >>> On Mon, May 5, 2014 at 10:38 AM, Manuel Klimek <klimek at google.com>
>> >> >>> wrote:
>> >> >>> > On Mon, May 5, 2014 at 7:29 PM, Jordan Rose
>> >> >>> > <jordan_rose at apple.com>
>> >> >>> > wrote:
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> On May 5, 2014, at 10:27 , Manuel Klimek <klimek at google.com>
>> >> >>> >> wrote:
>> >> >>> >>
>> >> >>> >> On Mon, May 5, 2014 at 6:49 PM, Jordan Rose
>> >> >>> >> <jordan_rose at apple.com>
>> >> >>> >> wrote:
>> >> >>> >>>
>> >> >>> >>> Neither of these are part of the analyzer; they're part of the
>> >> >>> >>> analysis-based warnings in Clang. libClangAnalysis is the
>> >> >>> >>> common
>> >> >>> >>> base
>> >> >>> >>> between the two, which is why we have to be careful about the
>> >> >>> >>> CFG,
>> >> >>> >>> but the
>> >> >>> >>> full analyzer infrastructure isn't going to be the answer.
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> What would be "full analyzer infrastructure" here? The
>> >> >>> >> REGISTER_MAP_WITH_PROGRAMSTATE part?
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> ProgramState at all. The analysis-based warnings track their own
>> >> >>> >> state
>> >> >>> >> in
>> >> >>> >> more lightweight structures, and aren't usually following paths
>> >> >>> >> anyway.
>> >> >>> >
>> >> >>> >
>> >> >>> > The consumed analysis seems to have to? Otherwise, how would it
>> >> >>> > figure
>> >> >>> > out
>> >> >>> > that something was not consumed in all paths?
>> >> >>> >
>> >> >>> >>> "A new block may only be started if there is control flow"
>> >> >>> >>> probably
>> >> >>> >>> is
>> >> >>> >>> not enforced, but I'm not sure why we wouldn't do that.
>> >> >>> >>> ("control
>> >> >>> >>> flow"
>> >> >>> >>> doesn't have to mean a C/C++ notion of control flow; it can be
>> >> >>> >>> something
>> >> >>> >>> like the static variable DeclStmt that clients of the CFG can
>> >> >>> >>> use
>> >> >>> >>> to
>> >> >>> >>> decide
>> >> >>> >>> where execution goes next.)
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> I'm aware control flow doesn't necessarily mean C/C++ control
>> >> >>> >> flow.
>> >> >>> >> Here's my reasons why we might not want to have that invariant.
>> >> >>> >> 1. if I remember correctly I have already seen blocks that don't
>> >> >>> >> have
>> >> >>> >> terminators that just flow into a single other block (I'll try
>> >> >>> >> to
>> >> >>> >> fish
>> >> >>> >> one
>> >> >>> >> out if you're curious); so I was surprised to see other places
>> >> >>> >> basically
>> >> >>> >> assert in a comment that this is an assumed invariant
>> >> >>> >> 2. it seems that sometimes having the CFG be more uniform
>> >> >>> >> regarding
>> >> >>> >> different related patterns might be nice (although I see that it
>> >> >>> >> is
>> >> >>> >> a
>> >> >>> >> very
>> >> >>> >> very weak argument)
>> >> >>> >> 3. it might just be too hard to assert the invariant, in which
>> >> >>> >> case
>> >> >>> >> I'd
>> >> >>> >> say it's better to explicitly say it's *not* an invariant than
>> >> >>> >> to
>> >> >>> >> let
>> >> >>> >> checks
>> >> >>> >> or cfg-based compiler checks rely on it
>> >> >>> >>
>> >> >>> >>
>> >> >>> >> I guess it's not. After an if-statement, both branches have to
>> >> >>> >> transition
>> >> >>> >> to the same block, of course. I'm not sure what I was thinking,
>> >> >>> >> nor
>> >> >>> >> am
>> >> >>> >> I
>> >> >>> >> sure what the comment was getting at.
>> >> >>> >
>> >> >>> >
>> >> >>> > Well, I thought "incoming" control flow (at the end of an if) is
>> >> >>> > just
>> >> >>> > part
>> >> >>> > of control flow.
>> >> >>> >
>> >> >>> > And I think I caused some confusion, as the invariant proposal I
>> >> >>> > made
>> >> >>> > was
>> >> >>> > not actually a citation of the code (but might have looked like I
>> >> >>> > implied
>> >> >>> > that).
>> >> >>> > The comment is:
>> >> >>> > // Note also that we are restricting
>> >> >>> > // to looking at return statements in the same CFGBlock,
>> >> >>> > // so this will intentionally not catch cases where the
>> >> >>> > // return statement contains nested control-flow.
>> >> >>> >
>> >> >>> > Which seems to imply that invariant, though?
>> >> >>> >
>> >> >>> > Cheers,
>> >> >>> > /Manuel
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>> --
>> >> >>> DeLesley Hutchins | Software Engineer | delesley at google.com |
>> >> >>> 505-206-0315
>> >> >>
>> >> >>
>> >> >
>> >>
>> >>
>> >>
>> >> --
>> >> DeLesley Hutchins | Software Engineer | delesley at google.com |
>> >> 505-206-0315
>> >
>> >
>>
>>
>>
>> --
>> DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
>
>



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315