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

Tue May 6 10:54:27 PDT 2014

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.

  -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