Re: New feature, warning on n=n++ etc.

[Robert Lipe <robertl at dgii dot com>]

I could find no discussion of this patch nor any evidence it was
committed.

I just dusted it off, cleaned it up, applied it to my egcs tree and
performed a bootstrap with it in place.   No problems noted.  

I then fed a large chunk of code through it and it did successfully spot
a few problems.  After I unravelled the macros that generated the sleaze
in question, I agree with the new warning.  The code in question is
wrong.

As EGCS' optimizer gets increasingly ambitious, offering programmers
a warning that we may change the order of their undefined operations
seems like a good plan.  I can't speak to the accuracy/reliability of
the implementation, but I really like the idea that Michael presents.

I can help beat the top bit into a testsuite entry if there's interest.

I can also work with Michael on the minor style violations (I put spaces
in front of my semis, too, but I know that isn't the way things are
done here) and the mechanics of the commit if given clearance that
the implementation is OK.  If necessary, we can then submit a "final"
version back to the list for final approval.

How about it?

RJL

Michael Meeks wrote:

> 	A first attempt, at a first submission to egcs. I have signed a
> copyright assignment, posted it to RMS and recieved a reply confirming
> this.
> 
> 	The feature is to catch probably undefined syntax. The following
> cases are flagged, where Fail denotes a warning.
> 
>   a = a++ ; /* Fail */
>   a = --a ; /* Fail */
>   a = ++a + b ; /* Fail */
>   a = a-- + b ; /* Fail */
>   a = (a++ && 4) ; /* shouldn't Fail but does */
>   a[n]=b[n++] ; /* Fail */
>   a[--n]=b[n] ; /* Fail */
>   a[++n]=b[--n] ; /* Fail */
>   c[n][n]=c[n][n]++ ; /* should fail but doesn't */
>   c[n][p]=c[n][n++] ; /* Fail */ /* if [n][n] then fails twice */
>   *ptr++ = (int)ptr++ ; /* Fail */
>   ptr->a = ptr->a++ ; /* should fail but doesn't */
>   ptr->a = (int)(ptr++) ; /* Fail */
>   len = sprintf (ans, "%d", len++) ;    /* Fail */
>   *ptr++ = fn(*ptr) ; /* should fail but doesn't */
>   a = b = a++ ;   /* Fail */
>   b = a = --b ;   /* Fail */
>   a = 1 + (a=1) ; /* Fail */
>   a = (a=b) ;     /* Fail */
>   a = (a=b) + 1 ; /* Fail */
> 
> 	A changelog entry might look like : ?
> 
> Tue Jun 16 19:41:18 1998  Michael Meeks  (michael@imaginator.com)
> 
> 	* c-toplev.c: Added -Wundefined to lang_options.
> 	* c-decl.c (c_decode_option): Added -Wundefined, added warn_undefined
> 	to -Wall.
> 	* c-iterate.c (collect_iterators): Broke out operand classification code
> 	into get_max_tree_operands.
> 	* tree.c (get_max_tree_operands): Returns max. valid number of operands
> 	used solely in parse tree.
> 	* tree.h: appended prototype.
> 	* c-typeck.c (build_modify_expr): Added call to check_modify_expr
> 	(check_modify_expr): Created, checks and warns for some undefined syntax.
> 
> 
> 	I hope this is useful,
> 
> 	Regards,
> 
> 		Michael Meeks.
> 
> -- 
>  michael@imaginator.com  <><, Pseudo Engineer, itinerant idiot

Content-Description: The patch
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/c-decl.c oldgcc/c-decl.c
> *** gcc/c-decl.c	Mon Feb  9 21:15:13 1998
> --- oldgcc/c-decl.c	Tue Jun 16 19:54:10 1998
> ***************
> *** 558,567 ****
> --- 558,569 ----
>   
>   int warn_sign_compare = -1;
>   
>   /* Nonzero means `$' can be in an identifier.  */
>   
> + int warn_undefined = 0 ;
> + 
>   #ifndef DOLLARS_IN_IDENTIFIERS
>   #define DOLLARS_IN_IDENTIFIERS 1
>   #endif
>   int dollars_in_ident = DOLLARS_IN_IDENTIFIERS;
>   
> ***************
> *** 710,719 ****
> --- 712,723 ----
>       warn_parentheses = 0;
>     else if (!strcmp (p, "-Wreturn-type"))
>       warn_return_type = 1;
>     else if (!strcmp (p, "-Wno-return-type"))
>       warn_return_type = 0;
> +   else if (!strcmp(p, "-Wundefined"))
> +     warn_undefined = 1 ;
>     else if (!strcmp (p, "-Wcomment"))
>       ; /* cpp handles this one.  */
>     else if (!strcmp (p, "-Wno-comment"))
>       ; /* cpp handles this one.  */
>     else if (!strcmp (p, "-Wcomments"))
> ***************
> *** 756,765 ****
> --- 760,770 ----
>         warn_unused = 1;
>         warn_switch = 1;
>         warn_format = 1;
>         warn_char_subscripts = 1;
>         warn_parentheses = 1;
> +       warn_undefined = 1;
>         warn_missing_braces = 1;
>         /* We set this to 2 here, but 1 in -Wmain, so -ffreestanding can turn
>   	 it off only if it's not explicit.  */
>         warn_main = 2;
>       }
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/c-iterate.c oldgcc/c-iterate.c
> *** gcc/c-iterate.c	Mon Aug 11 16:57:03 1997
> --- oldgcc/c-iterate.c	Tue Jun 16 19:54:10 1998
> ***************
> *** 236,264 ****
>   						       list));
>   
>   	case 'e':
>   	case 'r':
>   	  {
> ! 	    int num_args = tree_code_length[(int) TREE_CODE (exp)];
>   	    int i;
>   
> - 	    /* Some tree codes have RTL, not trees, as operands.  */
> - 	    switch (TREE_CODE (exp))
> - 	      {
> - 	      case CALL_EXPR:
> - 		num_args = 2;
> - 		break;
> - 	      case METHOD_CALL_EXPR:
> - 		num_args = 3;
> - 		break;
> - 	      case WITH_CLEANUP_EXPR:
> - 		num_args = 1;
> - 		break;
> - 	      case RTL_EXPR:
> - 		return list;
> - 	      }
> - 		
>   	    for (i = 0; i < num_args; i++)
>   	      list = collect_iterators (TREE_OPERAND (exp, i), list);
>   	    return list;
>   	  }
>   	default:
> --- 236,249 ----
>   						       list));
>   
>   	case 'e':
>   	case 'r':
>   	  {
> ! 	    /* Some tree codes have RTL, not trees, as operands.  */
> ! 	    int num_args = get_max_tree_operands (exp) ;
>   	    int i;
>   
>   	    for (i = 0; i < num_args; i++)
>   	      list = collect_iterators (TREE_OPERAND (exp, i), list);
>   	    return list;
>   	  }
>   	default:
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/c-tree.h oldgcc/c-tree.h
> *** gcc/c-tree.h	Mon Aug 11 16:57:03 1997
> --- oldgcc/c-tree.h	Tue Jun 16 19:54:10 1998
> ***************
> *** 492,501 ****
> --- 492,503 ----
>   
>   /* Warn about comparison of signed and unsigned values.  */
>   
>   extern int warn_sign_compare;
>   
> + extern int warn_undefined;
> + 
>   /* Nonzero means this is a function to call to perform comptypes
>      on two record types.  */
>   
>   extern int (*comptypes_record_hook) ();
>   
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/c-typeck.c oldgcc/c-typeck.c
> *** gcc/c-typeck.c	Sat Sep 27 04:46:33 1997
> --- oldgcc/c-typeck.c	Tue Jun 16 21:08:57 1998
> ***************
> *** 3804,3813 ****
> --- 3804,3918 ----
>       value = non_lvalue (value);
>   
>     return value;
>   }
>   
> + /* Recursive Check for un-defined sytax, note lhs && rhs have
> +    no meaning ! ie. n++=n ; is equaly invalid to n=n++ ; */
> + void
> + check_modify_expr (lhs, rhs)
> +      tree lhs, rhs;
> + {
> + tree identifierName ;   /* a VAR_DECL name on the lhs, that could
> + 			   be the same as one on the rhs */
> + identifierName = NULL_TREE ;
> + 
> + if (lhs == NULL_TREE || rhs == NULL_TREE)
> +   return ;
> + 
> + /*fprintf (stderr, "Check modify expr in '%s', line '%d' : '%s' = '%s'\n",input_filename, lineno, tree_code_name[TREE_CODE(lhs)], tree_code_name[TREE_CODE(rhs)]) ;*/
> + 
> + switch (TREE_CODE(rhs))
> +   {
> +   case (VAR_DECL):
> +     identifierName = DECL_NAME(rhs) ;
> +     break;	  
> +   case PREDECREMENT_EXPR:
> +   case PREINCREMENT_EXPR:
> +   case POSTDECREMENT_EXPR:
> +   case POSTINCREMENT_EXPR:
> +     {
> +       tree var_decl = TREE_OPERAND(rhs,0) ;
> +       if (TREE_CODE(var_decl)==VAR_DECL)
> + 	identifierName=DECL_NAME(var_decl) ;
> +     }
> +   break ;
> +   case TREE_LIST:
> +     {
> +       tree parm = TREE_CHAIN(rhs) ;
> +       int lp = 0 ;
> +       /* Now scan all the list. eg. indices of multi dimensional array */
> +       while (parm)
> + 	{
> + 	  check_modify_expr(lhs, TREE_VALUE(parm)) ;
> + 	  parm = TREE_CHAIN(parm) ;
> + 	}
> +     }
> +     return ;
> +   case NOP_EXPR:
> +     return ;
> +   case MODIFY_EXPR:
> +     /* First check for form a = b = a++ by checking rhs */
> +     check_modify_expr (lhs, TREE_OPERAND(rhs, 1)) ;
> +     /* Then check for a = (a = 1) + 2 */
> +     identifierName = DECL_NAME(TREE_OPERAND(rhs,0)) ;
> +     break ;
> +   default: /* We don't know what to do... pray check_modify_expr removes loops in the tree */
> +     switch (TREE_CODE_CLASS(TREE_CODE(rhs)))
> +       {
> +       case 'r':
> +       case '<':	
> +       case '2':
> +       case 'b':
> +       case '1':
> +       case 'e':
> +       case 's':
> +       case 'x':
> + 	{
> + 	  int lp ;
> + 	  int max = get_max_tree_operands(rhs) ;
> + 	  for (lp=0;lp<max;lp++)
> + 	    check_modify_expr(lhs, TREE_OPERAND(rhs,lp)) ;
> + 	  return ;
> + 	}
> +       default:
> + 	return ;
> +       }
> +     break ;
> +   }
> + if (identifierName != NULL_TREE)
> +   {
> +     switch (TREE_CODE(lhs))
> +       {
> + 	/* perhaps this variable was incremented on the rhs */
> +       case VAR_DECL:
> + 	if (TREE_CODE(rhs)!=VAR_DECL)
> + 	  if (DECL_NAME(lhs)==identifierName)
> + 	    warning ("operation on '%s' may be undefined", IDENTIFIER_POINTER(DECL_NAME(lhs))) ;
> + 	break ;
> +       case PREDECREMENT_EXPR:
> +       case PREINCREMENT_EXPR:
> +       case POSTDECREMENT_EXPR:
> +       case POSTINCREMENT_EXPR:
> + 	{
> + 	  tree var_decl = TREE_OPERAND(lhs,0) ;
> + 	  if (TREE_CODE(var_decl)==VAR_DECL)
> + 	    if (identifierName==DECL_NAME(var_decl))
> + 	      warning ("operation on '%s' may be undefined", IDENTIFIER_POINTER(DECL_NAME(var_decl))) ;
> + 	}
> +       break ;
> +       case NULL_TREE:
> + 	return ;
> +       default:
> + 	/* to save duplicating tree traversal code swap args, and recurse */
> + 	check_modify_expr(rhs, lhs) ;
> + 	break ;
> +       }
> +   }
> + }
> + 
> + 
>   /* Build an assignment expression of lvalue LHS from value RHS.
>      MODIFYCODE is the code for a binary operator that we use
>      to combine the old value of LHS with RHS to get the new value.
>      Or else MODIFYCODE is NOP_EXPR meaning do a simple assignment.  */
>   
> ***************
> *** 3953,3962 ****
> --- 4058,4072 ----
>   
>     newrhs = convert_for_assignment (lhstype, newrhs, "assignment",
>   				   NULL_TREE, NULL_TREE, 0);
>     if (TREE_CODE (newrhs) == ERROR_MARK)
>       return error_mark_node;
> + 
> +   if (warn_undefined)
> +     check_modify_expr(lhs,rhs) ;
> + 
> +   /* Scan operands */
>   
>     result = build (MODIFY_EXPR, lhstype, lhs, newrhs);
>     TREE_SIDE_EFFECTS (result) = 1;
>   
>     /* If we got the LHS in a different type for storing in,
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/toplev.c oldgcc/toplev.c
> *** gcc/toplev.c	Mon Feb  9 00:56:08 1998
> --- oldgcc/toplev.c	Tue Jun 16 19:54:10 1998
> ***************
> *** 819,828 ****
> --- 819,829 ----
>     "-Wno-missing-prototypes",
>     "-Wnested-externs",
>     "-Wno-nested-externs",
>     "-Wparentheses",
>     "-Wno-parentheses",
> +   "-Wundefined",
>     "-Wpointer-arith",
>     "-Wno-pointer-arith",
>     "-Wredundant-decls",
>     "-Wno-redundant-decls",
>     "-Wsign-compare",
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/tree.c oldgcc/tree.c
> *** gcc/tree.c	Fri Nov 14 14:30:00 1997
> --- oldgcc/tree.c	Tue Jun 16 19:54:10 1998
> ***************
> *** 4713,4723 ****
> --- 4713,4750 ----
>   	  buffer[index] = 1;
>   	}
>       }
>     return non_const_bits;
>   }
> + 
> + /* Return the maximum valid number of operands that are used solely for
> +   the parse tree. NB. Some tree codes have RTL, not trees, as operands. */
>   
> + int
> + get_max_tree_operands (exp)
> + tree exp;
> + {
> +   if (exp==NULL_TREE)
> +     return 0 ;
> + 
> +   switch (TREE_CODE (exp))
> +     {
> +     case CALL_EXPR:
> +       return 2 ;
> +     case METHOD_CALL_EXPR:
> +       return 3 ;
> +     case WITH_CLEANUP_EXPR:
> +       return 1 ;
> +     case RTL_EXPR:
> +       return 0 ;
> +     case OP_IDENTIFIER:
> +       return 0 ;
> +     }
> +   
> +   return tree_code_length[(int) TREE_CODE(exp)] ;
> + }
> + 
>   /* Expand (the constant part of) a SET_TYPE CONSTRUCTOR node.
>      The result is placed in BUFFER (which is an array of bytes).
>      If the constructor is constant, NULL_TREE is returned.
>      Otherwise, a TREE_LIST of the non-constant elements is emitted.  */
>   
> diff -C 5 -x *.[ao] -x c-parse.c -x bi-parser.c -x cexp.c -x objc-parse.c -x Makefile -x config.status gcc/tree.h oldgcc/tree.h
> *** gcc/tree.h	Wed Oct 15 18:19:40 1997
> --- oldgcc/tree.h	Tue Jun 16 19:54:10 1998
> ***************
> *** 1887,1891 ****
> --- 1887,1896 ----
>   
>   /* Output a marker (i.e. a label) for the absolute end of the generated
>      code for a function definition.  */
>   
>   extern void dwarf2out_end_epilogue	PROTO((void));
> + 
> + extern int get_max_tree_operands PROTO((tree));
> + 
> + 
> + 
> 

-- 
Robert Lipe       http://www.dgii.com/people/robertl       robertl@dgii.com
              (WEB ADDRESS MAY BE TEMPORARILY UNAVAILABLE)