Community
Participate
Working Groups
The following function triggers a bogus "No return, in function returning non-void" warning: int foo(bool p) { for (;;) { if (p) { return 0; } } }
Great example of why this stuff is hard ;) Solvable, but man, you got to really spend the time on it to get it right.
So it looks like the cause of this is that we do ControlFlowGraphBuilder.isConstant(decision, 1) in order to decide whether the condition can ever terminate. The empty expression obviously isn't a constant expression, so this fails. I think treating an empty expression as true within a for loop condition is something that doesn't occur anywhere else in the C standard, so for me it makes sense to fix this in ControlFlowGraphBuilder.createFor() rather than changing the logic in addOutgoing(). The naive solution is something like: ICPPNodeFactory nodeFactory = CPPNodeFactory.getDefault(); CxxDecisionNode decision = factory.createDecisionNode(forNode.getConditionExpression()); if (decision.getData() == null) { decision.setData( nodeFactory.newLiteralExpression( IASTLiteralExpression.lk_integer_constant, "1")); } Constructing a CPPNodeFactory here doesn't seem like it's the right thing to do (it spits a bunch of API warnings, apart from anything else). Is there a better way to do this? Another problem with this approach is that it doesn't fix the bug that: int foo(bool p) { for(;42;) { if (p) { return 0; } } } reports a warning; this is because the isConstant(decision, 1) check only checks for it to be equal to 1. I think this is actually a bug in addOutgoing().
I agree that you probably don't want to be creating new AST nodes in ControlFlowGraphBuilder. What if we simply had isConstant() treat a null IASTNode as constant? I don't think a null IASTNode would arise anywhere other than the condition of a for-loop. To solve the second problem, we could change the second argument of isConstant() from long to boolean, and cast the numerical value of the expression to boolean before comparing it to the expected value.
Digging a little more into isConstant(), it doesn't seem to work properly. If I try: int bar(bool p) { for (int x = 0; x < 10; x++) { if (p) { return 0; } } } Then it doesn't report a warning. This seems to be because the "x < 10" evaluates as a constant according to isConstant(). The expression gets passed through to Value.evaluateBinaryExpression(), which returns 1 since the LHS returns a value of 0. It seems like the problem is that it thinks it has a value for the x variable without figuring out that it varies in this context. I haven't traced this all the way through yet. My guess is that the best fix is to evaluate the expression without knowledge of any variable values, though I don't know how easy that is to do.
Yikes! When Value.evaluateBinding() handles an IVariable, it should probably only use getInitialValue() if the variable is constant; otherwise, it should consider its value to be Value.UNKNOWN. You can check whether the variable is constant by getting its type, resolving any typedefs inside with 'SemanticUtil.getNestedType(type, TDEF)', and seeing if the resulting type is an IQualifierType with isConst() = true.
Right, this works but causes a regression in ControlFlowGraphTest.test_deadbranch(), which has: foo() { int a=10,x=5; if (x<0) a++; } The test requires that the else{} branch be a dead node. I'm not sure of the importance of this test case. No other tests seem to be failing, so perhaps it's OK to change to a test case with a const-qualified variable type. This is technically a regression, but the ability to infer variable values seems quite limited anyway. The other alternative is to fix just the original bug as reported and leave the "x < 10" case in the for loop to be dealt with as a separate issue. I think it's probably worth making the "x < 10" for loop case work, because that's the most common pattern, and unless anything specific is dependent on the other case that's a price worth paying.
I agree, we should just change that test case for now. In the longer term we could do something smarter, where if a variable is not constant but has an initial value, we try to determine whether its value could have changed between the point of initialization and the point of access, and if we can prove that it couldn't have, then we use its initial value.
New Gerrit change created: https://git.eclipse.org/r/54358
OK, Hudson claims the build is failing. Looking at it, I can't immediately see how the failure is related to my change. If I'm reading it right, the codan tests are passing but the build is being abandoned in the managed builder tests? https://hudson.eclipse.org/cdt/job/cdt-verify/3228/
(In reply to Tim Martin from comment #9) > OK, Hudson claims the build is failing. Looking at it, I can't immediately > see how the failure is related to my change. If I'm reading it right, the > codan tests are passing but the build is being abandoned in the managed > builder tests? > > https://hudson.eclipse.org/cdt/job/cdt-verify/3228/ Occasionally builds are aborted due to intermittent issues, I'm guessing with the test infrastructure. I see Sergey has retriggered the tests [1], and this time they did complete, and there are a few failures which do look related to your patch. [1] https://hudson.eclipse.org/cdt/job/cdt-verify/3229/
For the record, I changed the remaining test cases in the same way, for the same reasoning as above, but Elena objected to changing these test cases. I guess the non-const type is an important part of these test cases, but that wasn't clear to me from looking at the test case code. If this behaviour is important, then we can't fix it this way. I appreciate that the ideal is not to rely on const type to distinguish between cases where the initial variable value can be used or not. However, I think the level of inference required to solve the general case is going to be a lot more work than I was anticipating. I'm going to leave this ticket for someone else who knows the Codan code better than I do.
Yim, to do proper value propagation we need DFA (data-flow analysis) engine which is above and beyond scope of this PR. But to do original sample few lines of fix that calculate value for "null" ast nodes should be sufficient, no? (Part of your patch)
You're right, we can fix the original report easily and not worry about the for loop case for now. I've extracted the necessary lines and re-submitted the patch. I went with an overload of the isConstant() method as you suggested; the version that takes a long isn't used at the moment, so it seems unnecessary to me, but I suppose it could be used in the future.
How suppress this warning by using comment '@suppress("No return")'? Or this suppression comment not work?
(In reply to Alien Huker from comment #14) > How suppress this warning by using comment '@suppress("No return")'? > Or this suppression comment not work? It works for me. For example: int foo() // @suppress("No return") { } Note that the text of the suppression comment can be customized in Preferences -> C/C++ -> Code Analysis -> <select checker> -> Customize Selected.
*** Bug 515463 has been marked as a duplicate of this bug. ***
*** Bug 550509 has been marked as a duplicate of this bug. ***
