Fix PR50260

Hi,

the last change in ipa-split generated a new use of a previously unused 
PARM_DECL.  When one does this one has to call add_referenced_var.  Not 
doing so can cause segfault when accessing the (not initialized) var 
annotation.  So, fixed with the patch.

I took the opportunity to remove all explicit calls to get_var_ann because 
add_referenced_var is doing so as first thing.  Reviewing the code showed 
one potentially problematic case in tree-ssa-pre.c where a new variable 
was created without calling add_referenced_var.  It's only for 
place-holder SSA names but those can conceivably leak into the program 
stream by being reused during expression generation.

Currently regstrapping on x86_64-linux (without Ada).  Okay for trunk?


Ciao,
Michael.

Hi,

On Thu, Sep 01, 2011 at 02:41:15PM +0200, Michael Matz wrote:
> Hi,
> 
> the last change in ipa-split generated a new use of a previously unused 
> PARM_DECL.  When one does this one has to call add_referenced_var.  Not 
> doing so can cause segfault when accessing the (not initialized) var 
> annotation.  So, fixed with the patch.

This is how I have fixed the issue when I ran into it independently
about an hour ago and it is probably my preferred way of dealing with
this, at least for now (that is until Honza makes up his mind what to
do with the type attributes in fnsplit).

Thanks a lot,

Martin

> 
> I took the opportunity to remove all explicit calls to get_var_ann because 
> add_referenced_var is doing so as first thing.  Reviewing the code showed 
> one potentially problematic case in tree-ssa-pre.c where a new variable 
> was created without calling add_referenced_var.  It's only for 
> place-holder SSA names but those can conceivably leak into the program 
> stream by being reused during expression generation.
> 
> Currently regstrapping on x86_64-linux (without Ada).  Okay for trunk?
> 
> 
> Ciao,
> Michael.
> -- 
> 	PR middle-end/50260
> 	* ipa-split.c (split_function): Call add_referenced_var.
> 
> 	* tree-ssa-phiopt.c (cond_store_replacement): Don't call get_var_ann.
> 	(cond_if_else_store_replacement_1): Ditto.
> 	* tree-ssa-pre.c (get_representative_for): Ditto.
> 	(create_expression_by_pieces): Ditto.
> 	(insert_into_preds_of_block): Ditto.
> 	* tree-sra.c (create_access_replacement): Ditto.
> 	(get_replaced_param_substitute): Ditto.
> 
> testsuite/
> 	* gfortran.fortran-torture/compile/pr50260.f90: New test.
> 
> Index: ipa-split.c
> ===================================================================
> --- ipa-split.c	(revision 178408)
> +++ ipa-split.c	(working copy)
> @@ -988,6 +988,9 @@ split_function (struct split_point *spli
>  	arg = gimple_default_def (cfun, parm);
>  	if (!arg)
>  	  {
> +	    /* This parm wasn't used up to now, but is going to be used,
> +	       hence register it.  */
> +	    add_referenced_var (parm);
>  	    arg = make_ssa_name (parm, gimple_build_nop ());
>  	    set_default_def (parm, arg);
>  	  }
> Index: tree-ssa-phiopt.c
> ===================================================================
> --- tree-ssa-phiopt.c	(revision 178408)
> +++ tree-ssa-phiopt.c	(working copy)
> @@ -1269,10 +1269,7 @@ cond_store_replacement (basic_block midd
>    /* 2) Create a temporary where we can store the old content
>          of the memory touched by the store, if we need to.  */
>    if (!condstoretemp || TREE_TYPE (lhs) != TREE_TYPE (condstoretemp))
> -    {
> -      condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
> -      get_var_ann (condstoretemp);
> -    }
> +    condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
>    add_referenced_var (condstoretemp);
>  
>    /* 3) Insert a load from the memory of the store to the temporary
> @@ -1355,10 +1352,7 @@ cond_if_else_store_replacement_1 (basic_
>    /* 2) Create a temporary where we can store the old content
>  	of the memory touched by the store, if we need to.  */
>    if (!condstoretemp || TREE_TYPE (lhs) != TREE_TYPE (condstoretemp))
> -    {
> -      condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
> -      get_var_ann (condstoretemp);
> -    }
> +    condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
>    add_referenced_var (condstoretemp);
>  
>    /* 3) Create a PHI node at the join block, with one argument
> Index: tree-ssa-pre.c
> ===================================================================
> --- tree-ssa-pre.c	(revision 178408)
> +++ tree-ssa-pre.c	(working copy)
> @@ -1399,7 +1399,7 @@ get_representative_for (const pre_expr e
>    if (!pretemp || exprtype != TREE_TYPE (pretemp))
>      {
>        pretemp = create_tmp_reg (exprtype, "pretmp");
> -      get_var_ann (pretemp);
> +      add_referenced_var (pretemp);
>      }
>  
>    name = make_ssa_name (pretemp, gimple_build_nop ());
> @@ -3178,10 +3178,7 @@ create_expression_by_pieces (basic_block
>    /* Build and insert the assignment of the end result to the temporary
>       that we will return.  */
>    if (!pretemp || exprtype != TREE_TYPE (pretemp))
> -    {
> -      pretemp = create_tmp_reg (exprtype, "pretmp");
> -      get_var_ann (pretemp);
> -    }
> +    pretemp = create_tmp_reg (exprtype, "pretmp");
>  
>    temp = pretemp;
>    add_referenced_var (temp);
> @@ -3441,10 +3438,7 @@ insert_into_preds_of_block (basic_block
>  
>    /* Now build a phi for the new variable.  */
>    if (!prephitemp || TREE_TYPE (prephitemp) != type)
> -    {
> -      prephitemp = create_tmp_var (type, "prephitmp");
> -      get_var_ann (prephitemp);
> -    }
> +    prephitemp = create_tmp_var (type, "prephitmp");
>  
>    temp = prephitemp;
>    add_referenced_var (temp);
> Index: tree-sra.c
> ===================================================================
> --- tree-sra.c	(revision 178408)
> +++ tree-sra.c	(working copy)
> @@ -1825,7 +1825,6 @@ create_access_replacement (struct access
>    tree repl;
>  
>    repl = create_tmp_var (access->type, "SR");
> -  get_var_ann (repl);
>    add_referenced_var (repl);
>    if (rename)
>      mark_sym_for_renaming (repl);
> @@ -4106,7 +4105,6 @@ get_replaced_param_substitute (struct ip
>        DECL_NAME (repl) = get_identifier (pretty_name);
>        obstack_free (&name_obstack, pretty_name);
>  
> -      get_var_ann (repl);
>        add_referenced_var (repl);
>        adj->new_ssa_base = repl;
>      }
> Index: testsuite/gfortran.fortran-torture/compile/pr50260.f90
> ===================================================================
> --- testsuite/gfortran.fortran-torture/compile/pr50260.f90	(revision 0)
> +++ testsuite/gfortran.fortran-torture/compile/pr50260.f90	(revision 0)
> @@ -0,0 +1,48 @@
> +MODULE cp_parser_methods
> +  INTEGER, PARAMETER :: default_string_length=80
> +  INTEGER, PARAMETER :: default_path_length=250
> +  TYPE ilist_type
> +     LOGICAL                              :: in_use
> +  END TYPE ilist_type
> +  TYPE cp_parser_type
> +     CHARACTER(LEN=default_path_length)             :: ifn
> +     INTEGER                                        :: icol,icol1,icol2
> +     TYPE(ilist_type), POINTER                      :: ilist
> +  END TYPE cp_parser_type
> +  TYPE cp_error_type
> +  END TYPE cp_error_type
> +CONTAINS
> +  FUNCTION cts(i) RESULT(res)
> +    CHARACTER(len=6)                         :: res
> +  END FUNCTION cts
> +  FUNCTION parser_location(parser,error) RESULT(res)
> +    TYPE(cp_parser_type), POINTER            :: parser
> +    TYPE(cp_error_type), INTENT(inout)       :: error
> +    CHARACTER(len=default_path_length+default_string_length)       :: res
> +    LOGICAL                                  :: failure
> +    IF (.NOT. failure) THEN
> +       res="file:'"//TRIM(parser%ifn)//"' line:"//cts(parser%icol)
> +    END IF
> +  END FUNCTION parser_location
> +  SUBROUTINE parser_get_integer(parser,at_end, error)
> +    TYPE(cp_parser_type), POINTER            :: parser
> +    TYPE(cp_error_type), INTENT(inout)       :: error
> +    LOGICAL                                  :: failure, my_at_end
> +    IF (.NOT.failure) THEN
> +       IF (.NOT.parser%ilist%in_use) THEN
> +          CALL cp_assert("A"// TRIM(parser_location(parser,error)))
> +       END IF
> +    END IF
> +  END SUBROUTINE parser_get_integer
> +  SUBROUTINE parser_get_string(parser,at_end,error)
> +    TYPE(cp_parser_type), POINTER            :: parser
> +    LOGICAL, INTENT(out), OPTIONAL           :: at_end
> +    TYPE(cp_error_type), INTENT(inout)       :: error
> +    LOGICAL                                  :: failure, my_at_end
> +    IF (.NOT.failure) THEN
> +       IF (PRESENT(at_end)) THEN
> +          CALL cp_assert("s"//TRIM(parser_location(parser,error)))
> +       END IF
> +    END IF
> +  END SUBROUTINE parser_get_string
> +END MODULE cp_parser_methods

On Thu, Sep 1, 2011 at 2:41 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> the last change in ipa-split generated a new use of a previously unused
> PARM_DECL.  When one does this one has to call add_referenced_var.  Not
> doing so can cause segfault when accessing the (not initialized) var
> annotation.  So, fixed with the patch.
>
> I took the opportunity to remove all explicit calls to get_var_ann because
> add_referenced_var is doing so as first thing.  Reviewing the code showed
> one potentially problematic case in tree-ssa-pre.c where a new variable
> was created without calling add_referenced_var.  It's only for
> place-holder SSA names but those can conceivably leak into the program
> stream by being reused during expression generation.
>
> Currently regstrapping on x86_64-linux (without Ada).  Okay for trunk?

Ok.  Time to make get_var_ann private?

Richard.

>
> Ciao,
> Michael.
> --
>        PR middle-end/50260
>        * ipa-split.c (split_function): Call add_referenced_var.
>
>        * tree-ssa-phiopt.c (cond_store_replacement): Don't call get_var_ann.
>        (cond_if_else_store_replacement_1): Ditto.
>        * tree-ssa-pre.c (get_representative_for): Ditto.
>        (create_expression_by_pieces): Ditto.
>        (insert_into_preds_of_block): Ditto.
>        * tree-sra.c (create_access_replacement): Ditto.
>        (get_replaced_param_substitute): Ditto.
>
> testsuite/
>        * gfortran.fortran-torture/compile/pr50260.f90: New test.
>
> Index: ipa-split.c
> ===================================================================
> --- ipa-split.c (revision 178408)
> +++ ipa-split.c (working copy)
> @@ -988,6 +988,9 @@ split_function (struct split_point *spli
>        arg = gimple_default_def (cfun, parm);
>        if (!arg)
>          {
> +           /* This parm wasn't used up to now, but is going to be used,
> +              hence register it.  */
> +           add_referenced_var (parm);
>            arg = make_ssa_name (parm, gimple_build_nop ());
>            set_default_def (parm, arg);
>          }
> Index: tree-ssa-phiopt.c
> ===================================================================
> --- tree-ssa-phiopt.c   (revision 178408)
> +++ tree-ssa-phiopt.c   (working copy)
> @@ -1269,10 +1269,7 @@ cond_store_replacement (basic_block midd
>   /* 2) Create a temporary where we can store the old content
>         of the memory touched by the store, if we need to.  */
>   if (!condstoretemp || TREE_TYPE (lhs) != TREE_TYPE (condstoretemp))
> -    {
> -      condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
> -      get_var_ann (condstoretemp);
> -    }
> +    condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
>   add_referenced_var (condstoretemp);
>
>   /* 3) Insert a load from the memory of the store to the temporary
> @@ -1355,10 +1352,7 @@ cond_if_else_store_replacement_1 (basic_
>   /* 2) Create a temporary where we can store the old content
>        of the memory touched by the store, if we need to.  */
>   if (!condstoretemp || TREE_TYPE (lhs) != TREE_TYPE (condstoretemp))
> -    {
> -      condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
> -      get_var_ann (condstoretemp);
> -    }
> +    condstoretemp = create_tmp_reg (TREE_TYPE (lhs), "cstore");
>   add_referenced_var (condstoretemp);
>
>   /* 3) Create a PHI node at the join block, with one argument
> Index: tree-ssa-pre.c
> ===================================================================
> --- tree-ssa-pre.c      (revision 178408)
> +++ tree-ssa-pre.c      (working copy)
> @@ -1399,7 +1399,7 @@ get_representative_for (const pre_expr e
>   if (!pretemp || exprtype != TREE_TYPE (pretemp))
>     {
>       pretemp = create_tmp_reg (exprtype, "pretmp");
> -      get_var_ann (pretemp);
> +      add_referenced_var (pretemp);
>     }
>
>   name = make_ssa_name (pretemp, gimple_build_nop ());
> @@ -3178,10 +3178,7 @@ create_expression_by_pieces (basic_block
>   /* Build and insert the assignment of the end result to the temporary
>      that we will return.  */
>   if (!pretemp || exprtype != TREE_TYPE (pretemp))
> -    {
> -      pretemp = create_tmp_reg (exprtype, "pretmp");
> -      get_var_ann (pretemp);
> -    }
> +    pretemp = create_tmp_reg (exprtype, "pretmp");
>
>   temp = pretemp;
>   add_referenced_var (temp);
> @@ -3441,10 +3438,7 @@ insert_into_preds_of_block (basic_block
>
>   /* Now build a phi for the new variable.  */
>   if (!prephitemp || TREE_TYPE (prephitemp) != type)
> -    {
> -      prephitemp = create_tmp_var (type, "prephitmp");
> -      get_var_ann (prephitemp);
> -    }
> +    prephitemp = create_tmp_var (type, "prephitmp");
>
>   temp = prephitemp;
>   add_referenced_var (temp);
> Index: tree-sra.c
> ===================================================================
> --- tree-sra.c  (revision 178408)
> +++ tree-sra.c  (working copy)
> @@ -1825,7 +1825,6 @@ create_access_replacement (struct access
>   tree repl;
>
>   repl = create_tmp_var (access->type, "SR");
> -  get_var_ann (repl);
>   add_referenced_var (repl);
>   if (rename)
>     mark_sym_for_renaming (repl);
> @@ -4106,7 +4105,6 @@ get_replaced_param_substitute (struct ip
>       DECL_NAME (repl) = get_identifier (pretty_name);
>       obstack_free (&name_obstack, pretty_name);
>
> -      get_var_ann (repl);
>       add_referenced_var (repl);
>       adj->new_ssa_base = repl;
>     }
> Index: testsuite/gfortran.fortran-torture/compile/pr50260.f90
> ===================================================================
> --- testsuite/gfortran.fortran-torture/compile/pr50260.f90      (revision 0)
> +++ testsuite/gfortran.fortran-torture/compile/pr50260.f90      (revision 0)
> @@ -0,0 +1,48 @@
> +MODULE cp_parser_methods
> +  INTEGER, PARAMETER :: default_string_length=80
> +  INTEGER, PARAMETER :: default_path_length=250
> +  TYPE ilist_type
> +     LOGICAL                              :: in_use
> +  END TYPE ilist_type
> +  TYPE cp_parser_type
> +     CHARACTER(LEN=default_path_length)             :: ifn
> +     INTEGER                                        :: icol,icol1,icol2
> +     TYPE(ilist_type), POINTER                      :: ilist
> +  END TYPE cp_parser_type
> +  TYPE cp_error_type
> +  END TYPE cp_error_type
> +CONTAINS
> +  FUNCTION cts(i) RESULT(res)
> +    CHARACTER(len=6)                         :: res
> +  END FUNCTION cts
> +  FUNCTION parser_location(parser,error) RESULT(res)
> +    TYPE(cp_parser_type), POINTER            :: parser
> +    TYPE(cp_error_type), INTENT(inout)       :: error
> +    CHARACTER(len=default_path_length+default_string_length)       :: res
> +    LOGICAL                                  :: failure
> +    IF (.NOT. failure) THEN
> +       res="file:'"//TRIM(parser%ifn)//"' line:"//cts(parser%icol)
> +    END IF
> +  END FUNCTION parser_location
> +  SUBROUTINE parser_get_integer(parser,at_end, error)
> +    TYPE(cp_parser_type), POINTER            :: parser
> +    TYPE(cp_error_type), INTENT(inout)       :: error
> +    LOGICAL                                  :: failure, my_at_end
> +    IF (.NOT.failure) THEN
> +       IF (.NOT.parser%ilist%in_use) THEN
> +          CALL cp_assert("A"// TRIM(parser_location(parser,error)))
> +       END IF
> +    END IF
> +  END SUBROUTINE parser_get_integer
> +  SUBROUTINE parser_get_string(parser,at_end,error)
> +    TYPE(cp_parser_type), POINTER            :: parser
> +    LOGICAL, INTENT(out), OPTIONAL           :: at_end
> +    TYPE(cp_error_type), INTENT(inout)       :: error
> +    LOGICAL                                  :: failure, my_at_end
> +    IF (.NOT.failure) THEN
> +       IF (PRESENT(at_end)) THEN
> +          CALL cp_assert("s"//TRIM(parser_location(parser,error)))
> +       END IF
> +    END IF
> +  END SUBROUTINE parser_get_string
> +END MODULE cp_parser_methods
>