| Message ID | orwogh9p28.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | -Wmissing-attributes: avoid duplicates and false positives | expand |
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>; >
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?
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
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);
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>;