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

[Jim Goodnow II]

At 08:45 PM 12/22/2010, Ted Kremenek wrote:
>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?

I agree. How about this revised patch which displays the actual 
default arguments when the AST is dumped and actually handles the 
argument itself?

Index: lib/AST/StmtDumper.cpp
===================================================================
--- lib/AST/StmtDumper.cpp      (revision 122493)
+++ lib/AST/StmtDumper.cpp      (working copy)
@@ -157,6 +157,7 @@
      // C++
      void VisitCXXNamedCastExpr(CXXNamedCastExpr *Node);
      void VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *Node);
+    void VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Node);
      void VisitCXXThisExpr(CXXThisExpr *Node);
      void VisitCXXFunctionalCastExpr(CXXFunctionalCastExpr *Node);
      void VisitCXXConstructExpr(CXXConstructExpr *Node);
@@ -500,6 +501,12 @@
    OS << " " << (Node->getValue() ? "true" : "false");
  }

+void StmtDumper::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Node) {
+  DumpExpr(Node);
+  OS << "\n";
+  DumpSubTree(Node->getExpr());
+}
+
  void StmtDumper::VisitCXXThisExpr(CXXThisExpr *Node) {
    DumpExpr(Node);
    OS << " this";
Index: lib/EntoSA/Checkers/ExprEngine.cpp
===================================================================
--- lib/EntoSA/Checkers/ExprEngine.cpp  (revision 122493)
+++ lib/EntoSA/Checkers/ExprEngine.cpp  (working copy)
@@ -753,7 +753,6 @@
      // C++ stuff we don't support yet.
      case Stmt::CXXBindTemporaryExprClass:
      case Stmt::CXXCatchStmtClass:
-    case Stmt::CXXDefaultArgExprClass:
      case Stmt::CXXDependentScopeMemberExprClass:
      case Stmt::ExprWithCleanupsClass:
      case Stmt::CXXNullPtrLiteralExprClass:
@@ -911,6 +910,12 @@
        break;
      }

+    case Stmt::CXXDefaultArgExprClass: {
+      const CXXDefaultArgExpr *DAE = cast<CXXDefaultArgExpr>(S);
+      Visit(DAE->getExpr(), Pred, Dst);
+      break;
+    }
+
      case Stmt::CXXDeleteExprClass: {
        const CXXDeleteExpr *CDE = cast<CXXDeleteExpr>(S);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: defaultarg.patch
Type: application/octet-stream
Size: 1723 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20101223/09ea2b57/attachment.obj>