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

Tue May 6 09:57:51 PDT 2014

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