[cfe-commits] [PATCH][Review Request]CXXDefaultArg handling

[Ted Kremenek]

Hi Jim,

First, sorry for the delay in coming back this patch.

If I understand you correctly, the assumption you are making here is that the default arguments will be evaluated once we do the function call.  That means they are evaluated on the callee's side, not the caller's.  Is this the right thing to do?

As you know, checkers can monitor function/method calls at the the exact point before and after the call is evaluated. Technically this means that all the parameters, including the default parameters, have been evaluated.  Also, technically the function call may never take place if one of the default arguments introduces control-flow that aborts the call.  For example:

void halt() __attribute__((noreturn));
void foo(int x = (halt(), 1));

void test() {
  foo(); // the function foo() never actually gets called.
}

If we compile this to LLVM bytecode, we get:

define void @_Z4testv() ssp {
  call void @_Z4haltv() noreturn
  unreachable
                                                  ; No predecessors!
  call void @_Z3fooi(i32 1)
  ret void
}

>From the LLVM bytecode, we can see that technically the default arguments are evaluated in the caller, not the callee.  This means if we are going to truly support default parameters we need to embed CFG support for them in the caller's CFG, not the callee's.  I think the problem with your patch is it really skirts around the issue, and I'd rather we implement support for default parameters for real.  From this design perspective, I also think this has nothing to do with interprocedural analysis.  Intraprocedural analyses will need to be able to reason about default parameters as well.

What do you think?

On Dec 5, 2010, at 5:12 PM, Jim Goodnow II wrote:

> For the current intraprocedural analysis, I don't think anything really needs to be done for this class, but I left a FIXME for the future interprocedural analysis where CallEnter will need to deal with the actual default arguments for the called procedure.
> 
> - jim
> 
> Index: lib/Checker/GRExprEngine.cpp
> ===================================================================
> --- lib/Checker/GRExprEngine.cpp	(revision 120972)
> +++ lib/Checker/GRExprEngine.cpp	(working copy)
> @@ -795,7 +795,6 @@
>     // C++ stuff we don't support yet.
>     case Stmt::CXXBindTemporaryExprClass:
>     case Stmt::CXXCatchStmtClass:
> -    case Stmt::CXXDefaultArgExprClass:
>     case Stmt::CXXDependentScopeMemberExprClass:
>     case Stmt::CXXExprWithTemporariesClass:
>     case Stmt::CXXNullPtrLiteralExprClass:
> @@ -946,6 +945,12 @@
>       break;
>     }
> 
> +    // FIXME: for now, nothing to do, so just ignore, will want to do more
> +    // when doing interprocedural calls
> +    case Stmt::CXXDefaultArgExprClass:
> +      Dst.insert(Pred);
> +      break;
> +
>     case Stmt::CXXNewExprClass: {
>       const CXXNewExpr *NE = cast<CXXNewExpr>(S);
>       VisitCXXNewExpr(NE, Pred, Dst);<defaultarg.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits