| Message ID | 20150519140753.GW27320@redhat.com |
|---|---|
| State | New |
| Headers | show |
Ping. On Tue, May 19, 2015 at 04:07:53PM +0200, Marek Polacek wrote: > This PR points out that we output same -Wformat warning twice when using > __attribute__ ((format)). The problem was that attribute_value_equal > (called when processing merge_attributes) got two lists: > "format printf, 1, 2" and "__format__ __printf__, 1, 2", these should be > equal. But since attribute_value_equal uses simple_cst_list_equal when > it sees a TREE_LISTs, it doesn't consider "__printf__" and "printf" as > the same, so it said that the two lists aren't same. That means that the > type then contains two same format attributes and we warn twice. > Fixed by handling the format attribute specially. (The patch doesn't > consider the printf and the gnu_printf archetypes as the same, so we still > might get duplicate warnings when combining printf and gnu_printf.) > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2015-05-19 Marek Polacek <polacek@redhat.com> > > PR c/64223 > * tree.c (attribute_value_equal): Handle attribute format. > (cmp_attrib_identifiers): Factor out of lookup_ident_attribute. > > * gcc.dg/pr64223-1.c: New test. > * gcc.dg/pr64223-2.c: New test. > > diff --git gcc/testsuite/gcc.dg/pr64223-1.c gcc/testsuite/gcc.dg/pr64223-1.c > index e69de29..015bfd8 100644 > --- gcc/testsuite/gcc.dg/pr64223-1.c > +++ gcc/testsuite/gcc.dg/pr64223-1.c > @@ -0,0 +1,12 @@ > +/* PR c/64223: Test for duplicated warnings. */ > +/* { dg-do compile } */ > +/* { dg-options "-Wformat" } */ > + > +int printf (const char *, ...) __attribute__ ((__format__ (__printf__, 1, 2))); > + > +void > +foo (void) > +{ > + printf ("%d\n", 0UL); /* { dg-bogus "expects argument of type.*expects argument of type" } */ > + /* { dg-warning "expects argument of type" "" { target *-*-* } 10 } */ > +} > diff --git gcc/testsuite/gcc.dg/pr64223-2.c gcc/testsuite/gcc.dg/pr64223-2.c > index e69de29..2a1627e 100644 > --- gcc/testsuite/gcc.dg/pr64223-2.c > +++ gcc/testsuite/gcc.dg/pr64223-2.c > @@ -0,0 +1,13 @@ > +/* PR c/64223: Test for duplicated warnings. */ > +/* { dg-do compile } */ > +/* { dg-options "-Wformat" } */ > + > +int myprintf (const char *, ...) __attribute__ ((__format__ (printf, 1, 2))); > +int myprintf (const char *, ...) __attribute__ ((__format__ (__printf__, 1, 2))); > + > +void > +foo (void) > +{ > + myprintf ("%d\n", 0UL); /* { dg-bogus "expects argument of type.*expects argument of type" } */ > + /* { dg-warning "expects argument of type" "" { target *-*-* } 11 } */ > +} > diff --git gcc/tree.c gcc/tree.c > index 6297f04..a58ad7b 100644 > --- gcc/tree.c > +++ gcc/tree.c > @@ -4871,9 +4871,53 @@ simple_cst_list_equal (const_tree l1, const_tree l2) > return l1 == l2; > } > > +/* Compare two identifier nodes representing attributes. Either one may > + be in prefixed __ATTR__ form. Return true if they are the same, false > + otherwise. */ > + > +static bool > +cmp_attrib_identifiers (const_tree attr1, const_tree attr2) > +{ > + /* Make sure we're dealing with IDENTIFIER_NODEs. */ > + gcc_checking_assert (TREE_CODE (attr1) == IDENTIFIER_NODE > + && TREE_CODE (attr2) == IDENTIFIER_NODE); > + > + /* Identifiers can be compared directly for equality. */ > + if (attr1 == attr2) > + return true; > + > + /* If they are not equal, they may still be one in the form > + 'text' while the other one is in the form '__text__'. TODO: > + If we were storing attributes in normalized 'text' form, then > + this could all go away and we could take full advantage of > + the fact that we're comparing identifiers. :-) */ > + const size_t attr1_len = IDENTIFIER_LENGTH (attr1); > + const size_t attr2_len = IDENTIFIER_LENGTH (attr2); > + > + if (attr2_len == attr1_len + 4) > + { > + const char *p = IDENTIFIER_POINTER (attr2); > + const char *q = IDENTIFIER_POINTER (attr1); > + if (p[0] == '_' && p[1] == '_' > + && p[attr2_len - 2] == '_' && p[attr2_len - 1] == '_' > + && strncmp (q, p + 2, attr1_len) == 0) > + return true;; > + } > + else if (attr2_len + 4 == attr1_len) > + { > + const char *p = IDENTIFIER_POINTER (attr2); > + const char *q = IDENTIFIER_POINTER (attr1); > + if (q[0] == '_' && q[1] == '_' > + && q[attr1_len - 2] == '_' && q[attr1_len - 1] == '_' > + && strncmp (q + 2, p, attr2_len) == 0) > + return true; > + } > + > + return false; > +} > + > /* Compare two attributes for their value identity. Return true if the > - attribute values are known to be equal; otherwise return false. > -*/ > + attribute values are known to be equal; otherwise return false. */ > > bool > attribute_value_equal (const_tree attr1, const_tree attr2) > @@ -4883,10 +4927,25 @@ attribute_value_equal (const_tree attr1, const_tree attr2) > > if (TREE_VALUE (attr1) != NULL_TREE > && TREE_CODE (TREE_VALUE (attr1)) == TREE_LIST > - && TREE_VALUE (attr2) != NULL > + && TREE_VALUE (attr2) != NULL_TREE > && TREE_CODE (TREE_VALUE (attr2)) == TREE_LIST) > - return (simple_cst_list_equal (TREE_VALUE (attr1), > - TREE_VALUE (attr2)) == 1); > + { > + /* Handle attribute format. */ > + if (is_attribute_p ("format", TREE_PURPOSE (attr1))) > + { > + attr1 = TREE_VALUE (attr1); > + attr2 = TREE_VALUE (attr2); > + /* Compare the archetypes (printf/scanf/strftime/...). */ > + if (!cmp_attrib_identifiers (TREE_VALUE (attr1), > + TREE_VALUE (attr2))) > + return false; > + /* Archetypes are the same. Compare the rest. */ > + return (simple_cst_list_equal (TREE_CHAIN (attr1), > + TREE_CHAIN (attr2)) == 1); > + } > + return (simple_cst_list_equal (TREE_VALUE (attr1), > + TREE_VALUE (attr2)) == 1); > + } > > if ((flag_openmp || flag_openmp_simd) > && TREE_VALUE (attr1) && TREE_VALUE (attr2) > @@ -6038,38 +6097,10 @@ lookup_ident_attribute (tree attr_identifier, tree list) > gcc_checking_assert (TREE_CODE (get_attribute_name (list)) > == IDENTIFIER_NODE); > > - /* Identifiers can be compared directly for equality. */ > - if (attr_identifier == get_attribute_name (list)) > + if (cmp_attrib_identifiers (attr_identifier, > + get_attribute_name (list))) > + /* Found it. */ > break; > - > - /* If they are not equal, they may still be one in the form > - 'text' while the other one is in the form '__text__'. TODO: > - If we were storing attributes in normalized 'text' form, then > - this could all go away and we could take full advantage of > - the fact that we're comparing identifiers. :-) */ > - { > - size_t attr_len = IDENTIFIER_LENGTH (attr_identifier); > - size_t ident_len = IDENTIFIER_LENGTH (get_attribute_name (list)); > - > - if (ident_len == attr_len + 4) > - { > - const char *p = IDENTIFIER_POINTER (get_attribute_name (list)); > - const char *q = IDENTIFIER_POINTER (attr_identifier); > - if (p[0] == '_' && p[1] == '_' > - && p[ident_len - 2] == '_' && p[ident_len - 1] == '_' > - && strncmp (q, p + 2, attr_len) == 0) > - break; > - } > - else if (ident_len + 4 == attr_len) > - { > - const char *p = IDENTIFIER_POINTER (get_attribute_name (list)); > - const char *q = IDENTIFIER_POINTER (attr_identifier); > - if (q[0] == '_' && q[1] == '_' > - && q[attr_len - 2] == '_' && q[attr_len - 1] == '_' > - && strncmp (q + 2, p, ident_len) == 0) > - break; > - } > - } > list = TREE_CHAIN (list); > } > > > Marek Marek
On 05/26/2015 05:06 AM, Marek Polacek wrote: > Ping. > > On Tue, May 19, 2015 at 04:07:53PM +0200, Marek Polacek wrote: >> This PR points out that we output same -Wformat warning twice when using >> __attribute__ ((format)). The problem was that attribute_value_equal >> (called when processing merge_attributes) got two lists: >> "format printf, 1, 2" and "__format__ __printf__, 1, 2", these should be >> equal. But since attribute_value_equal uses simple_cst_list_equal when >> it sees a TREE_LISTs, it doesn't consider "__printf__" and "printf" as >> the same, so it said that the two lists aren't same. That means that the >> type then contains two same format attributes and we warn twice. >> Fixed by handling the format attribute specially. (The patch doesn't >> consider the printf and the gnu_printf archetypes as the same, so we still >> might get duplicate warnings when combining printf and gnu_printf.) >> >> Bootstrapped/regtested on x86_64-linux, ok for trunk? >> >> 2015-05-19 Marek Polacek <polacek@redhat.com> >> >> PR c/64223 >> * tree.c (attribute_value_equal): Handle attribute format. >> (cmp_attrib_identifiers): Factor out of lookup_ident_attribute. >> >> * gcc.dg/pr64223-1.c: New test. >> * gcc.dg/pr64223-2.c: New test. >> >> diff --git gcc/tree.c gcc/tree.c >> index 6297f04..a58ad7b 100644 >> --- gcc/tree.c >> +++ gcc/tree.c >> @@ -4871,9 +4871,53 @@ simple_cst_list_equal (const_tree l1, const_tree l2) >> return l1 == l2; >> } >> >> +/* Compare two identifier nodes representing attributes. Either one may >> + be in prefixed __ATTR__ form. Return true if they are the same, false >> + otherwise. */ I think "wrapped" may be better than "prefixed" above. But it's obviously a nit. Your call whether or not to change. >> + >> + if (attr2_len == attr1_len + 4) >> + { >> + const char *p = IDENTIFIER_POINTER (attr2); >> + const char *q = IDENTIFIER_POINTER (attr1); >> + if (p[0] == '_' && p[1] == '_' >> + && p[attr2_len - 2] == '_' && p[attr2_len - 1] == '_' >> + && strncmp (q, p + 2, attr1_len) == 0) >> + return true;; >> + } >> + else if (attr2_len + 4 == attr1_len) >> + { >> + const char *p = IDENTIFIER_POINTER (attr2); >> + const char *q = IDENTIFIER_POINTER (attr1); >> + if (q[0] == '_' && q[1] == '_' >> + && q[attr1_len - 2] == '_' && q[attr1_len - 1] == '_' >> + && strncmp (q + 2, p, attr2_len) == 0) >> + return true; >> + } Consider canonicalizing and using std::swap so that the longer identifier is always in attr1 and the second hunk of code can just go away. Obviously it's not a huge deal and again, your call whether or not to pursue this very minor cleanup. Ok for the trunk as is a patch which makes either or both of the trivial changes noted above. Jeff
diff --git gcc/testsuite/gcc.dg/pr64223-1.c gcc/testsuite/gcc.dg/pr64223-1.c index e69de29..015bfd8 100644 --- gcc/testsuite/gcc.dg/pr64223-1.c +++ gcc/testsuite/gcc.dg/pr64223-1.c @@ -0,0 +1,12 @@ +/* PR c/64223: Test for duplicated warnings. */ +/* { dg-do compile } */ +/* { dg-options "-Wformat" } */ + +int printf (const char *, ...) __attribute__ ((__format__ (__printf__, 1, 2))); + +void +foo (void) +{ + printf ("%d\n", 0UL); /* { dg-bogus "expects argument of type.*expects argument of type" } */ + /* { dg-warning "expects argument of type" "" { target *-*-* } 10 } */ +} diff --git gcc/testsuite/gcc.dg/pr64223-2.c gcc/testsuite/gcc.dg/pr64223-2.c index e69de29..2a1627e 100644 --- gcc/testsuite/gcc.dg/pr64223-2.c +++ gcc/testsuite/gcc.dg/pr64223-2.c @@ -0,0 +1,13 @@ +/* PR c/64223: Test for duplicated warnings. */ +/* { dg-do compile } */ +/* { dg-options "-Wformat" } */ + +int myprintf (const char *, ...) __attribute__ ((__format__ (printf, 1, 2))); +int myprintf (const char *, ...) __attribute__ ((__format__ (__printf__, 1, 2))); + +void +foo (void) +{ + myprintf ("%d\n", 0UL); /* { dg-bogus "expects argument of type.*expects argument of type" } */ + /* { dg-warning "expects argument of type" "" { target *-*-* } 11 } */ +} diff --git gcc/tree.c gcc/tree.c index 6297f04..a58ad7b 100644 --- gcc/tree.c +++ gcc/tree.c @@ -4871,9 +4871,53 @@ simple_cst_list_equal (const_tree l1, const_tree l2) return l1 == l2; } +/* Compare two identifier nodes representing attributes. Either one may + be in prefixed __ATTR__ form. Return true if they are the same, false + otherwise. */ + +static bool +cmp_attrib_identifiers (const_tree attr1, const_tree attr2) +{ + /* Make sure we're dealing with IDENTIFIER_NODEs. */ + gcc_checking_assert (TREE_CODE (attr1) == IDENTIFIER_NODE + && TREE_CODE (attr2) == IDENTIFIER_NODE); + + /* Identifiers can be compared directly for equality. */ + if (attr1 == attr2) + return true; + + /* If they are not equal, they may still be one in the form + 'text' while the other one is in the form '__text__'. TODO: + If we were storing attributes in normalized 'text' form, then + this could all go away and we could take full advantage of + the fact that we're comparing identifiers. :-) */ + const size_t attr1_len = IDENTIFIER_LENGTH (attr1); + const size_t attr2_len = IDENTIFIER_LENGTH (attr2); + + if (attr2_len == attr1_len + 4) + { + const char *p = IDENTIFIER_POINTER (attr2); + const char *q = IDENTIFIER_POINTER (attr1); + if (p[0] == '_' && p[1] == '_' + && p[attr2_len - 2] == '_' && p[attr2_len - 1] == '_' + && strncmp (q, p + 2, attr1_len) == 0) + return true;; + } + else if (attr2_len + 4 == attr1_len) + { + const char *p = IDENTIFIER_POINTER (attr2); + const char *q = IDENTIFIER_POINTER (attr1); + if (q[0] == '_' && q[1] == '_' + && q[attr1_len - 2] == '_' && q[attr1_len - 1] == '_' + && strncmp (q + 2, p, attr2_len) == 0) + return true; + } + + return false; +} + /* Compare two attributes for their value identity. Return true if the - attribute values are known to be equal; otherwise return false. -*/ + attribute values are known to be equal; otherwise return false. */ bool attribute_value_equal (const_tree attr1, const_tree attr2) @@ -4883,10 +4927,25 @@ attribute_value_equal (const_tree attr1, const_tree attr2) if (TREE_VALUE (attr1) != NULL_TREE && TREE_CODE (TREE_VALUE (attr1)) == TREE_LIST - && TREE_VALUE (attr2) != NULL + && TREE_VALUE (attr2) != NULL_TREE && TREE_CODE (TREE_VALUE (attr2)) == TREE_LIST) - return (simple_cst_list_equal (TREE_VALUE (attr1), - TREE_VALUE (attr2)) == 1); + { + /* Handle attribute format. */ + if (is_attribute_p ("format", TREE_PURPOSE (attr1))) + { + attr1 = TREE_VALUE (attr1); + attr2 = TREE_VALUE (attr2); + /* Compare the archetypes (printf/scanf/strftime/...). */ + if (!cmp_attrib_identifiers (TREE_VALUE (attr1), + TREE_VALUE (attr2))) + return false; + /* Archetypes are the same. Compare the rest. */ + return (simple_cst_list_equal (TREE_CHAIN (attr1), + TREE_CHAIN (attr2)) == 1); + } + return (simple_cst_list_equal (TREE_VALUE (attr1), + TREE_VALUE (attr2)) == 1); + } if ((flag_openmp || flag_openmp_simd) && TREE_VALUE (attr1) && TREE_VALUE (attr2) @@ -6038,38 +6097,10 @@ lookup_ident_attribute (tree attr_identifier, tree list) gcc_checking_assert (TREE_CODE (get_attribute_name (list)) == IDENTIFIER_NODE); - /* Identifiers can be compared directly for equality. */ - if (attr_identifier == get_attribute_name (list)) + if (cmp_attrib_identifiers (attr_identifier, + get_attribute_name (list))) + /* Found it. */ break; - - /* If they are not equal, they may still be one in the form - 'text' while the other one is in the form '__text__'. TODO: - If we were storing attributes in normalized 'text' form, then - this could all go away and we could take full advantage of - the fact that we're comparing identifiers. :-) */ - { - size_t attr_len = IDENTIFIER_LENGTH (attr_identifier); - size_t ident_len = IDENTIFIER_LENGTH (get_attribute_name (list)); - - if (ident_len == attr_len + 4) - { - const char *p = IDENTIFIER_POINTER (get_attribute_name (list)); - const char *q = IDENTIFIER_POINTER (attr_identifier); - if (p[0] == '_' && p[1] == '_' - && p[ident_len - 2] == '_' && p[ident_len - 1] == '_' - && strncmp (q, p + 2, attr_len) == 0) - break; - } - else if (ident_len + 4 == attr_len) - { - const char *p = IDENTIFIER_POINTER (get_attribute_name (list)); - const char *q = IDENTIFIER_POINTER (attr_identifier); - if (q[0] == '_' && q[1] == '_' - && q[attr_len - 2] == '_' && q[attr_len - 1] == '_' - && strncmp (q + 2, p, ident_len) == 0) - break; - } - } list = TREE_CHAIN (list); }