r175857 - [analyzer] Place all inlining policy checks into one palce

[Jordan Rose]

YAY. I could not figure out how to do this, especially with the block part.

A few comments.


On Feb 21, 2013, at 18:59 , Anna Zaks <ganna at apple.com> wrote:

> Author: zaks
> Date: Thu Feb 21 20:59:24 2013
> New Revision: 175857
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=175857&view=rev
> Log:
> [analyzer] Place all inlining policy checks into one palce
> 
> Previously, we had the decisions about inlining spread out
> over multiple functions.
> 
> In addition to the refactor, this commit ensures
> that we will always inline BodyFarm functions as long as the Decl
> is available. This fixes false positives due to those functions
> not being inlined when no or minimal inlining is enabled such (as
> shallow mode).
> 
> Modified:
>    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
>    cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
>    cfe/trunk/test/Analysis/NSString.m
> 

> +  const LocationContext *ParentOfCallee = CallerSFC;
> +  if (Call.getKind() == CE_Block) {
>     const BlockDataRegion *BR = cast<BlockCall>(Call).getBlockRegion();

This should probably just use dyn_cast now -- more idiomatic.


> +static bool shouldInlineCallKind(const CallEvent &Call,
> +                                      const ExplodedNode *Pred,
> +                                      AnalyzerOptions &Opts) {

Indentation is wrong.


> +bool ExprEngine::shouldInlineCall(const CallEvent &Call, const Decl *D,
> +                                  const ExplodedNode *Pred) {
> +  if (!D )
> +    return false;

Extra space?


> +  AnalyzerOptions &Opts = getAnalysisManager().options;
> +  AnalysisDeclContext *CalleeADC =
> +    Call.getLocationContext()->getAnalysisDeclContext()->
> +    getManager()->getContext(D);

AnalysisManager &AMgr = getAnalysisManager();
AnalyzerOptions &Opts = AMgr.options;
AnalysisDeclContextManager &ADCMgr = AMgr.getAnalysisDeclContextManager();

AnalysisDeclContext *CalleeADC = ADCMgr.getContext(D);


> +  // The auto-synthesized bodies are essential to inline as they are
> +  // usually small and commonly used. note, we should do this check early on to
> +  // ensure we always inline these calls.

Capitalization. (Also, conventionally a colon instead of a comma after "Note".)


> +  if (CalleeADC->isBodyAutosynthesized())
> +    return true;
> +
> +  if (HowToInline == Inline_None)
> +    return false;
> +
> +  // Check if we should inline a call based on it's kind.

s/it's/its/


> +  if (!shouldInlineCallKind(Call, Pred, Opts))
> +    return false;
> +
> +  // It is possible that the CFG cannot be constructed.
> +  // Be safe, and check if the CalleeCFG is valid.
> +  const CFG *CalleeCFG = CalleeADC->getCFG();
> +  if (!CalleeCFG)
> +    return false;
> +
> +  // Do not inline if recursive or we've reached max stack frame count.
> +  bool IsRecursive = false;
> +  unsigned StackDepth = 0;
> +  examineStackFrames(D, Pred->getLocationContext(), IsRecursive, StackDepth);
> +  if ((StackDepth >= Opts.InlineMaxStackDepth) &&
> +      ((CalleeCFG->getNumBlockIDs() > Opts.getAlwaysInlineSize())
> +       || IsRecursive))
> +    return false;
> +
> +  // Do not inline if it took too long to inline previously.
> +  if (Engine.FunctionSummaries->hasReachedMaxBlockCount(D))
> +    return false;
> +
> +  // Or if the function is too big.
> +  if (CalleeCFG->getNumBlockIDs() > Opts.getMaxInlinableSize())
> +    return false;
> +
> +  // Do not inline variadic calls (for now).
> +  if (const BlockDecl *BD = dyn_cast<BlockDecl>(D)) {
> +    if (BD->isVariadic())
> +      return false;
> +  }
> +  else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
> +    if (FD->isVariadic())
> +      return false;
> +  }

Now that there's a CallEvent here, we should make a CallEvent::isVariadic, since ObjC method calls can be variadic as well.


> +  // Check our template policy.
> +  if (getContext().getLangOpts().CPlusPlus) {
> +    if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
> +      // Conditionally allow the inlining of template functions.
> +      if (!Opts.mayInlineTemplateFunctions())
> +        if (FD->getTemplatedKind() != FunctionDecl::TK_NonTemplate)
> +          return false;
> +
> +      // Conditionally allow the inlining of C++ standard library functions.
> +      if (!Opts.mayInlineCXXStandardLibrary())
> +        if (getContext().getSourceManager().isInSystemHeader(FD->getLocation()))
> +          if (IsInStdNamespace(FD))
> +            return false;
> +    }
> +  }
> +
> +  // It is possible that the live variables analysis cannot be
> +  // run.  If so, bail out.
> +  if (!CalleeADC->getAnalysis<RelaxedLiveVariables>())
> +    return false;
> +
> +  // Do not inline large functions too many times.
> +  if (Engine.FunctionSummaries->getNumTimesInlined(D) >
> +      Opts.getMaxTimesInlineLarge() &&
> +      CalleeCFG->getNumBlockIDs() > 13) {

The indentation here confused me at first; IMHO, "Opts.getMaxTimesInlineLarge()" should be extra-indented, or perhaps the comparison should be wrapped in parens. The magic number 13 is pretty bad, too...

(I realize these aren't new problems, but they're here.)