[cfe-dev] [PATCH] Clang Static Analyzer support for temporary destructors

[Alex McCarthy]

Thanks for your advice, Jordan.

I've split the NORETURN printing changes to CFG.cpp and the new test
additions into a separate patch (attached). Hopefully this should be a safe
no-op from a behavior perspective: debug output changes slightly, and the
new test cases more accurately reflects current clang behavior. Does this
look safe to commit?

I didn't have time to get through the rest of your comments, but I'll dive
in again soon and get back to you with another patch.



-Alex


On Tue, Mar 25, 2014 at 7:16 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> On Mon, Mar 24, 2014 at 11:29 AM, Alex McCarthy <alexmc at google.com> wrote:
>
>> That's very helpful, thanks Jordan!
>>
>> Pavel's attempt to fix the liveliness problems was
>> http://llvm-reviews.chandlerc.com/rL189090, correct? I'll give that a
>> more thorough read.
>>
>> I take it that from the "sometimes-y-ness" of the bug, we don't have any
>> better regression test coverage than what's already in temporaries.cpp? In
>> particular, we don't have any test cases covering the looping misbehavior
>> you described?
>>
>
> We don't use the LiveVariables analysis for anything but persisting
> analyzer values, and I don't think there are any problematic cases other
> than the nested logical operators, which means there aren't really
> observable test cases with the trunk behavior. The two test cases we do
> have are test/Analysis/live-variables.m and .cpp (really just two versions
> of the same test case).
>
>
>
>> Would you mind speeding up my search by pointing me at 1) the IRGen code
>> that does this sort of variable synthesis 2) similar code in the clang
>> analyzer that does any sort of variable synthesis? (I think Ted said we do
>> something similar to track which objects have been constructed so we know
>> which destructors need to be called)
>>
>
> That's what we need to implement. The place we already do this sort of
> thing is for static variables, which you can find in CFG.cpp controlled by
> the "AddStaticInitBranches" build option, in CoreEngine::HandleBlockExit,
> and in ExprEngine::processStaticInitializer. I'm not sure we'd do the exact
> same thing here—someone needs to figure out what design makes sense.
>
> I'm not so familiar with IRGen, but you can see it doing the same thing in
> CGCleanup.cpp's SetupCleanupBlockActivation, PopCleanupBlock, and
> EmitCleanup (amid a ton of other logic).
>
>  On Mon, Mar 24, 2014 at 10:15 AM, Manuel Klimek <klimek at google.com>wrote:
>>
>>> On Mon, Mar 24, 2014 at 6:09 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>>>
>>>>
>>>> A while ago Ted mentioned that we should just synthesize shadow bool
>>>> variables of some kind to track whether or not something's been
>>>> initialized. I'm starting to think more and more that that's the right
>>>> idea. (It's basically what IRGen does.) We don't exactly have an
>>>> implementation for it, though.
>>>>
>>>
>>> If I remember correctly that was also Chandler's favorite opinion from
>>> the start (just do whatever IRGen does).
>>>
>>>
>>
> Oops. Credit to Chandler for this!
>
>
> And one more question: what do you think that the impact would be of
> submitting this change without fixing the logical operator liveliness
> problem? Would that only cause noreturn destructor-based results to be
> sometimes broken (better than the current always broken behavior), or do
> you think this change is likely to introduce regressions in other areas?
>
>
> As I see it, this would go from destructors of temporary objects being
> executed "never" to being exceuted "always, except when they participate in
> logical expressions". That may seem strictly better, but I really don't
> like the unpredictability aspect of that.
>
> Patch comments:
>
> Index: cfe/lib/Analysis/CFG.cpp
> ===================================================================
> --- cfe/lib/Analysis/CFG.cpp
> +++ cfe/lib/Analysis/CFG.cpp
> @@ -3899,6 +3899,8 @@
>      OS << " (EXIT)]\n";
>    else if (&B == cfg->getIndirectGotoBlock())
>      OS << " (INDIRECT GOTO DISPATCH)]\n";
> +  else if (B.hasNoReturnElement())
> +    OS << " (NORETURN)]\n";
>    else
>      OS << "]\n";
>
>
> This is fine to commit.
>
>    return getBooleanOption(IncludeTemporaryDtorsInCFG,
>                            "cfg-temporary-dtors",
> -                          /* Default = */ false);
> +                          /* Default = */ true);
>
>
> Not yet. ;-)
>
> Index: cfe/lib/StaticAnalyzer/Core/CallEvent.cpp
> ===================================================================
> --- cfe/lib/StaticAnalyzer/Core/CallEvent.cpp
> +++ cfe/lib/StaticAnalyzer/Core/CallEvent.cpp
> @@ -955,7 +955,6 @@
>    CFGElement E = (*B)[CalleeCtx->getIndex()];
>    assert(E.getAs<CFGImplicitDtor>() &&
>           "All other CFG elements should have exprs");
> -  assert(!E.getAs<CFGTemporaryDtor>() && "We don't handle temporaries
> yet");
>
>
> I'm concerned about removing this without actually doing some work to
> identify the triggering expression. This is used to place path notes, and
> not providing a better location here seems...suboptimal. Is there any way
> for us to recover the full-expression that included this destructor call?
>
> Something to think about, but I guess it unblocks your work to commit
> this, so okay.
>
> -  assert(I != E);
> +  // We might not have found a matching block if we're handling a
> temporary
> +  // destructor.
>
>
> This is the problem we have to solve to get destructors in logical
> expressions to work. I'm surprised that removing this assertion works; the
> next thing you'll hit here is an llvm_unreachable.
>
> +static const MemRegion *getRegionForConstructedObject(const
> CXXConstructExpr *CE,
> +                                                ExplodedNode *Pred,
> +                                                ExprEngine *ExprEngine,
> +                                                unsigned int CurrStmtIdx)
> {
>
>
> This is a nice refactoring anyway, even without the destructor change. You
> could do that in a separate commit, just to make it clearer what's changing
> afterwards.
>
> Also, watch your 80-column limit, and don't indent parameters all the way
> to the end if they don't all line up. Whatever clang-format does is
> probably good here, but if you're doing it manually you can either break
> after the return type or double-indent.
>
> Double-also, please don't shadow type names. We use "ExprEngine &Eng"
> enough in the analyzer that that's probably the best choice (including the
> reference type).
>
> +    // Is this a destructor? If so, we might be in the middle of an
> assignment
> +    // to a local or member: look ahead one more element to see what we
> find.
> +    if (Optional<CFGImplicitDtor> DtorElem =
> Next.getAs<CFGImplicitDtor>()) {
> +      return getRegionForConstructedObject(CE, Pred, ExprEngine,
> CurrStmtIdx + 1);
> +    }
>
>
> Does it make sense to do this first, inline, rather than doing a recursive
> call?
>
> +  // If this is an array, find a RecordDecl in the array so we know which
> +  // deconstructor to call. As described below, this currently just runs
> the
> +  // first destructor.
> +  if (ObjectType->isArrayType()) {
> +    SValBuilder &SVB = State->getStateManager().getSValBuilder();
> +    ASTContext &Ctx = SVB.getContext();
>
> +
> +    while (const ArrayType *ArrayType = Ctx.getAsArrayType(ObjectType)) {
> +      ObjectType = ArrayType->getElementType();
> +    }
> +  }
>
>
> makeZeroElementRegion already does this, and in fact it relies on you
> *not* doing it ahead of time. Perhaps its interface should be changed to
> make this more clear. (Sorry for being unclear last time this came up.)
>
> Index: cfe/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
> ===================================================================
> --- cfe/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
> +++ cfe/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
> @@ -571,7 +571,10 @@
>        return PathDiagnosticLocation::createEnd(CallerBody, SM, CallerCtx);
>      return PathDiagnosticLocation::create(CallerInfo->getDecl(), SM);
>    }
> -  case CFGElement::TemporaryDtor:
> +  case CFGElement::TemporaryDtor: {
> +    const CFGTemporaryDtor &Dtor = Source.castAs<CFGTemporaryDtor>();
> +    return PathDiagnosticLocation(Dtor.getBindTemporaryExpr(), SM,
> CallerCtx);
> +  }
>
>
> This is all right. I wonder if it's better to show the bind-temporary
> expression or the full-expression where the cleanup actually happens.
>
> Jordan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140331/4c415e27/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Prep-work-for-fixing-C-analysis-handling-of-temporar.patch
Type: application/octet-stream
Size: 21437 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140331/4c415e27/attachment.obj>