diff mbox

[C] Fix PR57258: unused variable warning is emitted for volatile variables

Message ID CADNgcEzv0gdtADDwE1V4yn1ruwVE32eOocXk_HmLtcPhDiWu3w@mail.gmail.com
State New
Headers show

Commit Message

Mingjie Xing Nov. 7, 2013, 3:53 a.m. UTC
2013/11/6 Richard Biener <richard.guenther@gmail.com>:
> You miss a testcase.
>
> Also why should the warning be omitted for unused automatic
> volatile variables?  They cannot be used in any way.
>
> Richard.

Thanks.  I've updated the patch with a test case.

c/ChangeLog

        PR 57258
        * c-decl.c (pop_scope): Don't emit unused variable warnings for
        accessed volatile variables.

testsuite/ChangeLog

        * gcc.dg/Wunused-var-4.c: New test.

Mingjie

Comments

Bin.Cheng Nov. 7, 2013, 7:37 a.m. UTC | #1
On Thu, Nov 7, 2013 at 11:53 AM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>> You miss a testcase.
>>
>> Also why should the warning be omitted for unused automatic
>> volatile variables?  They cannot be used in any way.
>>
>> Richard.
>
> Thanks.  I've updated the patch with a test case.
>
> c/ChangeLog
>
>         PR 57258

Should be:
           PR c/57258

Thanks,
bin
>         * c-decl.c (pop_scope): Don't emit unused variable warnings for
>         accessed volatile variables.
>
> testsuite/ChangeLog
>
>         * gcc.dg/Wunused-var-4.c: New test.
>
> Mingjie
Joey Ye Nov. 7, 2013, 9:42 a.m. UTC | #2
-  warning_at (DECL_SOURCE_LOCATION (p),
-       OPT_Wunused_but_set_variable,
-       "variable %qD set but not used", p);
+  {
+    if (!TREE_THIS_VOLATILE (p))
+      warning_at (DECL_SOURCE_LOCATION (p),
+    OPT_Wunused_but_set_variable,
+    "variable %qD set but not used", p);
+  }
I'd prefer

        else if (DECL_CONTEXT (p) == current_function_decl
                    && !TREE_THIS_VOLATILE (p))

which concises the logic and avoids indent change

Thanks,
Joey

On Thu, Nov 7, 2013 at 3:37 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Thu, Nov 7, 2013 at 11:53 AM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
>> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>>> You miss a testcase.
>>>
>>> Also why should the warning be omitted for unused automatic
>>> volatile variables?  They cannot be used in any way.
>>>
>>> Richard.
>>
>> Thanks.  I've updated the patch with a test case.
>>
>> c/ChangeLog
>>
>>         PR 57258
>
> Should be:
>            PR c/57258
>
> Thanks,
> bin
>>         * c-decl.c (pop_scope): Don't emit unused variable warnings for
>>         accessed volatile variables.
>>
>> testsuite/ChangeLog
>>
>>         * gcc.dg/Wunused-var-4.c: New test.
>>
>> Mingjie
>
>
>
> --
> Best Regards.
Joseph Myers Nov. 7, 2013, 1:36 p.m. UTC | #3
On Thu, 7 Nov 2013, Mingjie Xing wrote:

> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
> > You miss a testcase.
> >
> > Also why should the warning be omitted for unused automatic
> > volatile variables?  They cannot be used in any way.
> >
> > Richard.
> 
> Thanks.  I've updated the patch with a test case.

You don't seem to have answered Richard's question about why the change is 
desirable in the first place.
Mingjie Xing Nov. 8, 2013, 1:13 a.m. UTC | #4
2013/11/7 Joseph S. Myers <joseph@codesourcery.com>:
> On Thu, 7 Nov 2013, Mingjie Xing wrote:
>
>> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>> > You miss a testcase.
>> >
>> > Also why should the warning be omitted for unused automatic
>> > volatile variables?  They cannot be used in any way.
>> >
>> > Richard.
>>
>> Thanks.  I've updated the patch with a test case.
>
> You don't seem to have answered Richard's question about why the change is
> desirable in the first place.

Well, it is my understanding that the warning should be emitted for a
volatile variable only if it is not accessed.  Initialization means
accessing, even though it is not used anywhere.

Mingjie
Mike Stump Nov. 8, 2013, 9:03 a.m. UTC | #5
On Nov 7, 2013, at 5:13 PM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
> Well, it is my understanding that the warning should be emitted for a
> volatile variable only if it is not accessed.  Initialization means
> accessing, even though it is not used anywhere.

Let me try.  A warning is useful, if there is no way a conforming program can tell that the variable exists or not.  So, the question is, how can you notice the variable?  Answer, there is no way, so, there is no utility in having the variable.  The warning is to tell the user to remove the dead variable.
Bin.Cheng Nov. 8, 2013, 9:32 a.m. UTC | #6
On Fri, Nov 8, 2013 at 5:03 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Nov 7, 2013, at 5:13 PM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
>> Well, it is my understanding that the warning should be emitted for a
>> volatile variable only if it is not accessed.  Initialization means
>> accessing, even though it is not used anywhere.
>
> Let me try.  A warning is useful, if there is no way a conforming program can tell that the variable exists or not.  So, the question is, how can you notice the variable?  Answer, there is no way, so, there is no utility in having the variable.  The warning is to tell the user to remove the dead variable.

I am sort of lost.  The bug is straightforward and it suggests the
volatile variable is actually used if it is declared with an
initialization, and GCC should not emit warning message for it.

The patch is fixing this, right?  Any explanation?

Thanks,
bin
Mike Stump Nov. 8, 2013, 9:39 a.m. UTC | #7
On Nov 8, 2013, at 1:32 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:

> On Fri, Nov 8, 2013 at 5:03 PM, Mike Stump <mikestump@comcast.net> wrote:
>> On Nov 7, 2013, at 5:13 PM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
>>> Well, it is my understanding that the warning should be emitted for a
>>> volatile variable only if it is not accessed.  Initialization means
>>> accessing, even though it is not used anywhere.
>> 
>> Let me try.  A warning is useful, if there is no way a conforming program can tell that the variable exists or not.  So, the question is, how can you notice the variable?  Answer, there is no way, so, there is no utility in having the variable.  The warning is to tell the user to remove the dead variable.
> 
> I am sort of lost.

I can try again.  Begin your sentence, the important utility of this construct is demonstrated by the following code:

See if you can complete it.  If not, then, then there is no utility.  The warning says, there is no utility.  This isn't a theoretic thing, it is an engineering thing.
Richard Biener Nov. 8, 2013, 10:08 a.m. UTC | #8
On Fri, Nov 8, 2013 at 10:39 AM, Mike Stump <mikestump@comcast.net> wrote:
>
> On Nov 8, 2013, at 1:32 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>
>> On Fri, Nov 8, 2013 at 5:03 PM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Nov 7, 2013, at 5:13 PM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
>>>> Well, it is my understanding that the warning should be emitted for a
>>>> volatile variable only if it is not accessed.  Initialization means
>>>> accessing, even though it is not used anywhere.
>>>
>>> Let me try.  A warning is useful, if there is no way a conforming program can tell that the variable exists or not.  So, the question is, how can you notice the variable?  Answer, there is no way, so, there is no utility in having the variable.  The warning is to tell the user to remove the dead variable.
>>
>> I am sort of lost.
>
> I can try again.  Begin your sentence, the important utility of this construct is demonstrated by the following code:
>
> See if you can complete it.  If not, then, then there is no utility.  The warning says, there is no utility.  This isn't a theoretic thing, it is an engineering thing.

Yeah, and as opposed to the non-volatile case removing the volatile
set-but-unused variable even reduces code size!

Richard.
Bin.Cheng Nov. 8, 2013, 10:25 a.m. UTC | #9
On Fri, Nov 8, 2013 at 5:39 PM, Mike Stump <mikestump@comcast.net> wrote:
>
> On Nov 8, 2013, at 1:32 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>
>> On Fri, Nov 8, 2013 at 5:03 PM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Nov 7, 2013, at 5:13 PM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
>>>> Well, it is my understanding that the warning should be emitted for a
>>>> volatile variable only if it is not accessed.  Initialization means
>>>> accessing, even though it is not used anywhere.
>>>
>>> Let me try.  A warning is useful, if there is no way a conforming program can tell that the variable exists or not.  So, the question is, how can you notice the variable?  Answer, there is no way, so, there is no utility in having the variable.  The warning is to tell the user to remove the dead variable.
>>
>> I am sort of lost.
>
> I can try again.  Begin your sentence, the important utility of this construct is demonstrated by the following code:
>
> See if you can complete it.  If not, then, then there is no utility.  The warning says, there is no utility.  This isn't a theoretic thing, it is an engineering thing.

Thanks for elaborating.  The warning message is actually for no-use of
variable, and it has few things to do with whether it's accessed or
not.

Thanks,
bin
Mingjie Xing Nov. 8, 2013, 10:35 a.m. UTC | #10
Oops, if it is not a bug, please close the report
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57258

Thanks,
Mingjie

2013/11/8 Richard Biener <richard.guenther@gmail.com>:
> On Fri, Nov 8, 2013 at 10:39 AM, Mike Stump <mikestump@comcast.net> wrote:
>>
>> On Nov 8, 2013, at 1:32 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
>>
>>> On Fri, Nov 8, 2013 at 5:03 PM, Mike Stump <mikestump@comcast.net> wrote:
>>>> On Nov 7, 2013, at 5:13 PM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
>>>>> Well, it is my understanding that the warning should be emitted for a
>>>>> volatile variable only if it is not accessed.  Initialization means
>>>>> accessing, even though it is not used anywhere.
>>>>
>>>> Let me try.  A warning is useful, if there is no way a conforming program can tell that the variable exists or not.  So, the question is, how can you notice the variable?  Answer, there is no way, so, there is no utility in having the variable.  The warning is to tell the user to remove the dead variable.
>>>
>>> I am sort of lost.
>>
>> I can try again.  Begin your sentence, the important utility of this construct is demonstrated by the following code:
>>
>> See if you can complete it.  If not, then, then there is no utility.  The warning says, there is no utility.  This isn't a theoretic thing, it is an engineering thing.
>
> Yeah, and as opposed to the non-volatile case removing the volatile
> set-but-unused variable even reduces code size!
>
> Richard.
Mike Stump Nov. 8, 2013, 10:36 a.m. UTC | #11
On Nov 8, 2013, at 2:25 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> Thanks for elaborating.  The warning message is actually for no-use of
> variable, and it has few things to do with whether it's accessed or
> not.

I disagree.  If you examine why the warning was put in, you realize it was put in so that lazy programmers can find dead things.  Why do they want to do that, to remove the dead things.  This construct is a dead thing.  If you disagree, you'd have to say why the warning was produced.
Mike Stump Nov. 8, 2013, 11:01 a.m. UTC | #12
On Nov 8, 2013, at 2:35 AM, Mingjie Xing <mingjie.xing@gmail.com> wrote:
> Oops, if it is not a bug, please close the report
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57258

Well, I've stated my position.  I can be swayed by a good argument, if someone has one.  I'd give people a chance to weigh in if they can think of one.  A global variable with linkage is completely different, as the linker can grab it, dlsym can grab it, and who knows…  while a cleaver (Ian type of cleaver) person could grab an automatic variable, the difficulty is enormous and doing that would be exceedingly odd.  I don't know of any coding style where people do that sort of thing.  Reflection comes the closest; dynamic bindings (a la lisp) I guess comes second; shudder.  I'd make them mark it with the used attribute, since they're really weird.
diff mbox

Patch

Index: c/c-decl.c
===================================================================
--- c/c-decl.c	(revision 204285)
+++ c/c-decl.c	(working copy)
@@ -1187,11 +1187,17 @@  pop_scope (void)
 	      && scope != external_scope)
 	    {
 	      if (!TREE_USED (p))
-		warning (OPT_Wunused_variable, "unused variable %q+D", p);
+		{
+		  if (!TREE_THIS_VOLATILE (p) || !DECL_INITIAL (p))
+		    warning (OPT_Wunused_variable, "unused variable %q+D", p);
+		}
 	      else if (DECL_CONTEXT (p) == current_function_decl)
-		warning_at (DECL_SOURCE_LOCATION (p),
-			    OPT_Wunused_but_set_variable,
-			    "variable %qD set but not used", p);
+		{
+		  if (!TREE_THIS_VOLATILE (p))
+		    warning_at (DECL_SOURCE_LOCATION (p),
+				OPT_Wunused_but_set_variable,
+				"variable %qD set but not used", p);
+		}
 	    }
 
 	  if (b->inner_comp)