diff mbox

restore -Wunused-variable on a typedef'd variable in a function template (PR 79548)

Message ID 28dd4f25-b208-52af-af1e-7bc5e36db557@gmail.com
State New
Headers show

Commit Message

Martin Sebor Feb. 18, 2017, 1:07 a.m. UTC 
The attached patch fixes bug 79548 - [5/6/7 Regression] missing
-Wunused-variable on a typedef'd variable in a function template,
most likely broken by the introduction of -Wunused-local-typedefs.

While testing the patch I came across a couple of other bugs:

   79585 - spurious -Wunused-variable on a pointer with attribute
           unused in function template
and

   79586 - missing -Wdeprecated depending on position of attribute

The test I added for 79548 fails two assertions due to the first
of these two so I xfailed them.  The second doesn't have an impact
on it.  Neither of these is a regression so I didn't try to fix them.

Martin

Comments

Jason Merrill Feb. 21, 2017, 6:08 p.m. UTC | #1 
On 02/17/2017 05:07 PM, Martin Sebor wrote:
> 	* decl.c (poplevel): Avoid diagnosing entities declared with
> 	attribute unused.

This change is OK.

> 	(initialize_local_var): Do not consider the type of a variable
> 	when determining whether or not it's used.

This is not; the documentation for attribute unused says,

When attached to a type (including a @code{union} or a @code{struct}),
this attribute means that variables of that type are meant to appear
possibly unused.  GCC does not produce a warning for any variables of
that type, even if the variable appears to do nothing.  This is often
the case with lock or thread classes, which are usually defined and then
not referenced, but contain constructors and destructors that have
nontrivial bookkeeping functions.

So a TREE_USED type should imply TREE_USED on variables of that type.

Jason
Martin Sebor Feb. 21, 2017, 7 p.m. UTC | #2 
On 02/21/2017 11:08 AM, Jason Merrill wrote:
> On 02/17/2017 05:07 PM, Martin Sebor wrote:
>>     * decl.c (poplevel): Avoid diagnosing entities declared with
>>     attribute unused.
>
> This change is OK.
>
>>     (initialize_local_var): Do not consider the type of a variable
>>     when determining whether or not it's used.
>
> This is not; the documentation for attribute unused says,
>
> When attached to a type (including a @code{union} or a @code{struct}),
> this attribute means that variables of that type are meant to appear
> possibly unused.  GCC does not produce a warning for any variables of
> that type, even if the variable appears to do nothing.  This is often
> the case with lock or thread classes, which are usually defined and then
> not referenced, but contain constructors and destructors that have
> nontrivial bookkeeping functions.
>
> So a TREE_USED type should imply TREE_USED on variables of that type.

Yes, I'm familiar with the effect of the attribute on types but
in my testing this change doesn't affect how it works (i.e., it
passes a full bootstrap and regression tests and I haven't been
able to construct a failing test case.)  It looks like that's
because TREE_USED(decl) is already set here for a decl whose
type is declared attribute used.

While trying to come up with a test case to exercise the scenario
you describe I see that current trunk doesn't handle it correctly
but the patch just happens to fix that too.  For example:

template <class T>
void f ()
{
   T t;   // trunk warns for f<int> (good)

   typedef T U;
   U u;   // trunk doesn't warn for f<int> (bug 79548)
}

template void f<int>();

struct __attribute__ ((unused)) S { };

template void f<S>();   // no warnings here (good)

void g ()
{
   S s;

   typedef S T;
   T t;   // trunk warns here (new bug), doesn't with the patch
}

In the test case above, TREE_USED (TREE_TYPE (decl)) is set for
t in g() so trunk sets it on t too and warbs.  The patch doesn't
and so it doesn't warn as expected.

But it's entirely possible I'm missing a case.  Do you have
a suggestion for a test that trunk handles correctly but that
fails with the patch?

Thanks
Martin
Jason Merrill Feb. 21, 2017, 8:32 p.m. UTC | #3 
On Tue, Feb 21, 2017 at 11:00 AM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/21/2017 11:08 AM, Jason Merrill wrote:
>>
>> On 02/17/2017 05:07 PM, Martin Sebor wrote:
>>>
>>>     * decl.c (poplevel): Avoid diagnosing entities declared with
>>>     attribute unused.
>>
>>
>> This change is OK.
>>
>>>     (initialize_local_var): Do not consider the type of a variable
>>>     when determining whether or not it's used.
>>
>>
>> This is not; the documentation for attribute unused says,
>>
>> When attached to a type (including a @code{union} or a @code{struct}),
>> this attribute means that variables of that type are meant to appear
>> possibly unused.  GCC does not produce a warning for any variables of
>> that type, even if the variable appears to do nothing.  This is often
>> the case with lock or thread classes, which are usually defined and then
>> not referenced, but contain constructors and destructors that have
>> nontrivial bookkeeping functions.
>>
>> So a TREE_USED type should imply TREE_USED on variables of that type.
>
> Yes, I'm familiar with the effect of the attribute on types but
> in my testing this change doesn't affect how it works (i.e., it
> passes a full bootstrap and regression tests and I haven't been
> able to construct a failing test case.)  It looks like that's
> because TREE_USED(decl) is already set here for a decl whose
> type is declared attribute used.
>
> While trying to come up with a test case to exercise the scenario
> you describe I see that current trunk doesn't handle it correctly
> but the patch just happens to fix that too.  For example:
>
> template <class T>
> void f ()
> {
>   T t;   // trunk warns for f<int> (good)
>
>   typedef T U;
>   U u;   // trunk doesn't warn for f<int> (bug 79548)
> }
>
> template void f<int>();
>
> struct __attribute__ ((unused)) S { };
>
> template void f<S>();   // no warnings here (good)
>
> void g ()
> {
>   S s;
>
>   typedef S T;
>   T t;   // trunk warns here (new bug), doesn't with the patch
> }
>
> In the test case above, TREE_USED (TREE_TYPE (decl)) is set for
> t in g() so trunk sets it on t too and warbs.  The patch doesn't
> and so it doesn't warn as expected.
>
> But it's entirely possible I'm missing a case.  Do you have
> a suggestion for a test that trunk handles correctly but that
> fails with the patch?

Ah, I see, your patch changes attribute unused handling for local
variables from tracking TREE_USED to lookup_attribute.  I'm not
opposed to this change, but I'd like to understand why the TREE_USED
handling wasn't working.

Jason
Martin Sebor Feb. 22, 2017, 12:27 a.m. UTC | #4 
> Ah, I see, your patch changes attribute unused handling for local
> variables from tracking TREE_USED to lookup_attribute.  I'm not
> opposed to this change, but I'd like to understand why the TREE_USED
> handling wasn't working.

In the test case in the bug:

   template <class T>
   void g ()
   {
     T t;   // warning, ok

     typedef T U;
     U u;   // no warning, bug
   }

   template void g<int>();

both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
so the function doesn't set already_used or TREE_USED(t) and we get
a warning as expected.

But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
(to implement -Wunused-local-typedefs),  initialize_local_var then
sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
the warning.

Martin
Jason Merrill Feb. 22, 2017, 6:02 p.m. UTC | #5 
On Tue, Feb 21, 2017 at 4:27 PM, Martin Sebor <msebor@gmail.com> wrote:
>> Ah, I see, your patch changes attribute unused handling for local
>> variables from tracking TREE_USED to lookup_attribute.  I'm not
>> opposed to this change, but I'd like to understand why the TREE_USED
>> handling wasn't working.
>
>
> In the test case in the bug:
>
>   template <class T>
>   void g ()
>   {
>     T t;   // warning, ok
>
>     typedef T U;
>     U u;   // no warning, bug
>   }
>
>   template void g<int>();
>
> both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
> so the function doesn't set already_used or TREE_USED(t) and we get
> a warning as expected.
>
> But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
> (to implement -Wunused-local-typedefs),  initialize_local_var then
> sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
> the warning.

Hmm, I would expect maybe_record_typedef_use to set TREE_USED on the
TYPE_DECL, not on the *_TYPE which initialize_local_var checks.

Jason
Martin Sebor Feb. 22, 2017, 11:44 p.m. UTC | #6 
On 02/22/2017 11:02 AM, Jason Merrill wrote:
> On Tue, Feb 21, 2017 at 4:27 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> Ah, I see, your patch changes attribute unused handling for local
>>> variables from tracking TREE_USED to lookup_attribute.  I'm not
>>> opposed to this change, but I'd like to understand why the TREE_USED
>>> handling wasn't working.
>>
>>
>> In the test case in the bug:
>>
>>   template <class T>
>>   void g ()
>>   {
>>     T t;   // warning, ok
>>
>>     typedef T U;
>>     U u;   // no warning, bug
>>   }
>>
>>   template void g<int>();
>>
>> both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
>> so the function doesn't set already_used or TREE_USED(t) and we get
>> a warning as expected.
>>
>> But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
>> (to implement -Wunused-local-typedefs),  initialize_local_var then
>> sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
>> the warning.
>
> Hmm, I would expect maybe_record_typedef_use to set TREE_USED on the
> TYPE_DECL, not on the *_TYPE which initialize_local_var checks.

That's what it does:

   void
   maybe_record_typedef_use (tree t)
   {
     if (!is_typedef_decl (t))
       return;

     TREE_USED (t) = true;
   }

Here, t is a TYPE_DECL of the typedef U.

It has the effect of TREE_USED (TREE_TYPE (decl)) being set in
initialize_local_var.  The TREE_USED bit on the type (i.e., on
TREE_TYPE(decl) where decl is the u in the test case above) is
set when the function template is instantiated, in
set_underlying_type called from tsubst_decl.

Martin
Jason Merrill Feb. 23, 2017, 12:43 a.m. UTC | #7 
On Wed, Feb 22, 2017 at 3:44 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/22/2017 11:02 AM, Jason Merrill wrote:
>>
>> On Tue, Feb 21, 2017 at 4:27 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> Ah, I see, your patch changes attribute unused handling for local
>>>> variables from tracking TREE_USED to lookup_attribute.  I'm not
>>>> opposed to this change, but I'd like to understand why the TREE_USED
>>>> handling wasn't working.
>>>
>>>
>>>
>>> In the test case in the bug:
>>>
>>>   template <class T>
>>>   void g ()
>>>   {
>>>     T t;   // warning, ok
>>>
>>>     typedef T U;
>>>     U u;   // no warning, bug
>>>   }
>>>
>>>   template void g<int>();
>>>
>>> both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
>>> so the function doesn't set already_used or TREE_USED(t) and we get
>>> a warning as expected.
>>>
>>> But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
>>> (to implement -Wunused-local-typedefs),  initialize_local_var then
>>> sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
>>> the warning.
>>
>> Hmm, I would expect maybe_record_typedef_use to set TREE_USED on the
>> TYPE_DECL, not on the *_TYPE which initialize_local_var checks.
>
> That's what it does:
>
>   void
>   maybe_record_typedef_use (tree t)
>   {
>     if (!is_typedef_decl (t))
>       return;
>
>     TREE_USED (t) = true;
>   }
>
> Here, t is a TYPE_DECL of the typedef U.

Yes.  It is a TYPE_DECL, not a type.

> It has the effect of TREE_USED (TREE_TYPE (decl)) being set in
> initialize_local_var.  The TREE_USED bit on the type (i.e., on
> TREE_TYPE(decl) where decl is the u in the test case above) is
> set when the function template is instantiated, in
> set_underlying_type called from tsubst_decl.

Aha!  That seems like the problem.  Does removing that copy of
TREE_USED from the decl to the type break anything?

Jason
Martin Sebor Feb. 23, 2017, 8:56 p.m. UTC | #8 
On 02/22/2017 05:43 PM, Jason Merrill wrote:
> On Wed, Feb 22, 2017 at 3:44 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 02/22/2017 11:02 AM, Jason Merrill wrote:
>>>
>>> On Tue, Feb 21, 2017 at 4:27 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>>>
>>>>> Ah, I see, your patch changes attribute unused handling for local
>>>>> variables from tracking TREE_USED to lookup_attribute.  I'm not
>>>>> opposed to this change, but I'd like to understand why the TREE_USED
>>>>> handling wasn't working.
>>>>
>>>>
>>>>
>>>> In the test case in the bug:
>>>>
>>>>   template <class T>
>>>>   void g ()
>>>>   {
>>>>     T t;   // warning, ok
>>>>
>>>>     typedef T U;
>>>>     U u;   // no warning, bug
>>>>   }
>>>>
>>>>   template void g<int>();
>>>>
>>>> both TREE_USED(T) and TREE_USED(t) are zero in initialize_local_var
>>>> so the function doesn't set already_used or TREE_USED(t) and we get
>>>> a warning as expected.
>>>>
>>>> But because TREE_USED(U) is set to 1 in maybe_record_typedef_use
>>>> (to implement -Wunused-local-typedefs),  initialize_local_var then
>>>> sets already_used to 1 and later also TREE_USED(u) to 1, suppressing
>>>> the warning.
>>>
>>> Hmm, I would expect maybe_record_typedef_use to set TREE_USED on the
>>> TYPE_DECL, not on the *_TYPE which initialize_local_var checks.
>>
>> That's what it does:
>>
>>   void
>>   maybe_record_typedef_use (tree t)
>>   {
>>     if (!is_typedef_decl (t))
>>       return;
>>
>>     TREE_USED (t) = true;
>>   }
>>
>> Here, t is a TYPE_DECL of the typedef U.
>
> Yes.  It is a TYPE_DECL, not a type.
>
>> It has the effect of TREE_USED (TREE_TYPE (decl)) being set in
>> initialize_local_var.  The TREE_USED bit on the type (i.e., on
>> TREE_TYPE(decl) where decl is the u in the test case above) is
>> set when the function template is instantiated, in
>> set_underlying_type called from tsubst_decl.
>
> Aha!  That seems like the problem.  Does removing that copy of
> TREE_USED from the decl to the type break anything?

As far as I can see it breaks just gcc.dg/unused-3.c which is:

   typedef short unused_type __attribute__ ((unused));

   void f (void)
   {
     unused_type y;
   }

The reason is that the type is TREE_USED isn't set on the variable
(I didn't look where the bit from the type is copied into the var).,

This could be fixed by also looking at the type's attribute in this
case.  That would seem to me like the better approach because it
more faithfully represents what's going on in the code.  I.e., that
the variable is in fact unused, but that its type says not to complain
about it.

Let me know what you prefer.

While looking into this I noticed that regardless of this change,
the C++ front end warns on the following modified version of the
test case:

   static unused_type x;   // bogus -Wunused-variable

   void f (void)
   {
     static unused_type y;   // bogus -Wunused-variable
   }

I opened bug 79695 for it.

Martin
Jason Merrill Feb. 23, 2017, 11:33 p.m. UTC | #9 
On Thu, Feb 23, 2017 at 12:56 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 02/22/2017 05:43 PM, Jason Merrill wrote:
>> On Wed, Feb 22, 2017 at 3:44 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> On 02/22/2017 11:02 AM, Jason Merrill wrote:

>>> The TREE_USED bit on the type (i.e., on
>>> TREE_TYPE(decl) where decl is the u in the test case above) is
>>> set when the function template is instantiated, in
>>> set_underlying_type called from tsubst_decl.
>>
>> Aha!  That seems like the problem.  Does removing that copy of
>> TREE_USED from the decl to the type break anything?
>
> As far as I can see it breaks just gcc.dg/unused-3.c which is:
>
>   typedef short unused_type __attribute__ ((unused));
>
>   void f (void)
>   {
>     unused_type y;
>   }

Ah.  So the problem here is that we're using TREE_USED on a TYPE_DECL
for two things: to track whether the typedef has been used, and to
determine whether to treat a variable of that type as already used.
Normally this isn't too much of a problem because we copy the
TREE_USED flag from decl to type before there could be any uses, but
in a template I guess we mark the typedef within the template when it
is used, and then when we instantiate the typedef we incorrectly pass
that mark along to the type.

Your patch deals with this by ignoring TREE_USED on the type, so it
doesn't matter that it's wrong.  Another approach would be to correct
the setting of TREE_USED by setting it based on looking up the unused
attribute, rather than copying it from the TYPE_DECL.  So also going
back to the attribute, but in set_underlying_type rather than
poplevel.

Jason
Jason Merrill March 20, 2017, 9:11 p.m. UTC | #10 
On Thu, Feb 23, 2017 at 6:33 PM, Jason Merrill <jason@redhat.com> wrote:
> On Thu, Feb 23, 2017 at 12:56 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 02/22/2017 05:43 PM, Jason Merrill wrote:
>>> On Wed, Feb 22, 2017 at 3:44 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>> On 02/22/2017 11:02 AM, Jason Merrill wrote:
>
>>>> The TREE_USED bit on the type (i.e., on
>>>> TREE_TYPE(decl) where decl is the u in the test case above) is
>>>> set when the function template is instantiated, in
>>>> set_underlying_type called from tsubst_decl.
>>>
>>> Aha!  That seems like the problem.  Does removing that copy of
>>> TREE_USED from the decl to the type break anything?
>>
>> As far as I can see it breaks just gcc.dg/unused-3.c which is:
>>
>>   typedef short unused_type __attribute__ ((unused));
>>
>>   void f (void)
>>   {
>>     unused_type y;
>>   }
>
> Ah.  So the problem here is that we're using TREE_USED on a TYPE_DECL
> for two things: to track whether the typedef has been used, and to
> determine whether to treat a variable of that type as already used.
> Normally this isn't too much of a problem because we copy the
> TREE_USED flag from decl to type before there could be any uses, but
> in a template I guess we mark the typedef within the template when it
> is used, and then when we instantiate the typedef we incorrectly pass
> that mark along to the type.
>
> Your patch deals with this by ignoring TREE_USED on the type, so it
> doesn't matter that it's wrong.  Another approach would be to correct
> the setting of TREE_USED by setting it based on looking up the unused
> attribute, rather than copying it from the TYPE_DECL.  So also going
> back to the attribute, but in set_underlying_type rather than
> poplevel.

Thoughts?

Jason
diff mbox

Patch

PR c++/79548 - [5/6/7 Regression] missing -Wunused-variable on a typedef'd variable in a function template

gcc/cp/ChangeLog:

	PR c++/79548
	* decl.c (poplevel): Avoid diagnosing entities declared with
	attribute unused.
	(initialize_local_var): Do not consider the type of a variable
	when determining whether or not it's used.
	
gcc/testsuite/ChangeLog:

	PR c++/79548
	* g++.dg/warn/Wunused-var-26.C: New test.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 70c44fb..e315ad0 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -664,7 +664,8 @@  poplevel (int keep, int reverse, int functionbody)
 	    && (!CLASS_TYPE_P (type)
 		|| !TYPE_HAS_NONTRIVIAL_DESTRUCTOR (type)
 		|| lookup_attribute ("warn_unused",
-				     TYPE_ATTRIBUTES (TREE_TYPE (decl)))))
+				     TYPE_ATTRIBUTES (TREE_TYPE (decl))))
+	    && !lookup_attribute ("unused", TYPE_ATTRIBUTES (TREE_TYPE (decl))))
 	  {
 	    if (! TREE_USED (decl))
 	      warning_at (DECL_SOURCE_LOCATION (decl),
@@ -6546,7 +6547,6 @@  initialize_local_var (tree decl, tree init)
 {
   tree type = TREE_TYPE (decl);
   tree cleanup;
-  int already_used;
 
   gcc_assert (VAR_P (decl)
 	      || TREE_CODE (decl) == RESULT_DECL);
@@ -6564,7 +6564,7 @@  initialize_local_var (tree decl, tree init)
     return;
 
   /* Compute and store the initial value.  */
-  already_used = TREE_USED (decl) || TREE_USED (type);
+  bool already_used = TREE_USED (decl);
   if (TREE_USED (type))
     DECL_READ_P (decl) = 1;
 
diff --git a/gcc/testsuite/g++.dg/warn/Wunused-var-26.C b/gcc/testsuite/g++.dg/warn/Wunused-var-26.C
new file mode 100644
index 0000000..562f25b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wunused-var-26.C
@@ -0,0 +1,127 @@ 
+// PR c++/79548 - missing -Wunused-variable on a typedef'd variable
+// in a function template
+// { dg-do compile }
+// { dg-options "-Wunused" }
+
+
+#define UNUSED __attribute__ ((unused))
+
+template <class T>
+void f_int ()
+{
+  T t;                        // { dg-warning "unused variable" }
+
+  typedef T U;
+  U u;                        // { dg-warning "unused variable" }
+}
+
+template void f_int<int>();
+
+
+template <class T>
+void f_intptr ()
+{
+  T *t = 0;                   // { dg-warning "unused variable" }
+
+  typedef T U;
+  U *u = 0;                   // { dg-warning "unused variable" }
+}
+
+template void f_intptr<int>();
+
+
+template <class T>
+void f_var_unused ()
+{
+  // The variable is marked unused.
+  T t UNUSED;
+
+  typedef T U;
+  U u UNUSED;
+}
+
+template void f_var_unused<int>();
+
+
+template <class T>
+void f_var_type_unused ()
+{
+  // The variable's type is marked unused.
+  T* UNUSED t = new T;   // { dg-bogus "unused variable" "bug 79585" { xfail *-*-* } }
+
+  typedef T U;
+  U* UNUSED u = new U;   // { dg-bogus "unused variable" "bug 79585" { xfail *-*-* } }
+
+  typedef T UNUSED U;
+  U v = U ();   // { dg-bogus "unused variable" "bug 79585" { xfail *-*-* } }
+}
+
+template void f_var_type_unused<int>();
+
+
+struct A { int i; };
+
+template <class T>
+void f_A ()
+{
+  T t;                        // { dg-warning "unused variable" }
+
+  typedef T U;
+  U u;                        // { dg-warning "unused variable" }
+}
+
+template void f_A<A>();
+
+
+template <class T>
+void f_A_unused ()
+{
+  T t UNUSED;
+
+  typedef T U;
+  U u UNUSED;
+}
+
+template void f_A_unused<A>();
+
+
+struct B { B (); };
+
+template <class T>
+void f_B ()
+{
+  T t;
+
+  typedef T U;
+  U u;
+}
+
+template void f_B<B>();
+
+
+struct C { ~C (); };
+
+template <class T>
+void f_C ()
+{
+  T t;
+
+  typedef T U;
+  U u;
+}
+
+template void f_C<C>();
+
+
+struct D { B b; };
+
+template <class T>
+void f_D ()
+{
+  T t;
+
+  typedef T U;
+  U u;
+}
+
+template void f_D<D>();