Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [cdt-dev] False positives with Codan using Avr-gcc toolchain

I've reconsidered and modified my patch - please see attached, I'll look at testing and submitting via gerrit ASAP, in the meantime, can someone look at and check my code - I'm not familiar with the API and it's very possible I've made a silly mistake or assumed too much.

I've moved the checks to the better position (lets assume that things get worse) so that there shouldn't be any impact on performance - however if we cannot feasibly garauntee that we check for this statement being the last in the a Statement-_expression_, the cheaper option is to not walking inside of it and have a single check before we go down the "hasNoEffect" branch


On Fri, Apr 25, 2014 at 8:58 AM, Chris Marr <cdabone@xxxxxxxxx> wrote:
Normally I would agree, however branch tests are of such low cost, I can't see the impact being significant (even for low end machines) unless there's some aspect of the java runtime that makes these branch tests of a high performance cost

I did however decide that there is a problem with the logic: if the statement being tested is not the last in the list of a statement-_expression_, then it could be a valid "statement has no effect" warning.

I don't know much about the object model, but if it's possible to identify that the statement is the last in it's block, AND it's a part of a statement-_expression_, then ignore it, otherwise, make normal checks - I will look into modifying the patch accordingly and see if I can add a test


On Thu, Apr 24, 2014 at 8:54 PM, Alena Laskavaia <elaskavaia.cdt@xxxxxxxxx> wrote:
Btw moving this check up will have a performance impact, it is better to check this condition when we found something to report


On Thu, Apr 24, 2014 at 12:10 PM, Nathan Ridge <zeratul976@xxxxxxxxxxx> wrote:
Hi Chris,

Thanks for writing the patch, it looks good!

Could you submit it via Gerrit as described here [1]? Thanks!

If you wanted to add a test to StatementHasNoEffectCheckerTest.java
as well, that would round things out nicely.

Regards,
Nate

[1] http://wiki.eclipse.org/CDT/git#Using_Gerrit_for_CDT

________________________________
> Date: Thu, 24 Apr 2014 13:06:19 +0100
> From: cdabone@xxxxxxxxx
> To: cdt-dev@xxxxxxxxxxx
> Subject: Re: [cdt-dev] False positives with Codan using Avr-gcc toolchain
>
> I've dug out the codan analyser source and looked thought about it
> some, based on what you've said [Nathan] - it seems to me that the
> logic for this scenario can never flag true for a statement-_expression_,
> as all C operations will return either void (valid for
> statement-expressions according to the manual) or otherwise be valid
> R-values. The only other thing to check for is perhaps a void return
> from a statement-_expression_, being assigned to an Lvalue, but that will
> show up as a compile error (I believe, I may be wrong...), rather than
> a "statement has no effect" type bug.
>
> Based on that, I've made some adjustments to the .java file for that
> checker and attached a patch file (apologies if it's not perfect, I'm
> on a windows machine at the moment behind a troublesome proxy, I've
> copied and pasted the source into a.txt file and used that to generate
> the patch)
>
> Chris
>
>
>
> On Thu, Apr 24, 2014 at 12:22 PM, Chris Marr
> <cdabone@xxxxxxxxx<mailto:cdabone@xxxxxxxxx>> wrote:
> Thanks muchly Nathan, Statement-Expressions are new to me (being very
> much tied into historic and non-GNU compilers in my day job) so as they
> say, every day is school day.
>
> Thanks for the feedback
> Chris
>
>
> On Thu, Apr 24, 2014 at 8:30 AM, Nathan Ridge
> <zeratul976@xxxxxxxxxxx<mailto:zeratul976@xxxxxxxxxxx>> wrote:
> Hi Chris,
>
> This actually looks like a bug in Codan.
>
> Expressions of the form
>
>      ({ <statements> })
>
> are a GCC extension called "statement-expressions" [1]. Their
> semantics is that they evaluate to the value of the _expression_
> in the last statement (or void if the last statement is not an
> _expression_ statement).
>
> ('__extension__' is just a GCC builtin that prevents certain
> warnings from being issued [2]. CDT treats it as a macro with
> an empty expansion.)
>
> It is therefore incorrect to give a "statement has no effect"
> warning for the last statement in a statement-_expression_, since
> in this case the value of the _expression_ in the statement is
> actually used.
>
> Codan already has some logic to avoid giving this warning in
> such cases, but it's buggy and some cases like yours slip
> through the cracks. I filed a bug for this [3] and will provide
> a patch for it soon.
>
> Regards,
> Nate
>
> [1] http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> [2] http://gcc.gnu.org/onlinedocs/gcc/Alternate-Keywords.html
> [3] https://bugs.eclipse.org/bugs/show_bug.cgi?id=433357
>
> ________________________________
> > Date: Wed, 23 Apr 2014 23:56:30 +0100
> > From: cdabone@xxxxxxxxx<mailto:cdabone@xxxxxxxxx>
> > To: cdt-dev@xxxxxxxxxxx<mailto:cdt-dev@xxxxxxxxxxx>
> > Subject: [cdt-dev] False positives with Codan using Avr-gcc toolchain
> >
> > Hi,
> >
> > I've just started developing with Eclipse and the Avr-GCC (WinAVR)
> > toolchain and I get a few false positives with codan. They're
> > irritating and out of harmony (I like clean code) so I'm trying to
> > alleviate them where possible. For those that don't know, one aspect of
> > Avr-GCC is a flash memory management library that uses some C
> > extensions to move RAM type initialisation into flash (avr/pgmspace.h
> > header) and manage it all via a set of macros. One specific macro for
> > managing strings (so you can pass info back and forth through a serial
> > port etc) is PSTR(s) which expands thus:
> >
> > # define PSTR(s) (__extension__({static char __c[] PROGMEM = (s);
> &__c[0];}))
> >
> > That "&__c[0];" is getting interpretted as a Codan warning "Statement
> > has no effect {0}" when it's actually the return value of the PSTR
> > macro (why not just regular macros? why have __extension__ in there? If
> > it needs to be a function, be a useful function and return the pointer
> > it's generating)
> >
> > I'd like to supress it either by fixing the avr toolchain (beyond me
> > today) or supress the error in this instance (add an exception to
> > codan's generation of this message).
> >
> > So 1) does codan unwrap macros before evaluating exceptions?
> >
> > I'd like to add an exception to the error that filters out the
> > _expression_: "(__extension({*}))" or should I filter "PSTR(*)"?
> >
> > I'm new to the "__extension__" keyword, so I'm not entirely sure how
> > that's evaluated in avr-gcc (and if someone says lmgtfy I'll slap them,
> > because if it was that simple, i wouldn't be here asking the question)
> > - the manual's a little thin in that regard
> >
> > in anticipation
> >
> > Chris
> >
> > _______________________________________________ cdt-dev mailing list
> > cdt-dev@xxxxxxxxxxx<mailto:cdt-dev@xxxxxxxxxxx>
> https://dev.eclipse.org/mailman/listinfo/cdt-dev
>
> _______________________________________________
> cdt-dev mailing list
> cdt-dev@xxxxxxxxxxx<mailto:cdt-dev@xxxxxxxxxxx>
> https://dev.eclipse.org/mailman/listinfo/cdt-dev
>
>
>
> _______________________________________________ cdt-dev mailing list
> cdt-dev@xxxxxxxxxxx https://dev.eclipse.org/mailman/listinfo/cdt-dev

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cdt-dev


_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cdt-dev



--- StatementHasNoEffectChecker.java	Thu Apr 24 12:40:40 2014
+++ StatementHasNoEffectChecker.java	Fri Apr 25 12:22:06 2014
@@ -68,6 +68,25 @@
 			return PROCESS_CONTINUE;
 		}
 
+		private boolean isFinalStatementExpression(IASTExpression e) {
+			// check if it is part of GNU comp stmt expression i.e. ({foo();a;})
+			IASTNode stmt = e.getParent();
+			if (stmt instanceof IASTExpressionStatement) {
+				IASTNode parentComp = stmt.getParent();
+				if (parentComp instanceof IASTCompoundStatement) {
+					IASTNode parentStmtExpr = parentComp.getParent();
+					if (parentStmtExpr instanceof IGNUASTCompoundStatementExpression) {
+						// Is it the final statement in the statement-expression?
+						// NOTE: Presumes that the array returned is in the same order as presented in the source
+						// NOTE: Presumes that the expression is the actual expression, and not a copy.
+						IASTNode childlist[] = parentComp.getChildren();
+						if (stmt == childlist[childlist.length-1])
+							return true;
+					}
+				}
+			}
+			return false;
+		}
 		/**
 		 * We consider has not effect binary statements without assignment and
 		 * unary statement which is not dec and inc. If operator is overloaded
@@ -88,7 +107,7 @@
 					case IASTBinaryExpression.op_logicalAnd:
 						return hasNoEffect(binExpr.getOperand1()) && hasNoEffect(binExpr.getOperand2());
 				}
-				return true;
+				return isFinalStatementExpression(e) ? false : true;
 			}
 			if (e instanceof IASTUnaryExpression) {
 				IASTUnaryExpression unaryExpr = (IASTUnaryExpression) e;
@@ -105,22 +124,11 @@
 					case IASTUnaryExpression.op_bracketedPrimary:
 						return hasNoEffect(unaryExpr.getOperand());
 				}
-				return true;
+				return isFinalStatementExpression(e) ? false : true;
 			}
 			// simply a;
 			if (e instanceof IASTIdExpression) {
-				// check if it is part of GNU comp stmt expression i.e. ({foo();a;})
-				IASTNode parent = e.getParent();
-				if (parent instanceof IASTExpressionStatement) {
-					IASTNode parent2 = parent.getParent();
-					if (parent2 instanceof IASTCompoundStatement) {
-						IASTNode parent3 = parent2.getParent();
-						if (parent3 instanceof IGNUASTCompoundStatementExpression) {
-							return false;
-						}
-					}
-				}
-				return true;
+				return isFinalStatementExpression(e) ? false : true;
 			}
 			return false;
 		}

Back to the top