[cfe-commits] r164339 - in /cfe/trunk: include/clang/Analysis/ include/clang/StaticAnalyzer/Core/ include/clang/StaticAnalyzer/Core/BugReporter/ include/clang/StaticAnalyzer/Core/PathSensitive/ lib/Analysis/ lib/StaticAnalyzer/Core/ test/Analysis/

[Jordan Rose]

On Sep 20, 2012, at 23:00 , Ted Kremenek <kremenek at apple.com> wrote:

>> 
>> 
>>> }
>>> 
>>> AnalysisDeclContext *AnalysisDeclContextManager::getContext(const Decl *D) {
>>> +  if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
>>> +    FD->hasBody(FD);
>>> +    D = FD;
>>> +  }
>> 
>> So there is apparently a Decl::getBody that does the right thing (i.e. returns 0 if there is no body). Can we just use that?
> 
> We're not trying to get the body here.  We're trying to update the D to be the one that has the body (if one exists), and use that as the Decl* we use to construct the AnalysisDeclContext.  Maybe getBody() looks cleaner, but it functionality isn't really any different.

Oops, right. What I really want is getDefinition, which currently doesn't exist, but easily could.


> Note that apparently not updating 'D' here correctly broke a ton of stuff.

Yes. Yes it does.



>> 
>>> +  // Everything checks out.  Create a fake body that just calls the block.
>>> +  // This is basically just an AST dump of:
>>> +  //
>>> +  // void dispatch_sync(dispatch_queue_t queue, void (^block)(void)) {
>>> +  //   block();
>>> +  // }
>>> +  //
>>> +  DeclRefExpr *DR = DeclRefExpr::CreateEmpty(C, false, false, false, false);
>>> +  DR->setDecl(const_cast<ParmVarDecl*>(PV));
>>> +  DR->setValueKind(VK_LValue);
>>> +  ImplicitCastExpr *ICE = ImplicitCastExpr::Create(C, Ty, CK_LValueToRValue,
>>> +                                                   DR, 0, VK_RValue);
>>> +  CallExpr *CE = new (C) CallExpr(C, ICE, ArrayRef<Expr*>(), C.VoidTy,
>>> +                                  VK_RValue, SourceLocation());
>>> +  return CE;
>>> +}
>> 
>> The name for ArrayRef<Expr*>() is 
> 
> Hmm?

Oops. I meant to go look up a nice alias because the template is ugly, and in fact MultiExprArg is compatible (it's MutableArrayRef<Expr*>), but this does match the form in CallExpr's constructor.


>> It is probably not a good idea to make a function body that isn't a CompoundStmt or CXXTryStmt.
> 
> Why?  I know how the CFGBuilder works.  This isn't an issue.  The CFGBuilder just flattens out the CompoundStmt.  That concept  doesn't even exist in the CFG.
> 
>> Right now I don't think anything depends on it besides the diagnostics, which don't apply here, but still.

I guess if it's ONLY diagnostics that depend on it, it doesn't matter. (PathDiagnosticLocation::createDeclBegin has a FIXME about 'try'.)



> 
>> 
>> 
>>>   switch (piece->getKind()) {
>>>     case PathDiagnosticPiece::Call: {
>>>       PathDiagnosticCallPiece *call = cast<PathDiagnosticCallPiece>(piece);
>>> @@ -142,8 +148,17 @@
>>>       }
>>>       // Recursively clean out the subclass.  Keep this call around if
>>>       // it contains any informative diagnostics.
>>> -        if (!RemoveUneededCalls(call->path, R))
>>> +        PathDiagnosticCallPiece *NewCallWithLoc =
>>> +          call->getLocation().asLocation().isValid()
>>> +            ? call : CallWithLoc;
>> 
>> I think there should actually be an assertion here -- only inlined functions can be synthesized, so there will always be a non-synthesized call with a valid location.
> 
> Not true.  The synthesized function itself can call other functions.  That call doesn't have a valid location.

Right. I meant that if !call->getLocation().asLocation().isValid(), assert CallWithLoc. Or rather, just assert NewCallWithLoc after this, and then remove one of the tests from the 'if' below...


>> 
>>> +        if (!RemoveUneededCalls(call->path, R, NewCallWithLoc))
>>>         continue;
>>> +
>>> +        if (NewCallWithLoc == CallWithLoc && CallWithLoc) {
>>> +          call->callEnter = CallWithLoc->callEnter;
>>> +        }
>> 
>> Extra braces? Also, when does callExit get set?
> 
> Good point.  We'll need a way to propagate back the call exit location from the bottom.

Well, in the normal traversal, the exit information gets set first. In RemoveUneededCalls (hm, we have a typo there), we already have both entry and exit information, so we should be able to just set it here as well.