[cfe-commits] [PATCH][Review Request]CXXDefaultArg handling
Ted Kremenek
kremenek at apple.com
Thu Dec 23 14:48:53 PST 2010
Hi Jim,
Unfortunately I don't think it is this simple. The problem is that the default argument expressions reside with the declaration for the called function; they aren't part of the statement tree for the caller. For example, here is an example dump using your patch:
$ cat t.cpp
int bar();
void foo(int y = bar() > 10 ? 1 : 0);
void test() {
foo();
foo();
}
$ clang -fsyntax-only -Xclang -ast-dump t.cpp
... <SNIP>
void test() (CompoundStmt 0x105842bc0 <t.cpp:3:13, line:6:1>
(CallExpr 0x105842a38 <line:4:3, col:7> 'void'
(ImplicitCastExpr 0x105842a18 <col:3> 'void (*)(int)' <FunctionToPointerDecay>
(DeclRefExpr 0x1058429c0 <col:3> 'void (int)' lvalue FunctionDecl='foo' 0x1058427f0))
(CXXDefaultArgExpr 0x105842a78 <<invalid sloc>> 'int'
(ConditionalOperator 0x105842748 <line:2:18, col:35> 'int'
(BinaryOperator 0x1058426b8 <col:18, col:26> '_Bool' '>'
(CallExpr 0x105842658 <col:18, col:22> 'int'
(ImplicitCastExpr 0x105842638 <col:18> 'int (*)(void)' <FunctionToPointerDecay>
(DeclRefExpr 0x105817fb8 <col:18> 'int (void)' lvalue FunctionDecl='bar' 0x105817e60)))
(IntegerLiteral 0x105842688 <col:26> 'int' 10))
(IntegerLiteral 0x1058426e8 <col:31> 'int' 1)
(IntegerLiteral 0x105842718 <col:35> 'int' 0))))
(CallExpr 0x105842b58 <line:5:3, col:7> 'void'
(ImplicitCastExpr 0x105842b38 <col:3> 'void (*)(int)' <FunctionToPointerDecay>
(DeclRefExpr 0x105842b08 <col:3> 'void (int)' lvalue FunctionDecl='foo' 0x1058427f0))
(CXXDefaultArgExpr 0x105842b98 <<invalid sloc>> 'int'
(ConditionalOperator 0x105842748 <line:2:18, col:35> 'int'
(BinaryOperator 0x1058426b8 <col:18, col:26> '_Bool' '>'
(CallExpr 0x105842658 <col:18, col:22> 'int'
(ImplicitCastExpr 0x105842638 <col:18> 'int (*)(void)' <FunctionToPointerDecay>
(DeclRefExpr 0x105817fb8 <col:18> 'int (void)' lvalue FunctionDecl='bar' 0x105817e60)))
(IntegerLiteral 0x105842688 <col:26> 'int' 10))
(IntegerLiteral 0x1058426e8 <col:31> 'int' 1)
(IntegerLiteral 0x105842718 <col:35> 'int' 0)))))
Here's the parts I want to highlight:
void test() (CompoundStmt 0x105842bc0 <t.cpp:3:13, line:6:1>
(CallExpr 0x105842a38 <line:4:3, col:7> 'void'
...
(CXXDefaultArgExpr 0x105842a78 <<invalid sloc>> 'int'
(ConditionalOperator 0x105842748 <line:2:18, col:35> 'int'
...
(CallExpr 0x105842b58 <line:5:3, col:7> 'void'
....
(CXXDefaultArgExpr 0x105842b98 <<invalid sloc>> 'int'
(ConditionalOperator 0x105842748 <line:2:18, col:35> 'int'
...
Note that the pointer address for both ConditionalOperator is the same:
(ConditionalOperator 0x105842748 <line:2:18, col:35> 'int'
(ConditionalOperator 0x105842748 <line:2:18, col:35> 'int'
The reason they are the same because they are the same exact expression tree. This is bad for the analyzer because we use the uniqueness of Stmt* values to identify statements/expressions within an analyzed function. Further, neither nested conditional expression is represented in the caller's CFG.
To correctly analyze CXXDefaultArgExprs, I see two options:
(1) We try and inline them in some way into the caller's CFG. This poses lots of challenges, because the Stmt*'s won't be unique.
(2) We try and handle this using interprocedural analysis via inlining. That is, when we see a CXXDefaultArgExpr, we construct a CFG for its subexpressions, jump to the CFG, analyze those expressions, and then jump back to the original CFG. This is basically the same way we will handle CallExprs via interprocedural analysis via inlining.
Note that (2) is *not* the same as including the analysis of default arguments are part of analyzing the called function. The default arguments need to be analyzed first, then the CallExpr needs to be analyzed separately. This will allow Checkers to interpose themselves at the right places. To do (2), we will need to create a new LocationContext subclass, and add logic to GRExprEngine to lazily construct the CFG for the default argument expression, and do all the value propagation (e.g., we need to capture the final value of the evaluated default argument and propagate it back to the original function we are analyzing).
On Dec 23, 2010, at 3:37 AM, Jim Goodnow II wrote:
> 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);
> <defaultarg.patch>
More information about the cfe-commits
mailing list