calloc = malloc + memset

Jakub Jelinek jakub@redhat.com
Fri Apr 18 11:31:00 GMT 2014


On Sun, Mar 23, 2014 at 05:13:46PM +0100, Marc Glisse wrote:
> 2014-03-23  Marc Glisse  <marc.glisse@inria.fr>
> 
>         PR tree-optimization/57742
> gcc/
>         * tree-ssa-strlen.c (get_string_length): Ignore malloc.
>         (handle_builtin_malloc, handle_builtin_memset): New functions.
> 	(strlen_optimize_stmt): Call them.
> 	* passes.def: Move strlen after loop+dom.
> gcc/testsuite/
>         * g++.dg/tree-ssa/calloc.C: New testcase.
>         * gcc.dg/tree-ssa/calloc-1.c: Likewise.
>         * gcc.dg/tree-ssa/calloc-2.c: Likewise.
> 	* gcc.dg/strlenopt-9.c: Adapt.

Some CL lines are indented by 8 spaces instead of tabs.
The passes.def change makes me a little bit nervous, but if it works,
perhaps.

> --- gcc/testsuite/g++.dg/tree-ssa/calloc.C	(revision 0)
> +++ gcc/testsuite/g++.dg/tree-ssa/calloc.C	(working copy)
> @@ -0,0 +1,35 @@
> +/* { dg-do compile { target c++11 } } */
> +/* { dg-options "-O3 -fdump-tree-optimized" } */
> +
> +#include <new>
> +#include <vector>
> +#include <cstdlib>
> +
> +void g(void*);
> +inline void* operator new(std::size_t sz)
> +{
> +  void *p;
> +
> +  if (sz == 0)
> +    sz = 1;
> +
> +  // Slightly modified from the libsupc++ version, that one has 2 calls
> +  // to malloc which makes it too hard to optimize.
> +  while ((p = std::malloc (sz)) == 0)
> +    {
> +      std::new_handler handler = std::get_new_handler ();
> +      if (! handler)
> +        throw std::bad_alloc();
> +      handler ();
> +    }
> +  return p;
> +}
> +
> +void f(void*p,int n){
> +  new(p)std::vector<int>(n);
> +}
> +
> +/* { dg-final { scan-tree-dump-times "calloc" 1 "optimized" } } */
> +/* { dg-final { scan-tree-dump-not "malloc" "optimized" } } */
> +/* { dg-final { scan-tree-dump-not "memset" "optimized" } } */
> +/* { dg-final { cleanup-tree-dump "optimized" } } */

This looks to me way too much fragile, any time the libstdc++
or glibc headers change a little bit, you might need to adjust the
dg-final directives.  Much better would be if you just provided
the prototypes yourself and subset of the std::vector you really need for
the testcase.  You can throw some class or int, it doesn't have to be
std::bad_alloc, etc.

> --- gcc/testsuite/gcc.dg/strlenopt-9.c	(revision 208772)
> +++ gcc/testsuite/gcc.dg/strlenopt-9.c	(working copy)
> @@ -11,21 +11,21 @@ fn1 (int r)
>       optimized away.  */
>    return strchr (p, '\0');
>  }
>  
>  __attribute__((noinline, noclone)) size_t
>  fn2 (int r)
>  {
>    char *p, q[10];
>    strcpy (q, "abc");
>    p = r ? "a" : q;
> -  /* String length for p varies, therefore strlen below isn't
> +  /* String length is constant for both alternatives, and strlen is
>       optimized away.  */
>    return strlen (p);

Is this because of jump threading?

> --- gcc/testsuite/gcc.dg/tree-ssa/calloc-1.c	(revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/calloc-1.c	(working copy)
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +#include <stdlib.h>
> +#include <string.h>

Even this I find unsafe.  The strlenopt*.c tests use it's custom
strlenopt.h header for a reason, you might just add a calloc
prototype in there and use that header.

> --- gcc/tree-ssa-strlen.c	(revision 208772)
> +++ gcc/tree-ssa-strlen.c	(working copy)
> @@ -496,20 +496,23 @@ get_string_length (strinfo si)
>  	case BUILT_IN_STPCPY_CHK:
>  	  gcc_assert (lhs != NULL_TREE);
>  	  loc = gimple_location (stmt);
>  	  si->endptr = lhs;
>  	  si->stmt = NULL;
>  	  lhs = fold_convert_loc (loc, size_type_node, lhs);
>  	  si->length = fold_convert_loc (loc, size_type_node, si->ptr);
>  	  si->length = fold_build2_loc (loc, MINUS_EXPR, size_type_node,
>  					lhs, si->length);
>  	  break;
> +	case BUILT_IN_MALLOC:
> +	  break;
> +	  /* calloc always has si->length set.  */

I guess it would be better to talk about BUILT_IN_CALLOC here, and align
the comment with case and default column, to make it clear the comment
is not about BUILT_IN_MALLOC.  Or is it?
But, see below.

>  	default:

>  	if (!si->dont_invalidate)
>  	  {
>  	    ao_ref r;
> +	    // Do not use si->length.

As the whole file uses /* ... */ comments, can you use them here too?

>  	    ao_ref_init_from_ptr_and_size (&r, si->ptr, NULL_TREE);
>  	    if (stmt_may_clobber_ref_p_1 (stmt, &r))
>  	      {
>  		set_strinfo (i, NULL);
>  		free_strinfo (si);
>  		continue;
>  	      }
>  	  }
>  	si->dont_invalidate = false;
>  	nonempty = true;
> @@ -1608,20 +1612,90 @@ handle_builtin_strcat (enum built_in_fun
>  	{
>  	  laststmt.stmt = stmt;
>  	  laststmt.len = srclen;
>  	  laststmt.stridx = dsi->idx;
>  	}
>      }
>    else if (dump_file && (dump_flags & TDF_DETAILS) != 0)
>      fprintf (dump_file, "not possible.\n");
>  }
>  
> +/* Handle a call to malloc or calloc.  */
> +
> +static void
> +handle_builtin_malloc (enum built_in_function bcode, gimple_stmt_iterator *gsi)
> +{
> +  gimple stmt = gsi_stmt (*gsi);
> +  tree lhs = gimple_call_lhs (stmt);
> +  gcc_assert (get_stridx (lhs) == 0);
> +  int idx = new_stridx (lhs);
> +  tree length = NULL_TREE;
> +  if (bcode == BUILT_IN_CALLOC)
> +    length = build_int_cst (size_type_node, 0);

Is this safe?  I mean, if you call int a = 0; ptr = calloc (a, n);
or ptr = calloc (n, a); or ptr = calloc (0, 0); etc., then there is no
zero termination.  Sure, calling strlen or strchr etc. on the result
is undefined behavior, just wondering if it isn't going to break anything.
Though, the result is zero length and thus any dereferencing would be a bug,
so perhaps we are fine.

> +static bool
> +handle_builtin_memset (gimple_stmt_iterator *gsi)
> +{
> +  gimple stmt1, stmt2 = gsi_stmt (*gsi);
> +  if (!integer_zerop (gimple_call_arg (stmt2, 1)))
> +    return true;
> +  tree ptr = gimple_call_arg (stmt2, 0);
> +  int idx1 = get_stridx (ptr);
> +  if (idx1 <= 0)
> +    return true;
> +  strinfo si1 = get_strinfo (idx1);
> +  if (!si1 || !(stmt1 = si1->stmt) || !is_gimple_call (stmt1))
> +    return true;

Please avoid assignments in if conditions if easily possible, in this
case just using separate if (si1 == NULL) return true; and
stmt1 = si1->stmt; and another if is IMHO better.

	Jakub



More information about the Gcc-patches mailing list