diff mbox

PR28901 Add two levels for -Wunused-const-variable.

Message ID 1456217637.7770.108.camel@redhat.com
State New
Headers show

Commit Message

Mark Wielaard Feb. 23, 2016, 8:53 a.m. UTC 
On Tue, 2016-02-23 at 09:26 +0100, Jakub Jelinek wrote:
> On Tue, Feb 23, 2016 at 08:55:40AM +0100, Mark Wielaard wrote:
> > On Mon, 2016-02-22 at 19:20 -0800, H.J. Lu wrote:
> > > It caused:
> > > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69911
> > 
> > Apologies. Apparently main_input_filename can be NULL. I am not entirely
> > sure when that happens. Or how I failed to see that test failure. I
> > think I didn't have java enabled, causing libffi to be skipped.
> > 
> > I am testing this patch that skips the test in that case:
> 
> Are you sure that is the problem?

No :) I was still bootstrapping. I did see it didn't solve the issue but
hadn't understood why yet...

> I think it doesn't hurt to check for non-NULL main_input_filename, perhaps
> some non-c-family languages might not set it, and this is in generic code,
> but at least on the gcc.target/i386/iamcu/test_passing_structs.c testcase and on one
> randomly selected libffi testcase I see the ICE from completely different
> reason - what is NULL is DECL_SOURCE_FILE (decl).

Thanks, that makes more sense than my first hypothesis.

> We are not really going to warn about this anyway, e.g. because
> it is DECL_ARTIFICIAL, but that is checked only in a much later
> condition.  So, I think you should check also for NULL DECL_SOURCE_FILE
> (and treat it as possibly in a header).  Unfortunately
> DECL_SOURCE_FILE involves a function call, so you might want to cache
> it in some automatic variable.
> 		&& (warn_unused_const_variable == 2
> 		    || (main_input_filename != NULL
> 		        && (decl_file = DECL_SOURCE_FILE (decl)) != NULL
> 			&& filename_cmp (main_input_filename,
> 					 decl_file) == 0))))

That looks right. Now testing:

Comments

Jakub Jelinek Feb. 23, 2016, 8:56 a.m. UTC | #1 
On Tue, Feb 23, 2016 at 09:53:57AM +0100, Mark Wielaard wrote:
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,10 @@
> +2016-02-23  Mark Wielaard  <mjw@redhat.com>
> +	    Jakub Jelinek  <jakub@redhat.com>
> +
> +	PR c/69911
> +	* cgraphunit.c (check_global_declaration): Check main_input_filename
> +	and DECL_SOURCE_FILE are not NULL.
> +
>  2016-02-20  Mark Wielaard  <mjw@redhat.com>

This is ok for trunk if it passes testing.  Thanks.

> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> index 27a073a..8b3fddc 100644
> --- a/gcc/cgraphunit.c
> +++ b/gcc/cgraphunit.c
> @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets,
>  static void
>  check_global_declaration (symtab_node *snode)
>  {
> +  const char *decl_file;
>    tree decl = snode->decl;
>  
>    /* Warn about any function declared static but not defined.  We don't
> @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode)
>         || (((warn_unused_variable && ! TREE_READONLY (decl))
>  	    || (warn_unused_const_variable > 0 && TREE_READONLY (decl)
>  		&& (warn_unused_const_variable == 2
> -		    || filename_cmp (main_input_filename,
> -				     DECL_SOURCE_FILE (decl)) == 0)))
> +		    || (main_input_filename != NULL
> +			&& (decl_file = DECL_SOURCE_FILE (decl)) != NULL
> +			&& filename_cmp (main_input_filename,
> +					 decl_file) == 0))))
>  	   && TREE_CODE (decl) == VAR_DECL))
>        && ! DECL_IN_SYSTEM_HEADER (decl)
>        && ! snode->referred_to_p (/*include_self=*/false)

	Jakub
Manuel López-Ibáñez Feb. 23, 2016, 9:51 a.m. UTC | #2 
On 23/02/16 08:56, Jakub Jelinek wrote:
> On Tue, Feb 23, 2016@09:53:57AM +0100, Mark Wielaard wrote:
>> --- a/gcc/ChangeLog
>> +++ b/gcc/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2016-02-23  Mark Wielaard  <mjw@redhat.com>
>> +	    Jakub Jelinek  <jakub@redhat.com>
>> +
>> +	PR c/69911
>> +	* cgraphunit.c (check_global_declaration): Check main_input_filename
>> +	and DECL_SOURCE_FILE are not NULL.
>> +
>>   2016-02-20  Mark Wielaard  <mjw@redhat.com>
>
> This is ok for trunk if it passes testing.  Thanks.
>
>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
>> index 27a073a..8b3fddc 100644
>> --- a/gcc/cgraphunit.c
>> +++ b/gcc/cgraphunit.c
>> @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets,
>>   static void
>>   check_global_declaration (symtab_node *snode)
>>   {
>> +  const char *decl_file;
>>     tree decl = snode->decl;
>>
>>     /* Warn about any function declared static but not defined.  We don't
>> @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode)
>>          || (((warn_unused_variable && ! TREE_READONLY (decl))
>>   	    || (warn_unused_const_variable > 0 && TREE_READONLY (decl)
>>   		&& (warn_unused_const_variable == 2
>> -		    || filename_cmp (main_input_filename,
>> -				     DECL_SOURCE_FILE (decl)) == 0)))
>> +		    || (main_input_filename != NULL
>> +			&& (decl_file = DECL_SOURCE_FILE (decl)) != NULL
>> +			&& filename_cmp (main_input_filename,
>> +					 decl_file) == 0))))


Can we please please please hide this ugliness behind an (inline?) function 
such as bool in_main_file_at (location_t) in input.[ch]? The condition here is 
quickly becoming unreadable.

Also because in the future somebody would want to re-implement this using 
MAIN_FILE_P() from line-map.h, which is faster.

Cheers,

	Manuel.
Jakub Jelinek Feb. 23, 2016, 9:55 a.m. UTC | #3 
On Tue, Feb 23, 2016 at 09:51:05AM +0000, Manuel López-Ibáñez wrote:
> >>diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> >>index 27a073a..8b3fddc 100644
> >>--- a/gcc/cgraphunit.c
> >>+++ b/gcc/cgraphunit.c
> >>@@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets,
> >>  static void
> >>  check_global_declaration (symtab_node *snode)
> >>  {
> >>+  const char *decl_file;
> >>    tree decl = snode->decl;
> >>
> >>    /* Warn about any function declared static but not defined.  We don't
> >>@@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode)
> >>         || (((warn_unused_variable && ! TREE_READONLY (decl))
> >>  	    || (warn_unused_const_variable > 0 && TREE_READONLY (decl)
> >>  		&& (warn_unused_const_variable == 2
> >>-		    || filename_cmp (main_input_filename,
> >>-				     DECL_SOURCE_FILE (decl)) == 0)))
> >>+		    || (main_input_filename != NULL
> >>+			&& (decl_file = DECL_SOURCE_FILE (decl)) != NULL
> >>+			&& filename_cmp (main_input_filename,
> >>+					 decl_file) == 0))))
> 
> 
> Can we please please please hide this ugliness behind an (inline?) function
> such as bool in_main_file_at (location_t) in input.[ch]? The condition here
> is quickly becoming unreadable.
> 
> Also because in the future somebody would want to re-implement this using
> MAIN_FILE_P() from line-map.h, which is faster.

I'm not against that, but I'd prefer if it is done incrementally, we don't
want to keep the trunk broken too much for too long.  So if Mark is already
testing this patch, IMHO it is better to commit it in this shape sooner.

	Jakub
Mark Wielaard Feb. 23, 2016, 12:10 p.m. UTC | #4 
On Tue, 2016-02-23 at 09:51 +0000, Manuel López-Ibáñez wrote:
> On 23/02/16 08:56, Jakub Jelinek wrote:
> > On Tue, Feb 23, 2016@09:53:57AM +0100, Mark Wielaard wrote:
> >> --- a/gcc/ChangeLog
> >> +++ b/gcc/ChangeLog
> >> @@ -1,3 +1,10 @@
> >> +2016-02-23  Mark Wielaard  <mjw@redhat.com>
> >> +	    Jakub Jelinek  <jakub@redhat.com>
> >> +
> >> +	PR c/69911
> >> +	* cgraphunit.c (check_global_declaration): Check main_input_filename
> >> +	and DECL_SOURCE_FILE are not NULL.
> >> +
> >>   2016-02-20  Mark Wielaard  <mjw@redhat.com>
> >
> > This is ok for trunk if it passes testing.  Thanks.
> >
> >> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
> >> index 27a073a..8b3fddc 100644
> >> --- a/gcc/cgraphunit.c
> >> +++ b/gcc/cgraphunit.c
> >> @@ -917,6 +917,7 @@ walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets,
> >>   static void
> >>   check_global_declaration (symtab_node *snode)
> >>   {
> >> +  const char *decl_file;
> >>     tree decl = snode->decl;
> >>
> >>     /* Warn about any function declared static but not defined.  We don't
> >> @@ -944,8 +945,10 @@ check_global_declaration (symtab_node *snode)
> >>          || (((warn_unused_variable && ! TREE_READONLY (decl))
> >>   	    || (warn_unused_const_variable > 0 && TREE_READONLY (decl)
> >>   		&& (warn_unused_const_variable == 2
> >> -		    || filename_cmp (main_input_filename,
> >> -				     DECL_SOURCE_FILE (decl)) == 0)))
> >> +		    || (main_input_filename != NULL
> >> +			&& (decl_file = DECL_SOURCE_FILE (decl)) != NULL
> >> +			&& filename_cmp (main_input_filename,
> >> +					 decl_file) == 0))))
> 
> 
> Can we please please please hide this ugliness behind an (inline?) function 
> such as bool in_main_file_at (location_t) in input.[ch]? The condition here is 
> quickly becoming unreadable.
> 
> Also because in the future somebody would want to re-implement this using 
> MAIN_FILE_P() from line-map.h, which is faster.

Sorry, I pushed the minimum fix. I am too embarrassed already for
breaking stuff to risk refactoring anything and making another dumb
mistake. (My mistake was only checking the top-level compiler .sum
files, I had forgotten to check the library ones).

It might be a good idea to file a bug for this and write a patch to
submit during next stage one if you believe this is a useful cleanup.

Cheers,

Mark
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 49f6c25..e9e1aab 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@ 
+2016-02-23  Mark Wielaard  <mjw@redhat.com>
+	    Jakub Jelinek  <jakub@redhat.com>
+
+	PR c/69911
+	* cgraphunit.c (check_global_declaration): Check main_input_filename
+	and DECL_SOURCE_FILE are not NULL.
+
 2016-02-20  Mark Wielaard  <mjw@redhat.com>
 
 	PR c/28901
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 27a073a..8b3fddc 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -917,6 +917,7 @@  walk_polymorphic_call_targets (hash_set<void *> *reachable_call_targets,
 static void
 check_global_declaration (symtab_node *snode)
 {
+  const char *decl_file;
   tree decl = snode->decl;
 
   /* Warn about any function declared static but not defined.  We don't
@@ -944,8 +945,10 @@  check_global_declaration (symtab_node *snode)
        || (((warn_unused_variable && ! TREE_READONLY (decl))
 	    || (warn_unused_const_variable > 0 && TREE_READONLY (decl)
 		&& (warn_unused_const_variable == 2
-		    || filename_cmp (main_input_filename,
-				     DECL_SOURCE_FILE (decl)) == 0)))
+		    || (main_input_filename != NULL
+			&& (decl_file = DECL_SOURCE_FILE (decl)) != NULL
+			&& filename_cmp (main_input_filename,
+					 decl_file) == 0))))
 	   && TREE_CODE (decl) == VAR_DECL))
       && ! DECL_IN_SYSTEM_HEADER (decl)
       && ! snode->referred_to_p (/*include_self=*/false)