Re: PATCH: remove unnecessary calls to chkr_set_right()

[Jeffrey A Law <law at cygnus dot com>]

  In message <381131CF.1F9BD61A@zipworld.com.au>you write:
  > At present, compilation of this:
  > 
  > foo()
  > {
  >     int x = 1;
  > }
  > 
  > results in a single call to chkr_check_addr().  This is correct, however th
  > ree spurious calls are made to chkr_set_right() when stacking the args to c
  > hkr_check_addr():
  > 
  > foo:
  >         pushl %ebp
  >         movl %esp,%ebp
  >         subl $4,%esp
  >         pushl %ebx
  >         leal -4(%ebp),%ebx
  >         pushl $2
  >         movl %esp,%eax
  >         pushl $3
  >         pushl $4
  >         pushl %eax
  >         call chkr_set_right
  >         addl $12,%esp
  >         pushl $4
  >         movl %esp,%eax
  >         pushl $3
  >         pushl $4
  >         pushl %eax
  >         call chkr_set_right
  >         addl $12,%esp
  >         pushl %ebx
  >         movl %esp,%eax
  >         pushl $3
  >         pushl $4
  >         pushl %eax
  >         call chkr_set_right
  >         addl $12,%esp
  >         call chkr_check_addr
  >         movl $1,-4(%ebp)
  >         movl -8(%ebp),%ebx
  >         movl %ebp,%esp
  >         popl %ebp
  >         ret
  > 
  > This patch passes the memcheck testsuite:
  > 
  > 
  > 
  >         * builtins.c, function.h, calls.c, expr.c, function.c:
  >           Introduce global int no_chkr_set_right to suppress
  >           generation of calls to chkr_set_right() when stacking
  >           args for chkr_check_addr()
Several problems that need to be fixed.

First, I strongly recommend you read the GNU coding standards.  You violated
them in many places.  You're going to need to clean them up before we can 
install this patch.

We have a variable "in_check_memory_usage" which indicates when we want to
avoid certain calls to the checker.  You should follow the same convention
for avoiding calls to the set_right checker function.

  >         /* Just check DST is writable and mark it as readable.  */
  >         if (current_function_check_memory_usage)
  > !       {
  > ! 	 no_chkr_set_right = 1;
  > ! 	 emit_library_call (chkr_check_addr_libfunc, 1, VOIDmode, 3,
  > ! 			    XEXP (dest_mem, 0), Pmode,
  > ! 			    len_rtx, TYPE_MODE (sizetype),
  > ! 			    GEN_INT (MEMORY_USE_WO),
  > ! 			    TYPE_MODE (integer_type_node));
  > !          no_chkr_set_right = 0;
Use a tab instead of 8 spaces in the "no_chkr_set_right = 0;" assignment.

Also note that braces must be indented two spaces in from their parent.
And statements within braces must be indented two spaces in from the
braces themselves.   You messed this up in many places.  They will all need
to be fixed.

	if (foo)
	  {
	    blah blah blah
	    foo bar com
	  }


  > + /* If true, do not emit calls to chkr_set_right() */
  > + extern int no_chkr_set_right;
The comment should read:

/* If nonzero, do not emit calls to chkr_set_right.  */

We generally do not use true/false, but zero/nonzero.  When referring to a
function name, do not put parens after it.  A comment should end with a period,
two spaces, then the close comment sequence.


  >     if (arg->value == arg->stack)
  >       {
  >         /* If the value is already in the stack slot, we are done.  */
  > !       if (current_function_check_memory_usage
  > !           && GET_CODE (arg->stack) == MEM
  > ! 	  && ! no_chkr_set_right)
Tab instead of 8 spaces.

  > --- 3213,3221 ----
  >   
  >         emit_move_insn (gen_rtx_MEM (mode, addr), x);
  >   
  > !       if (current_function_check_memory_usage
  > !           && ! in_check_memory_usage
  > ! 	  && ! no_chkr_set_right)
Likewise.  Tab instead of 8 spaces.

  > *************** expand_assignment (to, from, want_value,
  > *** 3402,3412 ****
  > --- 3408,3422 ----
  >   
  >   	  /* Check the access right of the pointer.  */
  >   	  if (size)
  > + 	  {
  > + 	    no_chkr_set_right = 1;
  >   	    emit_library_call (chkr_check_addr_libfunc, 1, VOIDmode, 3,
  >   			       to_addr, Pmode,
  >   			       GEN_INT (size), TYPE_MODE (sizetype),
  >   			       GEN_INT (MEMORY_USE_WO),
  >   			       TYPE_MODE (integer_type_node));
  > + 	    no_chkr_set_right = 0;
  > + 	  }
Indentions are all messed up here.

  > *************** store_expr (exp, target, want_value)
  > *** 3755,3770 ****
  > --- 3765,3786 ----
  >         && AGGREGATE_TYPE_P (TREE_TYPE (exp)))
  >       {
  >         if (GET_CODE (temp) == MEM)
  > +       {
  >           emit_library_call (chkr_copy_bitmap_libfunc, 1, VOIDmode, 3,
  >   			   XEXP (target, 0), Pmode,
  >   			   XEXP (temp, 0), Pmode,
  >   			   expr_size (exp), TYPE_MODE (sizetype));
  > +       }
  >         else
  > +       {
  > + 	no_chkr_set_right = 1;
  >           emit_library_call (chkr_check_addr_libfunc, 1, VOIDmode, 3,
  >   			   XEXP (target, 0), Pmode, 
  >   			   expr_size (exp), TYPE_MODE (sizetype),
  >   			   GEN_INT (MEMORY_USE_WO), 
  >   			   TYPE_MODE (integer_type_node));
  > + 	no_chkr_set_right = 0;
  > +       }
  >       }
Likewise.  The indentions are mucked up badly.  It also looks like you need
a tab instead of 8 spaces here too.

  > *************** store_expr (exp, target, want_value)
  > *** 3865,3875 ****
  > --- 3881,3895 ----
  >   		{
  >   		  /* Be sure we can write on ADDR.  */
  >   		  if (current_function_check_memory_usage)
  > + 		  {
  > + 		    no_chkr_set_right = 1;
  >   		    emit_library_call (chkr_check_addr_libfunc, 1, VOIDmode, 3,
  >   				       addr, Pmode,
  >   				       size, TYPE_MODE (sizetype),
  >    				       GEN_INT (MEMORY_USE_WO), 
  >   				       TYPE_MODE (integer_type_node));
  > + 		    no_chkr_set_right = 0;
  > + 		  }
Indentions are wrong.


  > *************** expand_expr (exp, target, tmode, modifie
  > *** 5780,5791 ****
  > --- 5800,5815 ----
  >   	  memory_usage = get_memory_usage_from_modifier (modifier);
  >   
  >   	  if (memory_usage != MEMORY_USE_DONT)
  > + 	  {
  > + 	    no_chkr_set_right = 1;
  >   	    emit_library_call (chkr_check_addr_libfunc, 1, VOIDmode, 3,
  >   			       XEXP (DECL_RTL (exp), 0), Pmode,
  >   			       GEN_INT (int_size_in_bytes (type)),
  >   			       TYPE_MODE (sizetype),
  >   			       GEN_INT (memory_usage),
  >   			       TYPE_MODE (integer_type_node));
  > + 	    no_chkr_set_right = 0;
  > + 	  }
  >   	}
Indentions are wrong.


  > *************** expand_expr (exp, target, tmode, modifie
  > *** 6594,6605 ****
  > --- 6620,6635 ----
  >   
  >           	/* Check the access right of the pointer.  */
  >   		if (size > BITS_PER_UNIT)
  > + 		{
  > + 		  no_chkr_set_right = 1;
  >   		  emit_library_call (chkr_check_addr_libfunc, 1, VOIDmode, 3,
  >   				     to, Pmode,
  >   				     GEN_INT (size / BITS_PER_UNIT),
  >   				     TYPE_MODE (sizetype),
  >   				     GEN_INT (memory_usage), 
  >   				     TYPE_MODE (integer_type_node));
  > + 		  no_chkr_set_right = 0;
  > + 		}
  >   	      }
  >   	  }
Indentions are wrong.

  > + /* If true, do not emit calls to chkr_set_right() */
  > + int no_chkr_set_right;
See earlier note about correct comments.

I believe once these issues are dealt with we can install this patch.
jeff