Fix PR86575

Message ID alpine.LSU.2.21.1811141449390.18790@wotan.suse.de
State New
Headers show
Series
  • Fix PR86575
Related show

Commit Message

Michael Matz Nov. 14, 2018, 2:51 p.m.
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.

Comments

Richard Biener Nov. 14, 2018, 3:03 p.m. | #1
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)
Marek Polacek Nov. 14, 2018, 3:59 p.m. | #2
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
Michael Matz Nov. 14, 2018, 4:03 p.m. | #3
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.

Patch

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)