[cfe-commits] r76821 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/Sema.h lib/Sema/SemaDecl.cpp test/Sema/return.c test/SemaCXX/return.cpp test/SemaObjC/return.m

[Daniel Dunbar]

Nice.

On Wed, Jul 22, 2009 at 4:56 PM, Mike Stump<mrs at apple.com> wrote:
> Author: mrs
> Date: Wed Jul 22 18:56:57 2009
> New Revision: 76821
>
> URL: http://llvm.org/viewvc/llvm-project?rev=76821&view=rev
> Log:
> Add warning for falling off the end of a function that should return a
> value.  This is on by default, and controlled by -Wreturn-type (-Wmost
> -Wall).  I believe there should be very few false positives, though
> the most interesting case would be:
>
>  int() { bar(); }
>
> when bar does:
>
>  bar() { while (1) ; }
>
> Here, we assume functions return, unless they are marked with the
> noreturn attribute.  I can envision a fixit note for functions that
> never return normally that don't have a noreturn attribute to add a
> noreturn attribute.

Seems like a good idea to me!


> +Sema::ControlFlowKind Sema::CheckFallThrough(Stmt *Root) {
> +  llvm::OwningPtr<CFG> cfg (CFG::buildCFG(Root, &Context));
> +
> +  // FIXME: They should never return 0, fix that, delete this code.
> +  if (cfg == 0)
> +    return NeverFallThrough;
> +  // The CFG leaves in dead things, run a simple mark and sweep on it
> +  // to weed out the trivially dead things.

Nit: this is just BFS, not mark and sweep?

> +  std::queue<CFGBlock*> workq;

This could use a SmallVector and DFS, which would be slightly more
efficient, although it may not matter.

> +  llvm::OwningArrayPtr<char> live(new char[cfg->getNumBlockIDs()]);

I switch this to a BitVector.

> +  // Prep work queue
> +  workq.push(&cfg->getEntry());
> +  // Solve
> +  while (!workq.empty()) {
> +    CFGBlock *item = workq.front();
> +    workq.pop();
> +    live[item->getBlockID()] = 1;
> +    CFGBlock::succ_iterator i;
> +    for (i=item->succ_begin(); i != item->succ_end(); ++i) {

The end iterator doesn't need to be recalculated, nor does the
variable declaration need to be outside the loop, please fix to be
more consistent with other code.

> +      if ((*i) && ! live[(*i)->getBlockID()]) {

No space after ! please. Three '(*i)' expressions is pushing my
limits, just to save a line of code. I can live with this, but please
spill '(*i)' in next loop into a temporary, especially to get rid of
**i.

> +        live[(*i)->getBlockID()] = 1;
> +        workq.push(*i);
> +      }
> +    }
> +  }
> +
> +  CFGBlock::succ_iterator i;

Again, doesn't need to be outside loop.

> +  bool HasLiveReturn = false;
> +  bool HasFakeEdge = false;
> +  bool HasPlainEdge = false;
> +  for (i=cfg->getExit().pred_begin(); i != cfg->getExit().pred_end(); ++i) {
> +    if (!live[(*i)->getBlockID()])
> +      continue;
> +    if ((*i)->size() == 0) {
> +      // A labeled empty statement, or the entry block...
> +      HasPlainEdge = true;
> +      continue;
> +    }
> +    Stmt *S = (**i)[(*i)->size()-1];
> +    if (isa<ReturnStmt>(S)) {
> +      HasLiveReturn = true;
> +      continue;
> +    }
> +    if (isa<ObjCAtThrowStmt>(S)) {
> +      HasFakeEdge = true;
> +      continue;
> +    }
> +    if (isa<CXXThrowExpr>(S)) {
> +      HasFakeEdge = true;
> +      continue;
> +    }
> +    bool NoReturnEdge = false;
> +    if (CallExpr *C = dyn_cast<CallExpr>(S))
> +      {
> +        Expr *CEE = C->getCallee()->IgnoreParenCasts();
> +        if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(CEE)) {
> +          if (FunctionDecl *FD = dyn_cast<FunctionDecl>(DRE->getDecl())) {
> +            if (FD->hasAttr<NoReturnAttr>()) {
> +              NoReturnEdge = true;
> +              HasFakeEdge = true;
> +            }

We should also check for noreturn message sends, I believe.

> +          }
> +        }
> +      }

The indentation and bracing here is inconsistent with clang, please
fix. I have a tingling suspicion the logic is also overly complicated,
there are three return values and 8 possible states of boolean
variables. The NoReturnEdge variable can at least be eliminated by
using 'continue' instead of falling through.

> +    if (NoReturnEdge == false)
> +      HasPlainEdge = true;
> +  }
> +  if (HasPlainEdge) {

Please reduce nesting, as in:
--
if (!HasPlainEdge)
  return NeverFallThrough;
--

> +    if (HasFakeEdge|HasLiveReturn)

Please use || with spaces.

> +      return MaybeFallThrough;
> +    // This says never for calls to functions that are not marked noreturn, that
> +    // don't return.  For people that don't like this, we encourage marking the
> +    // functions as noreturn.

This doesn't make sense. That is, I understand what it means, but the
prose is trying hard to prevent that from occurring. ;)

> +    return AlwaysFallThrough;
> +  }
> +  return NeverFallThrough;
> +}

> +/// CheckFallThroughForFunctionDef - Check that we don't fall off the end of a
> +/// function that should return a value.
> +///
> +/// \returns Never iff we can never alter control flow (we always fall off the
> +/// end of the statement, Conditional iff we might or might not alter
> +/// control-flow and Always iff we always alter control flow (we never fall off
> +/// the end of the statement).
> +void Sema::CheckFallThroughForFunctionDef(Decl *D, Stmt *Body) {
> +  // FIXME: Would be nice if we had a better way to control cascding errors, but
> +  // for now, avoid them.
> +  if (getDiagnostics().hasErrorOccurred())
> +    return;

Typo ('cascding'). Why is this needed?

> +  if (Diags.getDiagnosticLevel(diag::warn_maybe_falloff_nonvoid_function)
> +      == Diagnostic::Ignored)
> +    return;
> +  // FIXME: Funtion try block
> +  if (CompoundStmt *Compound = dyn_cast<CompoundStmt>(Body)) {
> +    switch (CheckFallThrough(Body)) {
> +    case MaybeFallThrough:
> +      Diag(Compound->getRBracLoc(), diag::warn_maybe_falloff_nonvoid_function);
> +      break;
> +    case AlwaysFallThrough:
>
> +      Diag(Compound->getRBracLoc(), diag::warn_falloff_nonvoid_function);
> +      break;
> +    case NeverFallThrough:
> +      break;
> +    }
> +  }
> +}
> +
>  /// CheckParmsForFunctionDef - Check that the parameters of the given
>  /// function are appropriate for the definition of a function. This
>  /// takes care of any checks that cannot be performed on the
> @@ -3246,6 +3357,10 @@
>   Stmt *Body = BodyArg.takeAs<Stmt>();
>   if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(dcl)) {
>     FD->setBody(Body);
> +    if (!FD->getResultType()->isVoidType()
> +        // C and C++ allow for main to automagically return 0.
> +        && !FD->isMain())
> +      CheckFallThroughForFunctionDef(FD, Body);

It would be nice to get the 'noreturn function does return warning'
too, which is now trivial to add.

Did you do any timing to see what the impact of CFG construction on Sema was?

 - Daniel