Fix PR86575

Hi,

our warning code sometimes adds locations to statement which didn't have 
them before, which can in turn lead to code changes (here only label 
numbers change).  It seems better to not do that from warning code, and 
here it's easy to do: just return the location we want to use for 
warnings, don't change it in the statement itself.

Regstrapped on x86-64, okay for trunk?


Ciao,
Michael.

	PR middle-end/86575
	* gimplify.c (collect_fallthrough_labels): Add new argument,
	return location via that, don't modify statements.
	(warn_implicit_fallthrough_r): Adjust call, don't use
	statement location directly.

On Wed, Nov 14, 2018 at 3:51 PM Michael Matz <matz@suse.de> wrote:
>
> Hi,
>
> our warning code sometimes adds locations to statement which didn't have
> them before, which can in turn lead to code changes (here only label
> numbers change).  It seems better to not do that from warning code, and
> here it's easy to do: just return the location we want to use for
> warnings, don't change it in the statement itself.
>
> Regstrapped on x86-64, okay for trunk?

OK.

Richard.

>
> Ciao,
> Michael.
>
>         PR middle-end/86575
>         * gimplify.c (collect_fallthrough_labels): Add new argument,
>         return location via that, don't modify statements.
>         (warn_implicit_fallthrough_r): Adjust call, don't use
>         statement location directly.
>
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 509fc2f3f5be..22dff0e546c9 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1938,10 +1938,12 @@ last_stmt_in_scope (gimple *stmt)
>
>  static gimple *
>  collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
> -                           auto_vec <struct label_entry> *labels)
> +                           auto_vec <struct label_entry> *labels,
> +                           location_t *prevloc)
>  {
>    gimple *prev = NULL;
>
> +  *prevloc = UNKNOWN_LOCATION;
>    do
>      {
>        if (gimple_code (gsi_stmt (*gsi_p)) == GIMPLE_BIND)
> @@ -1978,7 +1980,7 @@ collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
>               /* It might be a label without a location.  Use the
>                  location of the scope then.  */
>               if (!gimple_has_location (prev))
> -               gimple_set_location (prev, bind_loc);
> +               *prevloc = bind_loc;
>             }
>           gsi_next (gsi_p);
>           continue;
> @@ -2061,6 +2063,8 @@ collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
>          && (gimple_code (gsi_stmt (*gsi_p)) != GIMPLE_LABEL
>              || !gimple_has_location (gsi_stmt (*gsi_p))));
>
> +  if (prev && gimple_has_location (prev))
> +    *prevloc = gimple_location (prev);
>    return prev;
>  }
>
> @@ -2157,7 +2161,8 @@ warn_implicit_fallthrough_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>
>         /* Vector of labels that fall through.  */
>         auto_vec <struct label_entry> labels;
> -       gimple *prev = collect_fallthrough_labels (gsi_p, &labels);
> +       location_t prevloc;
> +       gimple *prev = collect_fallthrough_labels (gsi_p, &labels, &prevloc);
>
>         /* There might be no more statements.  */
>         if (gsi_end_p (*gsi_p))
> @@ -2185,8 +2190,8 @@ warn_implicit_fallthrough_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>                      /* Try to be clever and don't warn when the statement
>                         can't actually fall through.  */
>                      && gimple_stmt_may_fallthru (prev)
> -                    && gimple_has_location (prev))
> -             warned_p = warning_at (gimple_location (prev),
> +                    && prevloc != UNKNOWN_LOCATION)
> +             warned_p = warning_at (prevloc,
>                                      OPT_Wimplicit_fallthrough_,
>                                      "this statement may fall through");
>             if (warned_p)

On Wed, Nov 14, 2018 at 02:51:45PM +0000, Michael Matz wrote:
> Hi,
> 
> our warning code sometimes adds locations to statement which didn't have 
> them before, which can in turn lead to code changes (here only label 
> numbers change).  It seems better to not do that from warning code, and 
> here it's easy to do: just return the location we want to use for 
> warnings, don't change it in the statement itself.
> 
> Regstrapped on x86-64, okay for trunk?
> 
> 
> Ciao,
> Michael.
> 
> 	PR middle-end/86575
> 	* gimplify.c (collect_fallthrough_labels): Add new argument,
> 	return location via that, don't modify statements.
> 	(warn_implicit_fallthrough_r): Adjust call, don't use
> 	statement location directly.
> 
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 509fc2f3f5be..22dff0e546c9 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1938,10 +1938,12 @@ last_stmt_in_scope (gimple *stmt)
>  
>  static gimple *
>  collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
> -			    auto_vec <struct label_entry> *labels)
> +			    auto_vec <struct label_entry> *labels,
> +			    location_t *prevloc)
>  {
>    gimple *prev = NULL;

Looks good, thanks, though PREVLOC should probably be described in the comment.

Marek

Hi,

On Wed, 14 Nov 2018, Marek Polacek wrote:

> >  static gimple *
> >  collect_fallthrough_labels (gimple_stmt_iterator *gsi_p,
> > -			    auto_vec <struct label_entry> *labels)
> > +			    auto_vec <struct label_entry> *labels,
> > +			    location_t *prevloc)
> >  {
> >    gimple *prev = NULL;
> 
> Looks good, thanks, though PREVLOC should probably be described in the 
> comment.

Gah.  And I thought about exactly this shortly before commiting, and then 
forgot :)  Fixed in svn.


Ciao,
Michael.