Re: [PATCH] Fix build_access_matrix and -ftree-loop-linear memory leaks (PR tree-optimization/37031)

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Thu, Jan 8, 2009 at 4:28 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The testcase in the PR fails with -O3 -ftree-loop-linear when executed
> under valgrind, full q_matrix.c from SPEC2k6 fails even without it.
> The bug is that in lambda-code.c's build_access_matrix AM_MATRIX(am) = matrix
> is created with wrong size (lambda_nb_columns instead of ndim), and then
> ndim vectors are VEC_safe_pushed into matrix.  If ndim > lambda_nb_columns,
> then some of the pushes might have reallocated the vector, so matrix can be
> != AM_MATRIX (am) and thus AM_MATRIX (am) points to freed memory.
>
> After fixing this, we can (also in graphite.c) use VEC_quick_push, we know
> we reserved enough.  The rest of the patch is just fixing a bunch of memory
> leaks on the testcase.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2009-01-08  Jakub Jelinek  <jakub@redhat.com>
>
>        PR tree-optimization/37031
>        * lambda-code.c (lambda_collect_parameters): Call pointer_set_destroy
>        on parameter_set.
>        (build_access_matrix): Reserve correct size for AM_MATRIX vector,
>        allocate it using gc instead of heap, use VEC_quick_push instead of
>        VEC_safe_push.
>        * graphite.c (build_access_matrix): Allocate AM_MATRIX vector using gc
>        instead of heap, use VEC_quick_push instead of VEC_safe_push.
>        * tree-data-ref.h (struct access_matrix): Change matrix to gc
>        allocated vector from heap allocated.
>        * lambda.h: Add DEF_VEC_ALLOC_P for gc allocated lambda_vector.
>        * tree-loop-linear.c (linear_transform_loops): Allocate nest
>        vector only after perfect_loop_nest_depth call.
>
> --- gcc/lambda-code.c.jj        2009-01-08 13:56:15.000000000 +0100
> +++ gcc/lambda-code.c   2009-01-08 14:38:24.000000000 +0100
> @@ -1,5 +1,6 @@
>  /*  Loop transformation code generation
> -    Copyright (C) 2003, 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
> +    Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009
> +    Free Software Foundation, Inc.
>     Contributed by Daniel Berlin <dberlin@dberlin.org>
>
>     This file is part of GCC.
> @@ -2682,6 +2683,7 @@ lambda_collect_parameters (VEC (data_ref
>     for (j = 0; j < DR_NUM_DIMENSIONS (data_reference); j++)
>       lambda_collect_parameters_from_af (DR_ACCESS_FN (data_reference, j),
>                                         parameter_set, parameters);
> +  pointer_set_destroy (parameter_set);
>  }
>
>  /* Translates BASE_EXPR to vector CY.  AM is needed for inferring
> @@ -2792,15 +2794,13 @@ build_access_matrix (data_reference_p da
>   unsigned i, ndim = DR_NUM_DIMENSIONS (data_reference);
>   unsigned nivs = VEC_length (loop_p, nest);
>   unsigned lambda_nb_columns;
> -  lambda_vector_vec_p matrix;
>
>   AM_LOOP_NEST (am) = nest;
>   AM_NB_INDUCTION_VARS (am) = nivs;
>   AM_PARAMETERS (am) = parameters;
>
>   lambda_nb_columns = AM_NB_COLUMNS (am);
> -  matrix = VEC_alloc (lambda_vector, heap, lambda_nb_columns);
> -  AM_MATRIX (am) = matrix;
> +  AM_MATRIX (am) = VEC_alloc (lambda_vector, gc, ndim);
>
>   for (i = 0; i < ndim; i++)
>     {
> @@ -2810,7 +2810,7 @@ build_access_matrix (data_reference_p da
>       if (!av_for_af (access_function, access_vector, am))
>        return false;
>
> -      VEC_safe_push (lambda_vector, heap, matrix, access_vector);
> +      VEC_quick_push (lambda_vector, AM_MATRIX (am), access_vector);
>     }
>
>   DR_ACCESS_MATRIX (data_reference) = am;
> --- gcc/tree-data-ref.h.jj      2008-12-03 16:32:17.000000000 +0100
> +++ gcc/tree-data-ref.h 2009-01-08 14:31:41.000000000 +0100
> @@ -1,5 +1,6 @@
>  /* Data references and dependences detectors.
> -   Copyright (C) 2003, 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
> +   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009
> +   Free Software Foundation, Inc.
>    Contributed by Sebastian Pop <pop@cri.ensmp.fr>
>
>  This file is part of GCC.
> @@ -131,7 +132,7 @@ struct access_matrix
>   VEC (loop_p, heap) *loop_nest;
>   int nb_induction_vars;
>   VEC (tree, heap) *parameters;
> -  VEC (lambda_vector, heap) *matrix;
> +  VEC (lambda_vector, gc) *matrix;
>  };
>
>  #define AM_LOOP_NEST(M) (M)->loop_nest
> --- gcc/graphite.c.jj   2008-12-16 09:48:41.000000000 +0100
> +++ gcc/graphite.c      2009-01-08 14:32:27.000000000 +0100
> @@ -1,5 +1,5 @@
>  /* Gimple Represented as Polyhedra.
> -   Copyright (C) 2006, 2007, 2008 Free Software Foundation, Inc.
> +   Copyright (C) 2006, 2007, 2008, 2009 Free Software Foundation, Inc.
>    Contributed by Sebastian Pop <sebastian.pop@inria.fr>.
>
>  This file is part of GCC.
> @@ -3335,7 +3335,7 @@ build_access_matrix (data_reference_p re
>   int i, ndim = DR_NUM_DIMENSIONS (ref);
>   struct access_matrix *am = GGC_NEW (struct access_matrix);
>
> -  AM_MATRIX (am) = VEC_alloc (lambda_vector, heap, ndim);
> +  AM_MATRIX (am) = VEC_alloc (lambda_vector, gc, ndim);
>   DR_SCOP (ref) = GBB_SCOP (gb);
>
>   for (i = 0; i < ndim; i++)
> @@ -3347,7 +3347,7 @@ build_access_matrix (data_reference_p re
>       if (!build_access_matrix_with_af (af, v, scop, ref_nb_loops (ref)))
>        return false;
>
> -      VEC_safe_push (lambda_vector, heap, AM_MATRIX (am), v);
> +      VEC_quick_push (lambda_vector, AM_MATRIX (am), v);
>     }
>
>   DR_ACCESS_MATRIX (ref) = am;
> --- gcc/lambda.h.jj     2008-10-23 13:21:41.000000000 +0200
> +++ gcc/lambda.h        2009-01-08 14:36:19.000000000 +0100
> @@ -1,5 +1,6 @@
>  /* Lambda matrix and vector interface.
> -   Copyright (C) 2003, 2004, 2005, 2006, 2007 Free Software Foundation, Inc.
> +   Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009
> +   Free Software Foundation, Inc.
>    Contributed by Daniel Berlin <dberlin@dberlin.org>
>
>  This file is part of GCC.
> @@ -30,6 +31,7 @@ along with GCC; see the file COPYING3.
>  typedef int *lambda_vector;
>  DEF_VEC_P(lambda_vector);
>  DEF_VEC_ALLOC_P(lambda_vector,heap);
> +DEF_VEC_ALLOC_P(lambda_vector,gc);
>
>  typedef VEC(lambda_vector, heap) *lambda_vector_vec_p;
>  DEF_VEC_P (lambda_vector_vec_p);
> --- gcc/tree-loop-linear.c.jj   2008-10-23 13:21:39.000000000 +0200
> +++ gcc/tree-loop-linear.c      2009-01-08 14:43:13.000000000 +0100
> @@ -1,5 +1,6 @@
>  /* Linear Loop transforms
> -   Copyright (C) 2003, 2004, 2005, 2007 Free Software Foundation, Inc.
> +   Copyright (C) 2003, 2004, 2005, 2007, 2008, 2009
> +   Free Software Foundation, Inc.
>    Contributed by Daniel Berlin <dberlin@dberlin.org>.
>
>  This file is part of GCC.
> @@ -334,12 +335,13 @@ linear_transform_loops (void)
>       lambda_trans_matrix trans;
>       struct obstack lambda_obstack;
>       struct loop *loop;
> -      VEC(loop_p,heap) *nest = VEC_alloc (loop_p, heap, 3);
> +      VEC(loop_p,heap) *nest;
>
>       depth = perfect_loop_nest_depth (loop_nest);
>       if (depth == 0)
>        continue;
>
> +      nest = VEC_alloc (loop_p, heap, 3);
>       for (loop = loop_nest; loop; loop = loop->inner)
>        VEC_safe_push (loop_p, heap, nest, loop);
>
>
>        Jakub
>