-Wmissing-attributes: avoid duplicates and false positives

Message ID orwogh9p28.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • -Wmissing-attributes: avoid duplicates and false positives
Related show

Commit Message

Alexandre Oliva July 17, 2019, 6:02 a.m.
Hello, Martin,

The initial patch for PR 81824 fixed one of the possibilities of
-Wmissing-attributes reporting duplicates, namely, if TMPL had an
attribute in ATTRLIST that was missing from DECL's decl and type
attribute lists, both being non-empty.

Another possibility of duplicate reporting remained: when an attribute
in ATTRLIST is present in both decl and type attribute lists of TMPL,
and absent from DECL's attribute lists, it is reported once for each
of TMPL's lists.

The implementation still allowed for false positives: an attribute in
ATTRLIST that is present in TMPL will be regarded as missing as soon
as it is not found in DECL's decl attribute list, even if it is later
found in DECL's type attribute list.

This patch fixes both problems, so that an attribute from ATTRLIST
that is present in any of TMPL's lists will be reported at most once,
and only if it is missing from both DECL's lists.


Now, I realize there are some attributes that are only acceptable for
types, and some that are only acceptable for function declarations.
ISTM that some even have different meanings depending on whether
they're associated with types or declarations.  There's room for an
argument for checking only corresponding lists, for at least some of
the attributes.  AFAICT it doesn't apply to -Wmissing-attributes, that
are either acceptable in either list, or only in the FUNCTION_DECL
list, so I'm leaving it at that in the hope that it doesn't apply to
any other users of decls_mismatched_attributes either.


Regstrapping on x86_64-linux-gnu.  Ok to install if it passes?


for  gcc/ChangeLog

	PR middle-end/81824
	* attribs.c (decls_mismatched_attributes): Avoid duplicates
	and false positives.

for  gcc/testsuite/ChangeLog

	PR middle-end/81824
	* g++.dg/Wmissing-attributes-1.C: New.
---
 gcc/attribs.c                                |   14 +++++--
 gcc/testsuite/g++.dg/Wmissing-attributes-1.C |   55 ++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/Wmissing-attributes-1.C


-- 
Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
Be the change, be Free!                 FSF Latin America board member
GNU Toolchain Engineer                        Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara

Comments

Martin Sebor July 17, 2019, 4:33 p.m. | #1
On 7/17/19 12:02 AM, Alexandre Oliva wrote:
> Hello, Martin,

> 

> The initial patch for PR 81824 fixed one of the possibilities of

> -Wmissing-attributes reporting duplicates, namely, if TMPL had an

> attribute in ATTRLIST that was missing from DECL's decl and type

> attribute lists, both being non-empty.

> 

> Another possibility of duplicate reporting remained: when an attribute

> in ATTRLIST is present in both decl and type attribute lists of TMPL,

> and absent from DECL's attribute lists, it is reported once for each

> of TMPL's lists.

> 

> The implementation still allowed for false positives: an attribute in

> ATTRLIST that is present in TMPL will be regarded as missing as soon

> as it is not found in DECL's decl attribute list, even if it is later

> found in DECL's type attribute list.


Hey Alex!

Isn't this test sufficient to avoid the problems?

	      if (!k && kmax > 1)
		continue;

> This patch fixes both problems, so that an attribute from ATTRLIST

> that is present in any of TMPL's lists will be reported at most once,

> and only if it is missing from both DECL's lists.

> 

> 

> Now, I realize there are some attributes that are only acceptable for

> types, and some that are only acceptable for function declarations.

> ISTM that some even have different meanings depending on whether

> they're associated with types or declarations.  There's room for an

> argument for checking only corresponding lists, for at least some of

> the attributes.  AFAICT it doesn't apply to -Wmissing-attributes, that

> are either acceptable in either list, or only in the FUNCTION_DECL

> list, so I'm leaving it at that in the hope that it doesn't apply to

> any other users of decls_mismatched_attributes either.

> 

> 

> Regstrapping on x86_64-linux-gnu.  Ok to install if it passes?


The change looks cleaner than the cumbersome code that's there
now so I have no problem with it but I'm not sure it does more
than the test above.  The test case included in the patch also
gets just the expected warnings with the trunk so I'm wondering
how the problems you describe can come up.  Can you put together
a test case that does do the wrong thing?

Thanks
Martin

> 

> 

> for  gcc/ChangeLog

> 

> 	PR middle-end/81824

> 	* attribs.c (decls_mismatched_attributes): Avoid duplicates

> 	and false positives.

> 

> for  gcc/testsuite/ChangeLog

> 

> 	PR middle-end/81824

> 	* g++.dg/Wmissing-attributes-1.C: New.

> ---

>   gcc/attribs.c                                |   14 +++++--

>   gcc/testsuite/g++.dg/Wmissing-attributes-1.C |   55 ++++++++++++++++++++++++++

>   2 files changed, 65 insertions(+), 4 deletions(-)

>   create mode 100644 gcc/testsuite/g++.dg/Wmissing-attributes-1.C

> 

> diff --git a/gcc/attribs.c b/gcc/attribs.c

> index 8e5401655972..f4777c6a8233 100644

> --- a/gcc/attribs.c

> +++ b/gcc/attribs.c

> @@ -1931,15 +1931,19 @@ decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,

>   	  if (!has_attribute (tmpls[j], tmpl_attrs[j], blacklist[i]))

>   	    continue;

>   

> +	  bool found = false;

>   	  unsigned kmax = 1 + !!decl_attrs[1];

>   	  for (unsigned k = 0; k != kmax; ++k)

>   	    {

>   	      if (has_attribute (decls[k], decl_attrs[k], blacklist[i]))

> -		break;

> -

> -	      if (!k && kmax > 1)

> -		continue;

> +		{

> +		  found = true;

> +		  break;

> +		}

> +	    }

>   

> +	  if (!found)

> +	    {

>   	      if (nattrs)

>   		pp_string (attrstr, ", ");

>   	      pp_begin_quote (attrstr, pp_show_color (global_dc->printer));

> @@ -1947,6 +1951,8 @@ decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,

>   	      pp_end_quote (attrstr, pp_show_color (global_dc->printer));

>   	      ++nattrs;

>   	    }

> +

> +	  break;

>   	}

>       }

>   

> diff --git a/gcc/testsuite/g++.dg/Wmissing-attributes-1.C b/gcc/testsuite/g++.dg/Wmissing-attributes-1.C

> new file mode 100644

> index 000000000000..56d28b339e17

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/Wmissing-attributes-1.C

> @@ -0,0 +1,55 @@

> +// { dg-do compile }

> +// { dg-options "-Wmissing-attributes" }

> +

> +#define ATTR(list)   __attribute__ (list)

> +

> +/* Type attributes are normally absent in template functions, and the

> +   mere presence of any such attribute used to cause the

> +   -Wmissing-attributes checks, that checked for attributes typically

> +   associated with functions rather than types, to report any missing

> +   attributes twice: once for the specialization attribute list, once

> +   for its type attribute list.

> +

> +   If we force any of the checked-for attributes to be associated with

> +   types rather than functions, even later implementations that fixed

> +   the duplicate reporting problem above would still report them as

> +   missing (in the function attribute list) even when present (in the

> +   type attribute list).  */

> +typedef void* ATTR ((alloc_size (1))) f_type (int);

> +

> +template <class T>

> +f_type

> +ATTR ((malloc))

> +missing_malloc;            // { dg-message "missing primary template attribute .malloc." }

> +

> +template <>

> +f_type

> +missing_malloc<char>;      // { dg-warning "explicit specialization .\[^\n\r\]+. may be missing attributes" }

> +

> +

> +/* Check that even an attribute that appears in both lists (decl and

> +   type) in a template declaration is reported as missing only

> +   once.  */

> +

> +template <class T>

> +f_type

> +ATTR ((alloc_size (1))) // In both attr lists, decl's and type's.

> +missing_alloc_size;            // { dg-message "missing primary template attribute .alloc_size." }

> +

> +template <>

> +void *

> +missing_alloc_size<char>(int); // { dg-warning "explicit specialization .\[^\n\r\]+. may be missing attributes" }

> +

> +

> +/* Check that even an attribute that appears in both lists (decl and

> +   type) is not report as missing if it's present only in the type

> +   list.  */

> +

> +template <class T>

> +f_type

> +ATTR ((alloc_size (1))) // In both attr lists, decl's and type's.

> +missing_nothing;

> +

> +template <>

> +f_type

> +missing_nothing<char>;

>
Alexandre Oliva July 17, 2019, 7:14 p.m. | #2
On Jul 17, 2019, Martin Sebor <msebor@gmail.com> wrote:

> Isn't this test sufficient to avoid the problems?


> 	      if (!k && kmax > 1)

> 		continue;


It is, unless someone (i) doesn't realize attributes that are present in
the type can't be present in the decl, (ii) misreads the '!k' as just
'k', and (iii) uses the wrong toolchain to confirm the consequences of
the misreading ;-/  doh!

> Can you put together a test case that does do the wrong thing?


I'm afraid not, all of the additional errors I observed were correctly
covered by the test I misunderstood once I grasped the actual logic.
Sorry about the, erhm, false positive ;-)


> The change looks cleaner than the cumbersome code that's there

> now so I have no problem with it but I'm not sure it does more

> than the test above


Would you like me to put it in regardless?
Does it make sense to put the testcase in anyway?

-- 
Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
Be the change, be Free!                 FSF Latin America board member
GNU Toolchain Engineer                        Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara
Martin Sebor July 17, 2019, 7:58 p.m. | #3
On 7/17/19 1:14 PM, Alexandre Oliva wrote:
> On Jul 17, 2019, Martin Sebor <msebor@gmail.com> wrote:

> 

>> Isn't this test sufficient to avoid the problems?

> 

>> 	      if (!k && kmax > 1)

>> 		continue;

> 

> It is, unless someone (i) doesn't realize attributes that are present in

> the type can't be present in the decl, (ii) misreads the '!k' as just

> 'k', and (iii) uses the wrong toolchain to confirm the consequences of

> the misreading ;-/  doh!

> 

>> Can you put together a test case that does do the wrong thing?

> 

> I'm afraid not, all of the additional errors I observed were correctly

> covered by the test I misunderstood once I grasped the actual logic.

> Sorry about the, erhm, false positive ;-)


No worries.  The purpose of the test above is far from intuitive
(even to me now).

>> The change looks cleaner than the cumbersome code that's there

>> now so I have no problem with it but I'm not sure it does more

>> than the test above

> 

> Would you like me to put it in regardless?


Sure, if it's worthwhile to you I think it's an improvement even
if it doesn't fix a bug.  (In full disclosure I'm not empowered
to formally approve bigger patches but I think cleanups like this
can safely be committed as obvious.)

> Does it make sense to put the testcase in anyway?


If it isn't already covered by one of the existing tests I'd say
definitely.  I also tried the following while playing with it so
if this variation isn't being exercised either it might be worth
adding to the new test as well:

   template <class T>
   f_type
   missing_nothing2;

   template <>
   void *
   ATTR ((alloc_size (1)))
   missing_nothing2<char>(int);

It's your call.

Thanks!
Martin
Alexandre Oliva July 18, 2019, 12:42 a.m. | #4
On Jul 17, 2019, Martin Sebor <msebor@gmail.com> wrote:

> Sure, if it's worthwhile to you I think it's an improvement even

> if it doesn't fix a bug.  (In full disclosure I'm not empowered

> to formally approve bigger patches but I think cleanups like this

> can safely be committed as obvious.)


Thanks, I'm installing the patch below.

>> Does it make sense to put the testcase in anyway?


> If it isn't already covered by one of the existing tests I'd say

> definitely.  I also tried the following while playing with it so

> if this variation isn't being exercised either it might be worth

> adding to the new test as well:


Thanks, I added it to the new test



-Wmissing-attributes: check that we avoid duplicates and false positives

The initial patch for PR 81824 fixed various possibilities of
-Wmissing-attributes reporting duplicates and false positives.  The
test that avoided them was a little obscure, though, so this patch
rewrites it into a more self-evident form.

The patch also adds a testcase that already passed, but that
explicitly covers some of the possibilities of reporting duplicates
and false positives that preexisting tests did not cover.


for  gcc/ChangeLog

	PR middle-end/81824
	* attribs.c (decls_mismatched_attributes): Simplify the logic
	that avoids duplicates and false positives.

for  gcc/testsuite/ChangeLog

	PR middle-end/81824
	* g++.dg/Wmissing-attributes-1.C: New.  Some of its fragments
	are from Martin Sebor.
---
 gcc/attribs.c                                |   14 ++++--
 gcc/testsuite/g++.dg/Wmissing-attributes-1.C |   66 ++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/Wmissing-attributes-1.C

diff --git a/gcc/attribs.c b/gcc/attribs.c
index 8e54016559723..f4777c6a82336 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -1931,15 +1931,19 @@ decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,
 	  if (!has_attribute (tmpls[j], tmpl_attrs[j], blacklist[i]))
 	    continue;
 
+	  bool found = false;
 	  unsigned kmax = 1 + !!decl_attrs[1];
 	  for (unsigned k = 0; k != kmax; ++k)
 	    {
 	      if (has_attribute (decls[k], decl_attrs[k], blacklist[i]))
-		break;
-
-	      if (!k && kmax > 1)
-		continue;
+		{
+		  found = true;
+		  break;
+		}
+	    }
 
+	  if (!found)
+	    {
 	      if (nattrs)
 		pp_string (attrstr, ", ");
 	      pp_begin_quote (attrstr, pp_show_color (global_dc->printer));
@@ -1947,6 +1951,8 @@ decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,
 	      pp_end_quote (attrstr, pp_show_color (global_dc->printer));
 	      ++nattrs;
 	    }
+
+	  break;
 	}
     }
 
diff --git a/gcc/testsuite/g++.dg/Wmissing-attributes-1.C b/gcc/testsuite/g++.dg/Wmissing-attributes-1.C
new file mode 100644
index 0000000000000..972e68305bb90
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wmissing-attributes-1.C
@@ -0,0 +1,66 @@
+// { dg-do compile }
+// { dg-options "-Wmissing-attributes" }
+
+#define ATTR(list)   __attribute__ (list)
+
+/* Type attributes are normally absent in template functions, and the
+   mere presence of any such attribute used to cause the
+   -Wmissing-attributes checks, that checked for attributes typically
+   associated with functions rather than types, to report any missing
+   attributes twice: once for the specialization attribute list, once
+   for its type attribute list.
+
+   This test uses both decl and type attributes to exercise the code
+   that avoids reporting duplicates, in ways that failed in the past
+   but that were not covered in other tests.  */
+typedef void* ATTR ((alloc_size (1))) f_type (int);
+
+template <class T>
+f_type
+ATTR ((malloc))
+missing_malloc;            // { dg-message "missing primary template attribute .malloc." }
+
+template <>
+f_type
+missing_malloc<char>;      // { dg-warning "explicit specialization .\[^\n\r\]+. may be missing attributes" }
+
+
+/* Check that even an attribute that appears in both lists (decl and
+   type) in a template declaration is reported as missing only
+   once.  */
+
+template <class T>
+f_type
+ATTR ((alloc_size (1))) // In both attr lists, decl's and type's.
+missing_alloc_size;            // { dg-message "missing primary template attribute .alloc_size." }
+
+template <>
+void *
+missing_alloc_size<char>(int); // { dg-warning "explicit specialization .\[^\n\r\]+. may be missing attributes" }
+
+
+/* Check that even an attribute that appears in both lists (decl and
+   type) is not reported as missing if it's present only in the type
+   list.  */
+
+template <class T>
+f_type
+ATTR ((alloc_size (1))) // In both attr lists, decl's and type's.
+missing_nothing;
+
+template <>
+f_type
+missing_nothing<char>;
+
+
+/* For completeness, check that a type attribute is matched by a decl
+   attribute in the specialization.  */
+
+template <class T>
+f_type
+missing_nothing2;
+
+template <>
+void *
+ATTR ((alloc_size (1)))
+missing_nothing2<char>(int);


-- 
Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
Be the change, be Free!                 FSF Latin America board member
GNU Toolchain Engineer                        Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara

Patch

diff --git a/gcc/attribs.c b/gcc/attribs.c
index 8e5401655972..f4777c6a8233 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -1931,15 +1931,19 @@  decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,
 	  if (!has_attribute (tmpls[j], tmpl_attrs[j], blacklist[i]))
 	    continue;
 
+	  bool found = false;
 	  unsigned kmax = 1 + !!decl_attrs[1];
 	  for (unsigned k = 0; k != kmax; ++k)
 	    {
 	      if (has_attribute (decls[k], decl_attrs[k], blacklist[i]))
-		break;
-
-	      if (!k && kmax > 1)
-		continue;
+		{
+		  found = true;
+		  break;
+		}
+	    }
 
+	  if (!found)
+	    {
 	      if (nattrs)
 		pp_string (attrstr, ", ");
 	      pp_begin_quote (attrstr, pp_show_color (global_dc->printer));
@@ -1947,6 +1951,8 @@  decls_mismatched_attributes (tree tmpl, tree decl, tree attrlist,
 	      pp_end_quote (attrstr, pp_show_color (global_dc->printer));
 	      ++nattrs;
 	    }
+
+	  break;
 	}
     }
 
diff --git a/gcc/testsuite/g++.dg/Wmissing-attributes-1.C b/gcc/testsuite/g++.dg/Wmissing-attributes-1.C
new file mode 100644
index 000000000000..56d28b339e17
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wmissing-attributes-1.C
@@ -0,0 +1,55 @@ 
+// { dg-do compile }
+// { dg-options "-Wmissing-attributes" }
+
+#define ATTR(list)   __attribute__ (list)
+
+/* Type attributes are normally absent in template functions, and the
+   mere presence of any such attribute used to cause the
+   -Wmissing-attributes checks, that checked for attributes typically
+   associated with functions rather than types, to report any missing
+   attributes twice: once for the specialization attribute list, once
+   for its type attribute list.
+
+   If we force any of the checked-for attributes to be associated with
+   types rather than functions, even later implementations that fixed
+   the duplicate reporting problem above would still report them as
+   missing (in the function attribute list) even when present (in the
+   type attribute list).  */
+typedef void* ATTR ((alloc_size (1))) f_type (int);
+
+template <class T>
+f_type
+ATTR ((malloc))
+missing_malloc;            // { dg-message "missing primary template attribute .malloc." }
+
+template <>
+f_type
+missing_malloc<char>;      // { dg-warning "explicit specialization .\[^\n\r\]+. may be missing attributes" }
+
+
+/* Check that even an attribute that appears in both lists (decl and
+   type) in a template declaration is reported as missing only
+   once.  */
+
+template <class T>
+f_type
+ATTR ((alloc_size (1))) // In both attr lists, decl's and type's.
+missing_alloc_size;            // { dg-message "missing primary template attribute .alloc_size." }
+
+template <>
+void *
+missing_alloc_size<char>(int); // { dg-warning "explicit specialization .\[^\n\r\]+. may be missing attributes" }
+
+
+/* Check that even an attribute that appears in both lists (decl and
+   type) is not report as missing if it's present only in the type
+   list.  */
+
+template <class T>
+f_type
+ATTR ((alloc_size (1))) // In both attr lists, decl's and type's.
+missing_nothing;
+
+template <>
+f_type
+missing_nothing<char>;