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

Tue May 6 03:14:28 PDT 2014

/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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140506/0045c58a/attachment.html>