* [PATCH] c-family: ICE with [[gnu::nocf_check]] [PR106937] @ 2022-09-29 22:49 Marek Polacek 2022-09-30 13:12 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-09-29 22:49 UTC (permalink / raw) To: GCC Patches, Jason Merrill, Joseph Myers When getting the name of an attribute, we ought to use get_attribute_name, which handles both [[ ]] and __attribute__(()) forms. Failure to do so may result in an ICE, like here. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? PR c++/106937 gcc/c-family/ChangeLog: * c-pretty-print.cc (pp_c_attributes): Use get_attribute_name. (pp_c_attributes_display): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/fcf-protection-1.c: New test. --- gcc/c-family/c-pretty-print.cc | 8 ++++---- gcc/testsuite/gcc.dg/fcf-protection-1.c | 13 +++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/fcf-protection-1.c diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc index efa1768f4d6..91f88b830e3 100644 --- a/gcc/c-family/c-pretty-print.cc +++ b/gcc/c-family/c-pretty-print.cc @@ -863,7 +863,7 @@ pp_c_attributes (c_pretty_printer *pp, tree attributes) pp_c_left_paren (pp); for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) { - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); + pp_tree_identifier (pp, get_attribute_name (attributes)); if (TREE_VALUE (attributes)) pp_c_call_argument_list (pp, TREE_VALUE (attributes)); @@ -875,7 +875,7 @@ pp_c_attributes (c_pretty_printer *pp, tree attributes) } /* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes - marked to be displayed on disgnostic. */ + marked to be displayed on diagnostic. */ void pp_c_attributes_display (c_pretty_printer *pp, tree a) @@ -888,7 +888,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) for (; a != NULL_TREE; a = TREE_CHAIN (a)) { const struct attribute_spec *as; - as = lookup_attribute_spec (TREE_PURPOSE (a)); + as = lookup_attribute_spec (get_attribute_name (a)); if (!as || as->affects_type_identity == false) continue; if (c_dialect_cxx () @@ -906,7 +906,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) { pp_separate_with (pp, ','); } - pp_tree_identifier (pp, TREE_PURPOSE (a)); + pp_tree_identifier (pp, get_attribute_name (a)); if (TREE_VALUE (a)) pp_c_call_argument_list (pp, TREE_VALUE (a)); } diff --git a/gcc/testsuite/gcc.dg/fcf-protection-1.c b/gcc/testsuite/gcc.dg/fcf-protection-1.c new file mode 100644 index 00000000000..9d06feadfd1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fcf-protection-1.c @@ -0,0 +1,13 @@ +/* PR c++/106937 */ +/* { dg-options "-fcf-protection -w" } */ + +[[gnu::nocf_check]] typedef void (*FuncPointerWithNoCfCheck)(void); +typedef void (*FuncPointer)(void); +[[gnu::nocf_check]] void testNoCfCheck(); +void testNoCfCheck(){}; +int [[gnu::nocf_check]] i; +void testNoCfCheckImpl(double i [[gnu::nocf_check]]) {} +void testNoCfCheckMismatch(FuncPointer f) { + FuncPointerWithNoCfCheck fNoCfCheck = f; + (*fNoCfCheck)(); +} base-commit: c2ee70f20de8133a88553270073226b0f3f55f62 -- 2.37.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-09-29 22:49 [PATCH] c-family: ICE with [[gnu::nocf_check]] [PR106937] Marek Polacek @ 2022-09-30 13:12 ` Jason Merrill 2022-10-04 23:06 ` [PATCH v2] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-09-30 13:12 UTC (permalink / raw) To: Marek Polacek, GCC Patches, Joseph Myers On 9/29/22 18:49, Marek Polacek wrote: > When getting the name of an attribute, we ought to use > get_attribute_name, which handles both [[ ]] and __attribute__(()) > forms. Failure to do so may result in an ICE, like here. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? How do we print the attributes with this patch? Don't we also want to print the namespace, and use [[]] in the output? > PR c++/106937 > > gcc/c-family/ChangeLog: > > * c-pretty-print.cc (pp_c_attributes): Use get_attribute_name. > (pp_c_attributes_display): Likewise. > > gcc/testsuite/ChangeLog: > > * gcc.dg/fcf-protection-1.c: New test. > --- > gcc/c-family/c-pretty-print.cc | 8 ++++---- > gcc/testsuite/gcc.dg/fcf-protection-1.c | 13 +++++++++++++ > 2 files changed, 17 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/fcf-protection-1.c > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > index efa1768f4d6..91f88b830e3 100644 > --- a/gcc/c-family/c-pretty-print.cc > +++ b/gcc/c-family/c-pretty-print.cc > @@ -863,7 +863,7 @@ pp_c_attributes (c_pretty_printer *pp, tree attributes) > pp_c_left_paren (pp); > for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) > { > - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); > + pp_tree_identifier (pp, get_attribute_name (attributes)); > if (TREE_VALUE (attributes)) > pp_c_call_argument_list (pp, TREE_VALUE (attributes)); > > @@ -875,7 +875,7 @@ pp_c_attributes (c_pretty_printer *pp, tree attributes) > } > > /* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes > - marked to be displayed on disgnostic. */ > + marked to be displayed on diagnostic. */ > > void > pp_c_attributes_display (c_pretty_printer *pp, tree a) > @@ -888,7 +888,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > for (; a != NULL_TREE; a = TREE_CHAIN (a)) > { > const struct attribute_spec *as; > - as = lookup_attribute_spec (TREE_PURPOSE (a)); > + as = lookup_attribute_spec (get_attribute_name (a)); > if (!as || as->affects_type_identity == false) > continue; > if (c_dialect_cxx () > @@ -906,7 +906,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > { > pp_separate_with (pp, ','); > } > - pp_tree_identifier (pp, TREE_PURPOSE (a)); > + pp_tree_identifier (pp, get_attribute_name (a)); > if (TREE_VALUE (a)) > pp_c_call_argument_list (pp, TREE_VALUE (a)); > } > diff --git a/gcc/testsuite/gcc.dg/fcf-protection-1.c b/gcc/testsuite/gcc.dg/fcf-protection-1.c > new file mode 100644 > index 00000000000..9d06feadfd1 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/fcf-protection-1.c > @@ -0,0 +1,13 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection -w" } */ > + > +[[gnu::nocf_check]] typedef void (*FuncPointerWithNoCfCheck)(void); > +typedef void (*FuncPointer)(void); > +[[gnu::nocf_check]] void testNoCfCheck(); > +void testNoCfCheck(){}; > +int [[gnu::nocf_check]] i; > +void testNoCfCheckImpl(double i [[gnu::nocf_check]]) {} > +void testNoCfCheckMismatch(FuncPointer f) { > + FuncPointerWithNoCfCheck fNoCfCheck = f; > + (*fNoCfCheck)(); > +} > > base-commit: c2ee70f20de8133a88553270073226b0f3f55f62 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-09-30 13:12 ` Jason Merrill @ 2022-10-04 23:06 ` Marek Polacek 2022-10-06 21:42 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-10-04 23:06 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Joseph Myers On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: > On 9/29/22 18:49, Marek Polacek wrote: > > When getting the name of an attribute, we ought to use > > get_attribute_name, which handles both [[ ]] and __attribute__(()) > > forms. Failure to do so may result in an ICE, like here. > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > How do we print the attributes with this patch? Don't we also want to print > the namespace, and use [[]] in the output? Good point, however: while the testcase indeed has an attribute in the [[]] form in the typedef, here we're printing its "aka": warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. I could do that, but then we'd print aka 'void ([[nocf_check]] *)(void)' in the above, but that's invalid syntax! pp_c_attributes_display appears to be called for * and & only where you can't use an [[]] attribute. So perhaps we want to keep printing the GNU form here? I noticed that pp_c_attributes has never been used, so we can just remove it. I've also adjusted the test not to use "-w". Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- When getting the name of an attribute, we ought to use get_attribute_name, which handles both [[ ]] and __attribute__(()) forms. Failure to do so may result in an ICE, like here. pp_c_attributes has been unused since its introduction in r56273. PR c++/106937 gcc/c-family/ChangeLog: * c-pretty-print.cc (pp_c_attributes): Remove. (pp_c_attributes_display): Use get_attribute_name. * c-pretty-print.h (pp_c_attributes): Remove. gcc/testsuite/ChangeLog: * gcc.dg/fcf-protection-1.c: New test. --- gcc/c-family/c-pretty-print.cc | 30 +++---------------------- gcc/c-family/c-pretty-print.h | 1 - gcc/testsuite/gcc.dg/fcf-protection-1.c | 13 +++++++++++ 3 files changed, 16 insertions(+), 28 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/fcf-protection-1.c diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc index efa1768f4d6..2419e149333 100644 --- a/gcc/c-family/c-pretty-print.cc +++ b/gcc/c-family/c-pretty-print.cc @@ -850,32 +850,8 @@ c_pretty_printer::declaration (tree t) pp_c_init_declarator (this, t); } -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ - -void -pp_c_attributes (c_pretty_printer *pp, tree attributes) -{ - if (attributes == NULL_TREE) - return; - - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) - { - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); - if (TREE_VALUE (attributes)) - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); - - if (TREE_CHAIN (attributes)) - pp_separate_with (pp, ','); - } - pp_c_right_paren (pp); - pp_c_right_paren (pp); -} - /* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes - marked to be displayed on disgnostic. */ + marked to be displayed on diagnostic. */ void pp_c_attributes_display (c_pretty_printer *pp, tree a) @@ -888,7 +864,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) for (; a != NULL_TREE; a = TREE_CHAIN (a)) { const struct attribute_spec *as; - as = lookup_attribute_spec (TREE_PURPOSE (a)); + as = lookup_attribute_spec (get_attribute_name (a)); if (!as || as->affects_type_identity == false) continue; if (c_dialect_cxx () @@ -906,7 +882,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) { pp_separate_with (pp, ','); } - pp_tree_identifier (pp, TREE_PURPOSE (a)); + pp_tree_identifier (pp, get_attribute_name (a)); if (TREE_VALUE (a)) pp_c_call_argument_list (pp, TREE_VALUE (a)); } diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h index be86bed4fee..92674ab4d06 100644 --- a/gcc/c-family/c-pretty-print.h +++ b/gcc/c-family/c-pretty-print.h @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); /* Declarations. */ void pp_c_tree_decl_identifier (c_pretty_printer *, tree); void pp_c_function_definition (c_pretty_printer *, tree); -void pp_c_attributes (c_pretty_printer *, tree); void pp_c_attributes_display (c_pretty_printer *, tree); void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); void pp_c_type_qualifier_list (c_pretty_printer *, tree); diff --git a/gcc/testsuite/gcc.dg/fcf-protection-1.c b/gcc/testsuite/gcc.dg/fcf-protection-1.c new file mode 100644 index 00000000000..baad74cd86f --- /dev/null +++ b/gcc/testsuite/gcc.dg/fcf-protection-1.c @@ -0,0 +1,13 @@ +/* PR c++/106937 */ +/* { dg-options "-fcf-protection" } */ + +[[gnu::nocf_check]] typedef void (*FuncPointerWithNoCfCheck)(void); +typedef void (*FuncPointer)(void); +[[gnu::nocf_check]] void testNoCfCheck(); +void testNoCfCheck(){}; +int i; +void testNoCfCheckImpl(double i) {} +void testNoCfCheckMismatch(FuncPointer f) { + FuncPointerWithNoCfCheck fNoCfCheck = f; /* { dg-warning "initialization" } */ + (*fNoCfCheck)(); +} base-commit: ade1e0d5896221500d1cbda38cd631cf80325aaa -- 2.37.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-04 23:06 ` [PATCH v2] " Marek Polacek @ 2022-10-06 21:42 ` Jason Merrill 2022-10-07 2:12 ` [PATCH v3] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-10-06 21:42 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Joseph Myers On 10/4/22 19:06, Marek Polacek wrote: > On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: >> On 9/29/22 18:49, Marek Polacek wrote: >>> When getting the name of an attribute, we ought to use >>> get_attribute_name, which handles both [[ ]] and __attribute__(()) >>> forms. Failure to do so may result in an ICE, like here. >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >> >> How do we print the attributes with this patch? Don't we also want to print >> the namespace, and use [[]] in the output? > > Good point, however: while the testcase indeed has an attribute > in the [[]] form in the typedef, here we're printing its "aka": > > warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} > > c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. > I could do that, but then we'd print > > aka 'void ([[nocf_check]] *)(void)' > > in the above, but that's invalid syntax! Indeed, it should be void (* [[gnu::nocf_check]])(void) > pp_c_attributes_display appears > to be called for * and & only where you can't use an [[]] attribute. So > perhaps we want to keep printing the GNU form here? > > I noticed that pp_c_attributes has never been used, so we can just remove it. > > I've also adjusted the test not to use "-w". > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > When getting the name of an attribute, we ought to use > get_attribute_name, which handles both [[ ]] and __attribute__(()) > forms. Failure to do so may result in an ICE, like here. > > pp_c_attributes has been unused since its introduction in r56273. > > PR c++/106937 > > gcc/c-family/ChangeLog: > > * c-pretty-print.cc (pp_c_attributes): Remove. > (pp_c_attributes_display): Use get_attribute_name. > * c-pretty-print.h (pp_c_attributes): Remove. > > gcc/testsuite/ChangeLog: > > * gcc.dg/fcf-protection-1.c: New test. > --- > gcc/c-family/c-pretty-print.cc | 30 +++---------------------- > gcc/c-family/c-pretty-print.h | 1 - > gcc/testsuite/gcc.dg/fcf-protection-1.c | 13 +++++++++++ > 3 files changed, 16 insertions(+), 28 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/fcf-protection-1.c > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > index efa1768f4d6..2419e149333 100644 > --- a/gcc/c-family/c-pretty-print.cc > +++ b/gcc/c-family/c-pretty-print.cc > @@ -850,32 +850,8 @@ c_pretty_printer::declaration (tree t) > pp_c_init_declarator (this, t); > } > > -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ > - > -void > -pp_c_attributes (c_pretty_printer *pp, tree attributes) > -{ > - if (attributes == NULL_TREE) > - return; > - > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) > - { > - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); > - if (TREE_VALUE (attributes)) > - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); > - > - if (TREE_CHAIN (attributes)) > - pp_separate_with (pp, ','); > - } > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > -} > - > /* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes > - marked to be displayed on disgnostic. */ > + marked to be displayed on diagnostic. */ > > void > pp_c_attributes_display (c_pretty_printer *pp, tree a) > @@ -888,7 +864,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > for (; a != NULL_TREE; a = TREE_CHAIN (a)) > { > const struct attribute_spec *as; > - as = lookup_attribute_spec (TREE_PURPOSE (a)); > + as = lookup_attribute_spec (get_attribute_name (a)); > if (!as || as->affects_type_identity == false) > continue; > if (c_dialect_cxx () > @@ -906,7 +882,7 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > { > pp_separate_with (pp, ','); > } > - pp_tree_identifier (pp, TREE_PURPOSE (a)); > + pp_tree_identifier (pp, get_attribute_name (a)); > if (TREE_VALUE (a)) > pp_c_call_argument_list (pp, TREE_VALUE (a)); > } > diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h > index be86bed4fee..92674ab4d06 100644 > --- a/gcc/c-family/c-pretty-print.h > +++ b/gcc/c-family/c-pretty-print.h > @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); > /* Declarations. */ > void pp_c_tree_decl_identifier (c_pretty_printer *, tree); > void pp_c_function_definition (c_pretty_printer *, tree); > -void pp_c_attributes (c_pretty_printer *, tree); > void pp_c_attributes_display (c_pretty_printer *, tree); > void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); > void pp_c_type_qualifier_list (c_pretty_printer *, tree); > diff --git a/gcc/testsuite/gcc.dg/fcf-protection-1.c b/gcc/testsuite/gcc.dg/fcf-protection-1.c > new file mode 100644 > index 00000000000..baad74cd86f > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/fcf-protection-1.c > @@ -0,0 +1,13 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection" } */ > + > +[[gnu::nocf_check]] typedef void (*FuncPointerWithNoCfCheck)(void); > +typedef void (*FuncPointer)(void); > +[[gnu::nocf_check]] void testNoCfCheck(); > +void testNoCfCheck(){}; > +int i; > +void testNoCfCheckImpl(double i) {} > +void testNoCfCheckMismatch(FuncPointer f) { > + FuncPointerWithNoCfCheck fNoCfCheck = f; /* { dg-warning "initialization" } */ > + (*fNoCfCheck)(); > +} > > base-commit: ade1e0d5896221500d1cbda38cd631cf80325aaa ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-06 21:42 ` Jason Merrill @ 2022-10-07 2:12 ` Marek Polacek 2022-10-07 16:17 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-10-07 2:12 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Joseph Myers On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: > On 10/4/22 19:06, Marek Polacek wrote: > > On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: > > > On 9/29/22 18:49, Marek Polacek wrote: > > > > When getting the name of an attribute, we ought to use > > > > get_attribute_name, which handles both [[ ]] and __attribute__(()) > > > > forms. Failure to do so may result in an ICE, like here. > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > How do we print the attributes with this patch? Don't we also want to print > > > the namespace, and use [[]] in the output? > > > > Good point, however: while the testcase indeed has an attribute > > in the [[]] form in the typedef, here we're printing its "aka": > > > > warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} > > > > c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. > > I could do that, but then we'd print > > > > aka 'void ([[nocf_check]] *)(void)' > > > > in the above, but that's invalid syntax! > > Indeed, it should be > > void (* [[gnu::nocf_check]])(void) Ok, let me fix that too, then. I've updated the description to show what we print with the patch, and what was printed before. And I also noticed another thing: type_hash_canon_hash hashes only get_attribute_name, so build_type_attribute_qual_variant will use the same function_type tree for these two pointers: __attribute__((nocf_check)) typedef void (*fp1)(); [[gnu::nocf_check]] typedef void (*fp2)(); so if gcc prints a diagnostic with fp2, it will show the GNU form of the attribute, because the already existing type of fp1 will be used! I'm not addressing it here, just an oddity I discovered. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- When getting the name of an attribute, we ought to use get_attribute_name, which handles both [[]] and __attribute__(()) forms. Failure to do so may result in an ICE, like here. pp_c_attributes_display wasn't able to print the [[]] form of attributes, so this patch teaches it to. When printing a pointer to function with an attribute, the attribute should be printed after the *, because that's what it appertains to. With this patch we print: aka 'void (*__attribute__((nocf_check)))()' aka 'void (*[[gnu::nocf_check]])()' rather than aka 'void (__attribute__((nocf_check)) *)() or crashing in the second case. pp_c_attributes has been unused since its introduction in r56273 so this patch removes it. PR c++/106937 gcc/c-family/ChangeLog: * c-pretty-print.cc (pp_c_specifier_qualifier_list): When printing a pointer to function with an attribute, print the attribute after the *. (pp_c_attributes): Remove. (pp_c_attributes_display): Print the [[]] form if appropriate. Use get_attribute_name. Don't print a trailing space. * c-pretty-print.h (pp_c_attributes): Remove. gcc/cp/ChangeLog: * error.cc (dump_type_prefix): When printing a pointer/reference to function with an attribute, print the attribute after the */&. gcc/testsuite/ChangeLog: * g++.dg/diagnostic/aka5a.C: Adjust. * g++.dg/diagnostic/aka5b.C: Likewise. * gcc.dg/diag-aka-5a.c: Likewise. * gcc.dg/diag-aka-5b.c: Likewise. * c-c++-common/pointer-to-fn1.c: New test. --- gcc/c-family/c-pretty-print.cc | 92 +++++++++++---------- gcc/c-family/c-pretty-print.h | 1 - gcc/cp/error.cc | 4 +- gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ gcc/testsuite/g++.dg/diagnostic/aka5a.C | 8 +- gcc/testsuite/g++.dg/diagnostic/aka5b.C | 9 +- gcc/testsuite/gcc.dg/diag-aka-5a.c | 8 +- gcc/testsuite/gcc.dg/diag-aka-5b.c | 8 +- 8 files changed, 84 insertions(+), 64 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc index efa1768f4d6..2523939cffb 100644 --- a/gcc/c-family/c-pretty-print.cc +++ b/gcc/c-family/c-pretty-print.cc @@ -466,11 +466,16 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) { pp_c_whitespace (pp); pp_c_left_paren (pp); + pp_ptr_operator (pp, t); pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); } - else if (!c_dialect_cxx ()) - pp_c_whitespace (pp); - pp_ptr_operator (pp, t); + else + { + /* Removing this WS inconsistency breaks too many tests. */ + if (!c_dialect_cxx ()) + pp_c_whitespace (pp); + pp_ptr_operator (pp, t); + } } break; @@ -850,32 +855,7 @@ c_pretty_printer::declaration (tree t) pp_c_init_declarator (this, t); } -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ - -void -pp_c_attributes (c_pretty_printer *pp, tree attributes) -{ - if (attributes == NULL_TREE) - return; - - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) - { - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); - if (TREE_VALUE (attributes)) - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); - - if (TREE_CHAIN (attributes)) - pp_separate_with (pp, ','); - } - pp_c_right_paren (pp); - pp_c_right_paren (pp); -} - -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes - marked to be displayed on disgnostic. */ +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ void pp_c_attributes_display (c_pretty_printer *pp, tree a) @@ -885,10 +865,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) if (a == NULL_TREE) return; + const bool std_p = cxx11_attribute_p (a); + for (; a != NULL_TREE; a = TREE_CHAIN (a)) { - const struct attribute_spec *as; - as = lookup_attribute_spec (TREE_PURPOSE (a)); + const struct attribute_spec *as + = lookup_attribute_spec (get_attribute_name (a)); if (!as || as->affects_type_identity == false) continue; if (c_dialect_cxx () @@ -896,26 +878,46 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) /* In C++ transaction_safe is printed at the end of the declarator. */ continue; if (is_first) - { - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - is_first = false; - } + { + if (std_p) + { + pp_c_left_bracket (pp); + pp_c_left_bracket (pp); + } + else + { + pp_c_ws_string (pp, "__attribute__"); + pp_c_left_paren (pp); + pp_c_left_paren (pp); + } + is_first = false; + } else - { - pp_separate_with (pp, ','); - } - pp_tree_identifier (pp, TREE_PURPOSE (a)); + pp_separate_with (pp, ','); + tree ns; + if (std_p && (ns = get_attribute_namespace (a))) + { + pp_tree_identifier (pp, ns); + pp_colon (pp); + pp_colon (pp); + } + pp_tree_identifier (pp, get_attribute_name (a)); if (TREE_VALUE (a)) - pp_c_call_argument_list (pp, TREE_VALUE (a)); + pp_c_call_argument_list (pp, TREE_VALUE (a)); } if (!is_first) { - pp_c_right_paren (pp); - pp_c_right_paren (pp); - pp_c_whitespace (pp); + if (std_p) + { + pp_c_right_bracket (pp); + pp_c_right_bracket (pp); + } + else + { + pp_c_right_paren (pp); + pp_c_right_paren (pp); + } } } diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h index be86bed4fee..92674ab4d06 100644 --- a/gcc/c-family/c-pretty-print.h +++ b/gcc/c-family/c-pretty-print.h @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); /* Declarations. */ void pp_c_tree_decl_identifier (c_pretty_printer *, tree); void pp_c_function_definition (c_pretty_printer *, tree); -void pp_c_attributes (c_pretty_printer *, tree); void pp_c_attributes_display (c_pretty_printer *, tree); void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); void pp_c_type_qualifier_list (c_pretty_printer *, tree); diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 4514c8bbb44..4ce59157383 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -897,7 +897,6 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) { pp_cxx_whitespace (pp); pp_cxx_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); } if (TYPE_PTR_P (t)) pp_star (pp); @@ -908,6 +907,9 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) else pp_ampersand (pp); } + if (TREE_CODE (sub) == ARRAY_TYPE + || TREE_CODE (sub) == FUNCTION_TYPE) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); pp->padding = pp_before; pp_cxx_cv_qualifier_seq (pp, t); } diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c new file mode 100644 index 00000000000..8b842e0774a --- /dev/null +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c @@ -0,0 +1,18 @@ +/* PR c++/106937 */ +/* { dg-options "-fcf-protection" } */ +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ +/* Test printing a pointer to function with attribute. */ + +__attribute__((nocf_check)) typedef void (*FPA1)(); +[[gnu::nocf_check]] typedef void (*FPA2)(int); +typedef void (*FP1)(); +typedef void (*FP2)(int); + +void +g (FP1 f1, FP2 f2) +{ + FPA1 p1 = f1; // { dg-warning {aka .void \(\*__attribute__\(\(nocf_check\)\)\)\(\)} } + FPA2 p2 = f2; // { dg-warning {aka .void \(\*\[\[gnu::nocf_check\]\]\)\(int\)} } + FP1 p3 = p1; // { dg-warning {aka .void \(\*__attribute__\(\(nocf_check\)\)\)\(\)} } + FP2 p4 = p2; // { dg-warning {aka .void \(\*\[\[gnu::nocf_check\]\]\)\(int\)} } +} diff --git a/gcc/testsuite/g++.dg/diagnostic/aka5a.C b/gcc/testsuite/g++.dg/diagnostic/aka5a.C index e9d4c02f61d..94923c298dd 100644 --- a/gcc/testsuite/g++.dg/diagnostic/aka5a.C +++ b/gcc/testsuite/g++.dg/diagnostic/aka5a.C @@ -82,8 +82,8 @@ void f (s s1) ue_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*\)\(\)' {aka 'user_enum \(\*\)\(\)'} in assignment} } ue_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(user_enum\)'} in assignment} } ue_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(user_enum, \.\.\.\)'} in assignment} } - unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' in assignment} } - unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } + unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' in assignment} } + unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } us1 = s1; // { dg-error {no match for 'operator=' in 'us1 = s1' \(operand types are 'user_struct' and 's'\)} } us2 = s1; // { dg-error {no match for 'operator=' in 'us2 = s1' \(operand types are 'user_struct_copy' {aka 'user_struct'} and 's'\)} } @@ -122,6 +122,6 @@ void f (s s1) ui_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*\)\(\)' {aka 'int \(\*\)\(\)'} in assignment} } ui_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} in assignment} } ui_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} in assignment} } - unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } - unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } + unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } + unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } } diff --git a/gcc/testsuite/g++.dg/diagnostic/aka5b.C b/gcc/testsuite/g++.dg/diagnostic/aka5b.C index 6942be3eef1..45a0ce378f6 100644 --- a/gcc/testsuite/g++.dg/diagnostic/aka5b.C +++ b/gcc/testsuite/g++.dg/diagnostic/aka5b.C @@ -81,8 +81,8 @@ void f (s s1) ue_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*\)\(\)' {aka '__internal_enum \(\*\)\(\)'} in assignment} } ue_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(__internal_enum\)'} in assignment} } ue_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(__internal_enum, \.\.\.\)'} in assignment} } - unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka '__internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } - unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka '__internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } + unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka '__internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } + unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka '__internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } us1 = s1; // { dg-error {no match for 'operator=' in 'us1 = s1' \(operand types are 'user_struct' {aka '__internal_struct'} and 's'\)} } us2 = s1; // { dg-error {no match for 'operator=' in 'us2 = s1' \(operand types are 'user_struct_copy' {aka '__internal_struct'} and 's'\)} } @@ -121,7 +121,6 @@ void f (s s1) ui_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*\)\(\)' {aka 'int \(\*\)\(\)'} in assignment} } ui_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} in assignment} } ui_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} in assignment} } - unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } - unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } + unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } + unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } } - diff --git a/gcc/testsuite/gcc.dg/diag-aka-5a.c b/gcc/testsuite/gcc.dg/diag-aka-5a.c index 8768a79204a..227c01c16db 100644 --- a/gcc/testsuite/gcc.dg/diag-aka-5a.c +++ b/gcc/testsuite/gcc.dg/diag-aka-5a.c @@ -88,8 +88,8 @@ void f (struct s s) ue_fn_ptr4 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*\)\(void\)' {aka 'user_enum \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ ue_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(user_enum\)'} from incompatible pointer type 'struct s \*'} } */ ue_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(user_enum, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' from incompatible pointer type 'struct s \*'} } */ - unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' from incompatible pointer type 'struct s \*'} } */ + unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ us1 = s; /* { dg-error {assigning to type 'user_struct' from type 'struct s'} } */ us2 = s; /* { dg-error {assigning to type 'user_struct_copy' {aka 'user_struct'} from type 'struct s'} } */ @@ -130,6 +130,6 @@ void f (struct s s) ui_fn_ptr4 = &s; /* { dg-error {assignment to 'user_int_copy \(\*\)\(void\)' {aka 'int \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ ui_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} from incompatible pointer type 'struct s \*'} } */ ui_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ } diff --git a/gcc/testsuite/gcc.dg/diag-aka-5b.c b/gcc/testsuite/gcc.dg/diag-aka-5b.c index e0ec7c816a2..848b1b76888 100644 --- a/gcc/testsuite/gcc.dg/diag-aka-5b.c +++ b/gcc/testsuite/gcc.dg/diag-aka-5b.c @@ -87,8 +87,8 @@ void f (struct s s) ue_fn_ptr4 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*\)\(void\)' {aka 'enum __internal_enum \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ ue_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(enum __internal_enum\)'} from incompatible pointer type 'struct s \*'} } */ ue_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(enum __internal_enum, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'enum __internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'enum __internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'enum __internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'enum __internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ us1 = s; /* { dg-error {assigning to type 'user_struct' {aka 'struct __internal_struct'} from type 'struct s'} } */ us2 = s; /* { dg-error {assigning to type 'user_struct_copy' {aka 'struct __internal_struct'} from type 'struct s'} } */ @@ -129,6 +129,6 @@ void f (struct s s) ui_fn_ptr4 = &s; /* { dg-error {assignment to 'user_int_copy \(\*\)\(void\)' {aka 'int \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ ui_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} from incompatible pointer type 'struct s \*'} } */ ui_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ - unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ + unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ } base-commit: 629d04d35d819bdc26c30d215bc4ea66a74af15b -- 2.37.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-07 2:12 ` [PATCH v3] " Marek Polacek @ 2022-10-07 16:17 ` Jason Merrill 2022-10-07 21:08 ` [PATCH v4] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-10-07 16:17 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Joseph Myers On 10/6/22 22:12, Marek Polacek wrote: > On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: >> On 10/4/22 19:06, Marek Polacek wrote: >>> On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: >>>> On 9/29/22 18:49, Marek Polacek wrote: >>>>> When getting the name of an attribute, we ought to use >>>>> get_attribute_name, which handles both [[ ]] and __attribute__(()) >>>>> forms. Failure to do so may result in an ICE, like here. >>>>> >>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>> >>>> How do we print the attributes with this patch? Don't we also want to print >>>> the namespace, and use [[]] in the output? >>> >>> Good point, however: while the testcase indeed has an attribute >>> in the [[]] form in the typedef, here we're printing its "aka": >>> >>> warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} >>> >>> c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. >>> I could do that, but then we'd print >>> >>> aka 'void ([[nocf_check]] *)(void)' >>> >>> in the above, but that's invalid syntax! >> >> Indeed, it should be >> >> void (* [[gnu::nocf_check]])(void) > > Ok, let me fix that too, then. I've updated the description to show > what we print with the patch, and what was printed before. Oops, apparently I was wrongly assuming that the attribute appertained to the pointer. Since it actually appertains to the function type, it should be void (*)(void) [[gnu::nocf_check]]. But for GCC attribute syntax, I think we want to leave the __attribute__ where it was before, as in the manual's example void (__attribute__((noreturn)) ****f) (void); > And I also noticed another thing: type_hash_canon_hash hashes only > get_attribute_name, so build_type_attribute_qual_variant will use the > same function_type tree for these two pointers: > > __attribute__((nocf_check)) typedef void (*fp1)(); > [[gnu::nocf_check]] typedef void (*fp2)(); > > so if gcc prints a diagnostic with fp2, it will show the GNU form of > the attribute, because the already existing type of fp1 will be used! > I'm not addressing it here, just an oddity I discovered. That seems acceptable; they are the same type, just spelled differently. > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > When getting the name of an attribute, we ought to use > get_attribute_name, which handles both [[]] and __attribute__(()) > forms. Failure to do so may result in an ICE, like here. > > pp_c_attributes_display wasn't able to print the [[]] form of > attributes, so this patch teaches it to. > > When printing a pointer to function with an attribute, the attribute > should be printed after the *, because that's what it appertains to. > With this patch we print: > > aka 'void (*__attribute__((nocf_check)))()' > aka 'void (*[[gnu::nocf_check]])()' > > rather than > > aka 'void (__attribute__((nocf_check)) *)() > > or crashing in the second case. > > pp_c_attributes has been unused since its introduction in r56273 so > this patch removes it. > > PR c++/106937 > > gcc/c-family/ChangeLog: > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): When > printing a pointer to function with an attribute, print the > attribute after the *. > (pp_c_attributes): Remove. > (pp_c_attributes_display): Print the [[]] form if appropriate. > Use get_attribute_name. Don't print a trailing space. > * c-pretty-print.h (pp_c_attributes): Remove. > > gcc/cp/ChangeLog: > > * error.cc (dump_type_prefix): When printing a pointer/reference to > function with an attribute, print the attribute after the */&. > > gcc/testsuite/ChangeLog: > > * g++.dg/diagnostic/aka5a.C: Adjust. > * g++.dg/diagnostic/aka5b.C: Likewise. > * gcc.dg/diag-aka-5a.c: Likewise. > * gcc.dg/diag-aka-5b.c: Likewise. > * c-c++-common/pointer-to-fn1.c: New test. > --- > gcc/c-family/c-pretty-print.cc | 92 +++++++++++---------- > gcc/c-family/c-pretty-print.h | 1 - > gcc/cp/error.cc | 4 +- > gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ > gcc/testsuite/g++.dg/diagnostic/aka5a.C | 8 +- > gcc/testsuite/g++.dg/diagnostic/aka5b.C | 9 +- > gcc/testsuite/gcc.dg/diag-aka-5a.c | 8 +- > gcc/testsuite/gcc.dg/diag-aka-5b.c | 8 +- > 8 files changed, 84 insertions(+), 64 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > index efa1768f4d6..2523939cffb 100644 > --- a/gcc/c-family/c-pretty-print.cc > +++ b/gcc/c-family/c-pretty-print.cc > @@ -466,11 +466,16 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) > { > pp_c_whitespace (pp); > pp_c_left_paren (pp); > + pp_ptr_operator (pp, t); > pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > } > - else if (!c_dialect_cxx ()) > - pp_c_whitespace (pp); > - pp_ptr_operator (pp, t); > + else > + { > + /* Removing this WS inconsistency breaks too many tests. */ > + if (!c_dialect_cxx ()) > + pp_c_whitespace (pp); > + pp_ptr_operator (pp, t); > + } > } > break; > > @@ -850,32 +855,7 @@ c_pretty_printer::declaration (tree t) > pp_c_init_declarator (this, t); > } > > -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ > - > -void > -pp_c_attributes (c_pretty_printer *pp, tree attributes) > -{ > - if (attributes == NULL_TREE) > - return; > - > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) > - { > - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); > - if (TREE_VALUE (attributes)) > - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); > - > - if (TREE_CHAIN (attributes)) > - pp_separate_with (pp, ','); > - } > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > -} > - > -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes > - marked to be displayed on disgnostic. */ > +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ > > void > pp_c_attributes_display (c_pretty_printer *pp, tree a) > @@ -885,10 +865,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > if (a == NULL_TREE) > return; > > + const bool std_p = cxx11_attribute_p (a); > + > for (; a != NULL_TREE; a = TREE_CHAIN (a)) > { > - const struct attribute_spec *as; > - as = lookup_attribute_spec (TREE_PURPOSE (a)); > + const struct attribute_spec *as > + = lookup_attribute_spec (get_attribute_name (a)); > if (!as || as->affects_type_identity == false) > continue; > if (c_dialect_cxx () > @@ -896,26 +878,46 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > /* In C++ transaction_safe is printed at the end of the declarator. */ > continue; > if (is_first) > - { > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - is_first = false; > - } > + { > + if (std_p) > + { > + pp_c_left_bracket (pp); > + pp_c_left_bracket (pp); > + } > + else > + { > + pp_c_ws_string (pp, "__attribute__"); > + pp_c_left_paren (pp); > + pp_c_left_paren (pp); > + } > + is_first = false; > + } > else > - { > - pp_separate_with (pp, ','); > - } > - pp_tree_identifier (pp, TREE_PURPOSE (a)); > + pp_separate_with (pp, ','); > + tree ns; > + if (std_p && (ns = get_attribute_namespace (a))) > + { > + pp_tree_identifier (pp, ns); > + pp_colon (pp); > + pp_colon (pp); > + } > + pp_tree_identifier (pp, get_attribute_name (a)); > if (TREE_VALUE (a)) > - pp_c_call_argument_list (pp, TREE_VALUE (a)); > + pp_c_call_argument_list (pp, TREE_VALUE (a)); > } > > if (!is_first) > { > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > - pp_c_whitespace (pp); > + if (std_p) > + { > + pp_c_right_bracket (pp); > + pp_c_right_bracket (pp); > + } > + else > + { > + pp_c_right_paren (pp); > + pp_c_right_paren (pp); > + } > } > } > > diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h > index be86bed4fee..92674ab4d06 100644 > --- a/gcc/c-family/c-pretty-print.h > +++ b/gcc/c-family/c-pretty-print.h > @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); > /* Declarations. */ > void pp_c_tree_decl_identifier (c_pretty_printer *, tree); > void pp_c_function_definition (c_pretty_printer *, tree); > -void pp_c_attributes (c_pretty_printer *, tree); > void pp_c_attributes_display (c_pretty_printer *, tree); > void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); > void pp_c_type_qualifier_list (c_pretty_printer *, tree); > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc > index 4514c8bbb44..4ce59157383 100644 > --- a/gcc/cp/error.cc > +++ b/gcc/cp/error.cc > @@ -897,7 +897,6 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) > { > pp_cxx_whitespace (pp); > pp_cxx_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > } > if (TYPE_PTR_P (t)) > pp_star (pp); > @@ -908,6 +907,9 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) > else > pp_ampersand (pp); > } > + if (TREE_CODE (sub) == ARRAY_TYPE > + || TREE_CODE (sub) == FUNCTION_TYPE) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > pp->padding = pp_before; > pp_cxx_cv_qualifier_seq (pp, t); > } > diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > new file mode 100644 > index 00000000000..8b842e0774a > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > @@ -0,0 +1,18 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection" } */ > +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ > +/* Test printing a pointer to function with attribute. */ > + > +__attribute__((nocf_check)) typedef void (*FPA1)(); > +[[gnu::nocf_check]] typedef void (*FPA2)(int); > +typedef void (*FP1)(); > +typedef void (*FP2)(int); > + > +void > +g (FP1 f1, FP2 f2) > +{ > + FPA1 p1 = f1; // { dg-warning {aka .void \(\*__attribute__\(\(nocf_check\)\)\)\(\)} } > + FPA2 p2 = f2; // { dg-warning {aka .void \(\*\[\[gnu::nocf_check\]\]\)\(int\)} } > + FP1 p3 = p1; // { dg-warning {aka .void \(\*__attribute__\(\(nocf_check\)\)\)\(\)} } > + FP2 p4 = p2; // { dg-warning {aka .void \(\*\[\[gnu::nocf_check\]\]\)\(int\)} } > +} > diff --git a/gcc/testsuite/g++.dg/diagnostic/aka5a.C b/gcc/testsuite/g++.dg/diagnostic/aka5a.C > index e9d4c02f61d..94923c298dd 100644 > --- a/gcc/testsuite/g++.dg/diagnostic/aka5a.C > +++ b/gcc/testsuite/g++.dg/diagnostic/aka5a.C > @@ -82,8 +82,8 @@ void f (s s1) > ue_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*\)\(\)' {aka 'user_enum \(\*\)\(\)'} in assignment} } > ue_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(user_enum\)'} in assignment} } > ue_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(user_enum, \.\.\.\)'} in assignment} } > - unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' in assignment} } > - unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > + unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' in assignment} } > + unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > > us1 = s1; // { dg-error {no match for 'operator=' in 'us1 = s1' \(operand types are 'user_struct' and 's'\)} } > us2 = s1; // { dg-error {no match for 'operator=' in 'us2 = s1' \(operand types are 'user_struct_copy' {aka 'user_struct'} and 's'\)} } > @@ -122,6 +122,6 @@ void f (s s1) > ui_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*\)\(\)' {aka 'int \(\*\)\(\)'} in assignment} } > ui_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} in assignment} } > ui_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} in assignment} } > - unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > - unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > + unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > + unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > } > diff --git a/gcc/testsuite/g++.dg/diagnostic/aka5b.C b/gcc/testsuite/g++.dg/diagnostic/aka5b.C > index 6942be3eef1..45a0ce378f6 100644 > --- a/gcc/testsuite/g++.dg/diagnostic/aka5b.C > +++ b/gcc/testsuite/g++.dg/diagnostic/aka5b.C > @@ -81,8 +81,8 @@ void f (s s1) > ue_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*\)\(\)' {aka '__internal_enum \(\*\)\(\)'} in assignment} } > ue_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(__internal_enum\)'} in assignment} } > ue_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(__internal_enum, \.\.\.\)'} in assignment} } > - unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka '__internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > - unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka '__internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > + unsafe_ue_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka '__internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > + unsafe_ue_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka '__internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > > us1 = s1; // { dg-error {no match for 'operator=' in 'us1 = s1' \(operand types are 'user_struct' {aka '__internal_struct'} and 's'\)} } > us2 = s1; // { dg-error {no match for 'operator=' in 'us2 = s1' \(operand types are 'user_struct_copy' {aka '__internal_struct'} and 's'\)} } > @@ -121,7 +121,6 @@ void f (s s1) > ui_fn_ptr4 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*\)\(\)' {aka 'int \(\*\)\(\)'} in assignment} } > ui_fn_ptr5 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} in assignment} } > ui_fn_ptr6 = &s1; // { dg-error {cannot convert 's\*' to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} in assignment} } > - unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > - unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(\)'} in assignment} } > + unsafe_ui_fn_ptr1 = &s1; // { dg-error {cannot convert 's\*' to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > + unsafe_ui_fn_ptr2 = &s1; // { dg-error {cannot convert 's\*' to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(\)'} in assignment} } > } > - > diff --git a/gcc/testsuite/gcc.dg/diag-aka-5a.c b/gcc/testsuite/gcc.dg/diag-aka-5a.c > index 8768a79204a..227c01c16db 100644 > --- a/gcc/testsuite/gcc.dg/diag-aka-5a.c > +++ b/gcc/testsuite/gcc.dg/diag-aka-5a.c > @@ -88,8 +88,8 @@ void f (struct s s) > ue_fn_ptr4 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*\)\(void\)' {aka 'user_enum \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > ue_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(user_enum\)'} from incompatible pointer type 'struct s \*'} } */ > ue_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(user_enum, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' from incompatible pointer type 'struct s \*'} } */ > - unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' from incompatible pointer type 'struct s \*'} } */ > + unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > > us1 = s; /* { dg-error {assigning to type 'user_struct' from type 'struct s'} } */ > us2 = s; /* { dg-error {assigning to type 'user_struct_copy' {aka 'user_struct'} from type 'struct s'} } */ > @@ -130,6 +130,6 @@ void f (struct s s) > ui_fn_ptr4 = &s; /* { dg-error {assignment to 'user_int_copy \(\*\)\(void\)' {aka 'int \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > ui_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} from incompatible pointer type 'struct s \*'} } */ > ui_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > } > diff --git a/gcc/testsuite/gcc.dg/diag-aka-5b.c b/gcc/testsuite/gcc.dg/diag-aka-5b.c > index e0ec7c816a2..848b1b76888 100644 > --- a/gcc/testsuite/gcc.dg/diag-aka-5b.c > +++ b/gcc/testsuite/gcc.dg/diag-aka-5b.c > @@ -87,8 +87,8 @@ void f (struct s s) > ue_fn_ptr4 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*\)\(void\)' {aka 'enum __internal_enum \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > ue_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy\)' {aka 'void \(\*\)\(enum __internal_enum\)'} from incompatible pointer type 'struct s \*'} } */ > ue_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_enum_copy, \.\.\.\)' {aka 'void \(\*\)\(enum __internal_enum, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'enum __internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'enum __internal_enum \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ue_fn_ptr1 = &s; /* { dg-error {assignment to 'user_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'enum __internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ue_fn_ptr2 = &s; /* { dg-error {assignment to 'user_enum_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'enum __internal_enum \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > > us1 = s; /* { dg-error {assigning to type 'user_struct' {aka 'struct __internal_struct'} from type 'struct s'} } */ > us2 = s; /* { dg-error {assigning to type 'user_struct_copy' {aka 'struct __internal_struct'} from type 'struct s'} } */ > @@ -129,6 +129,6 @@ void f (struct s s) > ui_fn_ptr4 = &s; /* { dg-error {assignment to 'user_int_copy \(\*\)\(void\)' {aka 'int \(\*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > ui_fn_ptr5 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy\)' {aka 'void \(\*\)\(int\)'} from incompatible pointer type 'struct s \*'} } */ > ui_fn_ptr6 = &s; /* { dg-error {assignment to 'void \(\*\)\(user_int_copy, \.\.\.\)' {aka 'void \(\*\)\(int, \.\.\.\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > - unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)' {aka 'int \(__attribute__\(\(transaction_unsafe\)\) \*\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ui_fn_ptr1 = &s; /* { dg-error {assignment to 'user_int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > + unsafe_ui_fn_ptr2 = &s; /* { dg-error {assignment to 'user_int_copy \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)' {aka 'int \(\*__attribute__\(\(transaction_unsafe\)\)\)\(void\)'} from incompatible pointer type 'struct s \*'} } */ > } > > base-commit: 629d04d35d819bdc26c30d215bc4ea66a74af15b ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-07 16:17 ` Jason Merrill @ 2022-10-07 21:08 ` Marek Polacek 2022-10-07 21:56 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-10-07 21:08 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Joseph Myers On Fri, Oct 07, 2022 at 12:17:34PM -0400, Jason Merrill wrote: > On 10/6/22 22:12, Marek Polacek wrote: > > On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: > > > On 10/4/22 19:06, Marek Polacek wrote: > > > > On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: > > > > > On 9/29/22 18:49, Marek Polacek wrote: > > > > > > When getting the name of an attribute, we ought to use > > > > > > get_attribute_name, which handles both [[ ]] and __attribute__(()) > > > > > > forms. Failure to do so may result in an ICE, like here. > > > > > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > > > How do we print the attributes with this patch? Don't we also want to print > > > > > the namespace, and use [[]] in the output? > > > > > > > > Good point, however: while the testcase indeed has an attribute > > > > in the [[]] form in the typedef, here we're printing its "aka": > > > > > > > > warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} > > > > > > > > c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. > > > > I could do that, but then we'd print > > > > > > > > aka 'void ([[nocf_check]] *)(void)' > > > > > > > > in the above, but that's invalid syntax! > > > > > > Indeed, it should be > > > > > > void (* [[gnu::nocf_check]])(void) > > > > Ok, let me fix that too, then. I've updated the description to show > > what we print with the patch, and what was printed before. > > Oops, apparently I was wrongly assuming that the attribute appertained to > the pointer. So was I :/. > Since it actually appertains to the function type, it should > be > > void (*)(void) [[gnu::nocf_check]]. > > But for GCC attribute syntax, I think we want to leave the __attribute__ > where it was before, as in the manual's example > > void (__attribute__((noreturn)) ****f) (void); Thanks, done here: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- When getting the name of an attribute, we ought to use get_attribute_name, which handles both [[]] and __attribute__(()) forms. Failure to do so may result in an ICE, like here. pp_c_attributes_display wasn't able to print the [[]] form of attributes, so this patch teaches it to. When printing a pointer to function with a standard attribute, the attribute should be printed after the parameter-list. With this patch we print: aka 'void (*)(int) [[gnu::nocf_check]]' pp_c_attributes has been unused since its introduction in r56273 so this patch removes it. PR c++/106937 gcc/c-family/ChangeLog: * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print GNU attributes here. (c_pretty_printer::abstract_declarator): For a pointer to function, print the standard [[]] attributes here. (pp_c_attributes): Remove. (pp_c_attributes_display): Print the [[]] form if appropriate. Use get_attribute_name. Don't print a trailing space when printing the [[]] form. * c-pretty-print.h (pp_c_attributes): Remove. gcc/cp/ChangeLog: * error.cc: Include "attribs.h". (dump_type_prefix): Only print GNU attributes here. (dump_type_suffix): Print standard attributes here. gcc/testsuite/ChangeLog: * c-c++-common/pointer-to-fn1.c: New test. --- gcc/c-family/c-pretty-print.cc | 102 +++++++++++--------- gcc/c-family/c-pretty-print.h | 1 - gcc/cp/error.cc | 17 +++- gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ 4 files changed, 92 insertions(+), 46 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc index efa1768f4d6..349f0a07d3e 100644 --- a/gcc/c-family/c-pretty-print.cc +++ b/gcc/c-family/c-pretty-print.cc @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) { pp_c_whitespace (pp); pp_c_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); + /* If we're dealing with the GNU form of attributes, print this: + void (__attribute__((noreturn)) *f) (); + If it is the standard [[]] attribute, we'll print the attribute + in c_pretty_printer::abstract_declarator. */ + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); } else if (!c_dialect_cxx ()) pp_c_whitespace (pp); @@ -564,15 +569,26 @@ pp_c_parameter_type_list (c_pretty_printer *pp, tree t) void c_pretty_printer::abstract_declarator (tree t) { + bool fn_ptr_p = false; if (TREE_CODE (t) == POINTER_TYPE) { if (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE || TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) - pp_c_right_paren (this); + { + pp_c_right_paren (this); + fn_ptr_p = true; + } t = TREE_TYPE (t); } direct_abstract_declarator (t); + /* If this is the standard [[]] attribute, print + void (*)() [[noreturn]]; */ + if (fn_ptr_p && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) + { + pp_space (this); + pp_c_attributes_display (this, TYPE_ATTRIBUTES (t)); + } } /* direct-abstract-declarator: @@ -850,32 +866,7 @@ c_pretty_printer::declaration (tree t) pp_c_init_declarator (this, t); } -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ - -void -pp_c_attributes (c_pretty_printer *pp, tree attributes) -{ - if (attributes == NULL_TREE) - return; - - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) - { - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); - if (TREE_VALUE (attributes)) - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); - - if (TREE_CHAIN (attributes)) - pp_separate_with (pp, ','); - } - pp_c_right_paren (pp); - pp_c_right_paren (pp); -} - -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes - marked to be displayed on disgnostic. */ +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ void pp_c_attributes_display (c_pretty_printer *pp, tree a) @@ -885,10 +876,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) if (a == NULL_TREE) return; + const bool std_p = cxx11_attribute_p (a); + for (; a != NULL_TREE; a = TREE_CHAIN (a)) { - const struct attribute_spec *as; - as = lookup_attribute_spec (TREE_PURPOSE (a)); + const struct attribute_spec *as + = lookup_attribute_spec (get_attribute_name (a)); if (!as || as->affects_type_identity == false) continue; if (c_dialect_cxx () @@ -896,26 +889,47 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) /* In C++ transaction_safe is printed at the end of the declarator. */ continue; if (is_first) - { - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - is_first = false; - } + { + if (std_p) + { + pp_c_left_bracket (pp); + pp_c_left_bracket (pp); + } + else + { + pp_c_ws_string (pp, "__attribute__"); + pp_c_left_paren (pp); + pp_c_left_paren (pp); + } + is_first = false; + } else - { - pp_separate_with (pp, ','); - } - pp_tree_identifier (pp, TREE_PURPOSE (a)); + pp_separate_with (pp, ','); + tree ns; + if (std_p && (ns = get_attribute_namespace (a))) + { + pp_tree_identifier (pp, ns); + pp_colon (pp); + pp_colon (pp); + } + pp_tree_identifier (pp, get_attribute_name (a)); if (TREE_VALUE (a)) - pp_c_call_argument_list (pp, TREE_VALUE (a)); + pp_c_call_argument_list (pp, TREE_VALUE (a)); } if (!is_first) { - pp_c_right_paren (pp); - pp_c_right_paren (pp); - pp_c_whitespace (pp); + if (std_p) + { + pp_c_right_bracket (pp); + pp_c_right_bracket (pp); + } + else + { + pp_c_right_paren (pp); + pp_c_right_paren (pp); + pp_c_whitespace (pp); + } } } diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h index be86bed4fee..92674ab4d06 100644 --- a/gcc/c-family/c-pretty-print.h +++ b/gcc/c-family/c-pretty-print.h @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); /* Declarations. */ void pp_c_tree_decl_identifier (c_pretty_printer *, tree); void pp_c_function_definition (c_pretty_printer *, tree); -void pp_c_attributes (c_pretty_printer *, tree); void pp_c_attributes_display (c_pretty_printer *, tree); void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); void pp_c_type_qualifier_list (c_pretty_printer *, tree); diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 4514c8bbb44..8cd011fab0e 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see #include "internal-fn.h" #include "gcc-rich-location.h" #include "cp-name-hint.h" +#include "attribs.h" #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') @@ -897,7 +898,12 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) { pp_cxx_whitespace (pp); pp_cxx_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); + /* If we're dealing with the GNU form of attributes, print this: + void (__attribute__((noreturn)) *f) (); + If it is the standard [[]] attribute, we'll print the attribute + in dump_type_suffix. */ + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (sub))) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); } if (TYPE_PTR_P (t)) pp_star (pp); @@ -1027,6 +1033,15 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) TREE_CODE (t) == FUNCTION_TYPE && (flags & TFF_POINTER)); dump_ref_qualifier (pp, t, flags); + /* If this is the standard [[]] attribute, print + void (*)() [[noreturn]]; */ + if ((flags & TFF_POINTER) + && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) + { + pp_space (pp); + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); + pp->padding = pp_before; + } if (tx_safe_fn_type_p (t)) pp_cxx_ws_string (pp, "transaction_safe"); dump_exception_spec (pp, TYPE_RAISES_EXCEPTIONS (t), flags); diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c new file mode 100644 index 00000000000..975885462e9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c @@ -0,0 +1,18 @@ +/* PR c++/106937 */ +/* { dg-options "-fcf-protection" } */ +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ +/* Test printing a pointer to function with attribute. */ + +__attribute__((nocf_check)) typedef void (*FPA1)(); +[[gnu::nocf_check]] typedef void (*FPA2)(int); +typedef void (*FP1)(); +typedef void (*FP2)(int); + +void +g (FP1 f1, FP2 f2) +{ + FPA1 p1 = f1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } + FPA2 p2 = f2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } + FP1 p3 = p1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } + FP2 p4 = p2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } +} base-commit: f30e9fd33e56a5a721346ea6140722e1b193db42 -- 2.37.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-07 21:08 ` [PATCH v4] " Marek Polacek @ 2022-10-07 21:56 ` Jason Merrill 2022-10-07 22:16 ` Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-10-07 21:56 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Joseph Myers On 10/7/22 17:08, Marek Polacek wrote: > On Fri, Oct 07, 2022 at 12:17:34PM -0400, Jason Merrill wrote: >> On 10/6/22 22:12, Marek Polacek wrote: >>> On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: >>>> On 10/4/22 19:06, Marek Polacek wrote: >>>>> On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: >>>>>> On 9/29/22 18:49, Marek Polacek wrote: >>>>>>> When getting the name of an attribute, we ought to use >>>>>>> get_attribute_name, which handles both [[ ]] and __attribute__(()) >>>>>>> forms. Failure to do so may result in an ICE, like here. >>>>>>> >>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>>> >>>>>> How do we print the attributes with this patch? Don't we also want to print >>>>>> the namespace, and use [[]] in the output? >>>>> >>>>> Good point, however: while the testcase indeed has an attribute >>>>> in the [[]] form in the typedef, here we're printing its "aka": >>>>> >>>>> warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} >>>>> >>>>> c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. >>>>> I could do that, but then we'd print >>>>> >>>>> aka 'void ([[nocf_check]] *)(void)' >>>>> >>>>> in the above, but that's invalid syntax! >>>> >>>> Indeed, it should be >>>> >>>> void (* [[gnu::nocf_check]])(void) >>> >>> Ok, let me fix that too, then. I've updated the description to show >>> what we print with the patch, and what was printed before. >> >> Oops, apparently I was wrongly assuming that the attribute appertained to >> the pointer. > > So was I :/. > >> Since it actually appertains to the function type, it should >> be >> >> void (*)(void) [[gnu::nocf_check]]. >> >> But for GCC attribute syntax, I think we want to leave the __attribute__ >> where it was before, as in the manual's example >> >> void (__attribute__((noreturn)) ****f) (void); > > Thanks, done here: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > When getting the name of an attribute, we ought to use > get_attribute_name, which handles both [[]] and __attribute__(()) > forms. Failure to do so may result in an ICE, like here. > > pp_c_attributes_display wasn't able to print the [[]] form of > attributes, so this patch teaches it to. > > When printing a pointer to function with a standard attribute, the attribute > should be printed after the parameter-list. With this patch we print: > > aka 'void (*)(int) [[gnu::nocf_check]]' > > pp_c_attributes has been unused since its introduction in r56273 so > this patch removes it. > > PR c++/106937 > > gcc/c-family/ChangeLog: > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print > GNU attributes here. > (c_pretty_printer::abstract_declarator): For a pointer to function, > print the standard [[]] attributes here. > (pp_c_attributes): Remove. > (pp_c_attributes_display): Print the [[]] form if appropriate. Use > get_attribute_name. Don't print a trailing space when printing the > [[]] form. > * c-pretty-print.h (pp_c_attributes): Remove. > > gcc/cp/ChangeLog: > > * error.cc: Include "attribs.h". > (dump_type_prefix): Only print GNU attributes here. > (dump_type_suffix): Print standard attributes here. > > gcc/testsuite/ChangeLog: > > * c-c++-common/pointer-to-fn1.c: New test. > --- > gcc/c-family/c-pretty-print.cc | 102 +++++++++++--------- > gcc/c-family/c-pretty-print.h | 1 - > gcc/cp/error.cc | 17 +++- > gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ > 4 files changed, 92 insertions(+), 46 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > index efa1768f4d6..349f0a07d3e 100644 > --- a/gcc/c-family/c-pretty-print.cc > +++ b/gcc/c-family/c-pretty-print.cc > @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) > { > pp_c_whitespace (pp); > pp_c_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > + /* If we're dealing with the GNU form of attributes, print this: > + void (__attribute__((noreturn)) *f) (); > + If it is the standard [[]] attribute, we'll print the attribute > + in c_pretty_printer::abstract_declarator. */ > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > } > else if (!c_dialect_cxx ()) > pp_c_whitespace (pp); > @@ -564,15 +569,26 @@ pp_c_parameter_type_list (c_pretty_printer *pp, tree t) > void > c_pretty_printer::abstract_declarator (tree t) > { > + bool fn_ptr_p = false; > if (TREE_CODE (t) == POINTER_TYPE) > { > if (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE > || TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) > - pp_c_right_paren (this); > + { > + pp_c_right_paren (this); > + fn_ptr_p = true; > + } > t = TREE_TYPE (t); > } > > direct_abstract_declarator (t); > + /* If this is the standard [[]] attribute, print > + void (*)() [[noreturn]]; */ > + if (fn_ptr_p && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) Do you need to check fn_ptr_p? The attributes on a function type go here whether or not we were dealing with a pointer. > + { > + pp_space (this); > + pp_c_attributes_display (this, TYPE_ATTRIBUTES (t)); > + } > } > > /* direct-abstract-declarator: > @@ -850,32 +866,7 @@ c_pretty_printer::declaration (tree t) > pp_c_init_declarator (this, t); > } > > -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ > - > -void > -pp_c_attributes (c_pretty_printer *pp, tree attributes) > -{ > - if (attributes == NULL_TREE) > - return; > - > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) > - { > - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); > - if (TREE_VALUE (attributes)) > - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); > - > - if (TREE_CHAIN (attributes)) > - pp_separate_with (pp, ','); > - } > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > -} > - > -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes > - marked to be displayed on disgnostic. */ > +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ > > void > pp_c_attributes_display (c_pretty_printer *pp, tree a) > @@ -885,10 +876,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > if (a == NULL_TREE) > return; > > + const bool std_p = cxx11_attribute_p (a); > + > for (; a != NULL_TREE; a = TREE_CHAIN (a)) > { > - const struct attribute_spec *as; > - as = lookup_attribute_spec (TREE_PURPOSE (a)); > + const struct attribute_spec *as > + = lookup_attribute_spec (get_attribute_name (a)); > if (!as || as->affects_type_identity == false) > continue; > if (c_dialect_cxx () > @@ -896,26 +889,47 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > /* In C++ transaction_safe is printed at the end of the declarator. */ > continue; > if (is_first) > - { > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - is_first = false; > - } > + { > + if (std_p) > + { > + pp_c_left_bracket (pp); > + pp_c_left_bracket (pp); > + } > + else > + { > + pp_c_ws_string (pp, "__attribute__"); > + pp_c_left_paren (pp); > + pp_c_left_paren (pp); > + } > + is_first = false; > + } > else > - { > - pp_separate_with (pp, ','); > - } > - pp_tree_identifier (pp, TREE_PURPOSE (a)); > + pp_separate_with (pp, ','); > + tree ns; > + if (std_p && (ns = get_attribute_namespace (a))) > + { > + pp_tree_identifier (pp, ns); > + pp_colon (pp); > + pp_colon (pp); > + } > + pp_tree_identifier (pp, get_attribute_name (a)); > if (TREE_VALUE (a)) > - pp_c_call_argument_list (pp, TREE_VALUE (a)); > + pp_c_call_argument_list (pp, TREE_VALUE (a)); > } > > if (!is_first) > { > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > - pp_c_whitespace (pp); > + if (std_p) > + { > + pp_c_right_bracket (pp); > + pp_c_right_bracket (pp); > + } > + else > + { > + pp_c_right_paren (pp); > + pp_c_right_paren (pp); > + pp_c_whitespace (pp); > + } > } > } > > diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h > index be86bed4fee..92674ab4d06 100644 > --- a/gcc/c-family/c-pretty-print.h > +++ b/gcc/c-family/c-pretty-print.h > @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); > /* Declarations. */ > void pp_c_tree_decl_identifier (c_pretty_printer *, tree); > void pp_c_function_definition (c_pretty_printer *, tree); > -void pp_c_attributes (c_pretty_printer *, tree); > void pp_c_attributes_display (c_pretty_printer *, tree); > void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); > void pp_c_type_qualifier_list (c_pretty_printer *, tree); > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc > index 4514c8bbb44..8cd011fab0e 100644 > --- a/gcc/cp/error.cc > +++ b/gcc/cp/error.cc > @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see > #include "internal-fn.h" > #include "gcc-rich-location.h" > #include "cp-name-hint.h" > +#include "attribs.h" > > #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') > #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') > @@ -897,7 +898,12 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) > { > pp_cxx_whitespace (pp); > pp_cxx_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > + /* If we're dealing with the GNU form of attributes, print this: > + void (__attribute__((noreturn)) *f) (); > + If it is the standard [[]] attribute, we'll print the attribute > + in dump_type_suffix. */ > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (sub))) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > } > if (TYPE_PTR_P (t)) > pp_star (pp); > @@ -1027,6 +1033,15 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) > TREE_CODE (t) == FUNCTION_TYPE > && (flags & TFF_POINTER)); > dump_ref_qualifier (pp, t, flags); > + /* If this is the standard [[]] attribute, print > + void (*)() [[noreturn]]; */ > + if ((flags & TFF_POINTER) And likewise here. > + && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > + { > + pp_space (pp); > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); > + pp->padding = pp_before; > + } > if (tx_safe_fn_type_p (t)) > pp_cxx_ws_string (pp, "transaction_safe"); > dump_exception_spec (pp, TYPE_RAISES_EXCEPTIONS (t), flags); > diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > new file mode 100644 > index 00000000000..975885462e9 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > @@ -0,0 +1,18 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection" } */ > +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ > +/* Test printing a pointer to function with attribute. */ > + > +__attribute__((nocf_check)) typedef void (*FPA1)(); > +[[gnu::nocf_check]] typedef void (*FPA2)(int); > +typedef void (*FP1)(); > +typedef void (*FP2)(int); > + > +void > +g (FP1 f1, FP2 f2) > +{ > + FPA1 p1 = f1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } > + FPA2 p2 = f2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } > + FP1 p3 = p1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } > + FP2 p4 = p2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } > +} > > base-commit: f30e9fd33e56a5a721346ea6140722e1b193db42 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-07 21:56 ` Jason Merrill @ 2022-10-07 22:16 ` Marek Polacek 2022-10-10 14:49 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-10-07 22:16 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Joseph Myers On Fri, Oct 07, 2022 at 05:56:18PM -0400, Jason Merrill wrote: > On 10/7/22 17:08, Marek Polacek wrote: > > On Fri, Oct 07, 2022 at 12:17:34PM -0400, Jason Merrill wrote: > > > On 10/6/22 22:12, Marek Polacek wrote: > > > > On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: > > > > > On 10/4/22 19:06, Marek Polacek wrote: > > > > > > On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: > > > > > > > On 9/29/22 18:49, Marek Polacek wrote: > > > > > > > > When getting the name of an attribute, we ought to use > > > > > > > > get_attribute_name, which handles both [[ ]] and __attribute__(()) > > > > > > > > forms. Failure to do so may result in an ICE, like here. > > > > > > > > > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > > > > > > > How do we print the attributes with this patch? Don't we also want to print > > > > > > > the namespace, and use [[]] in the output? > > > > > > > > > > > > Good point, however: while the testcase indeed has an attribute > > > > > > in the [[]] form in the typedef, here we're printing its "aka": > > > > > > > > > > > > warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} > > > > > > > > > > > > c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. > > > > > > I could do that, but then we'd print > > > > > > > > > > > > aka 'void ([[nocf_check]] *)(void)' > > > > > > > > > > > > in the above, but that's invalid syntax! > > > > > > > > > > Indeed, it should be > > > > > > > > > > void (* [[gnu::nocf_check]])(void) > > > > > > > > Ok, let me fix that too, then. I've updated the description to show > > > > what we print with the patch, and what was printed before. > > > > > > Oops, apparently I was wrongly assuming that the attribute appertained to > > > the pointer. > > > > So was I :/. > > > > > Since it actually appertains to the function type, it should > > > be > > > > > > void (*)(void) [[gnu::nocf_check]]. > > > > > > But for GCC attribute syntax, I think we want to leave the __attribute__ > > > where it was before, as in the manual's example > > > > > > void (__attribute__((noreturn)) ****f) (void); > > > > Thanks, done here: > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > -- >8 -- > > When getting the name of an attribute, we ought to use > > get_attribute_name, which handles both [[]] and __attribute__(()) > > forms. Failure to do so may result in an ICE, like here. > > > > pp_c_attributes_display wasn't able to print the [[]] form of > > attributes, so this patch teaches it to. > > > > When printing a pointer to function with a standard attribute, the attribute > > should be printed after the parameter-list. With this patch we print: > > > > aka 'void (*)(int) [[gnu::nocf_check]]' > > > > pp_c_attributes has been unused since its introduction in r56273 so > > this patch removes it. > > > > PR c++/106937 > > > > gcc/c-family/ChangeLog: > > > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print > > GNU attributes here. > > (c_pretty_printer::abstract_declarator): For a pointer to function, > > print the standard [[]] attributes here. > > (pp_c_attributes): Remove. > > (pp_c_attributes_display): Print the [[]] form if appropriate. Use > > get_attribute_name. Don't print a trailing space when printing the > > [[]] form. > > * c-pretty-print.h (pp_c_attributes): Remove. > > > > gcc/cp/ChangeLog: > > > > * error.cc: Include "attribs.h". > > (dump_type_prefix): Only print GNU attributes here. > > (dump_type_suffix): Print standard attributes here. > > > > gcc/testsuite/ChangeLog: > > > > * c-c++-common/pointer-to-fn1.c: New test. > > --- > > gcc/c-family/c-pretty-print.cc | 102 +++++++++++--------- > > gcc/c-family/c-pretty-print.h | 1 - > > gcc/cp/error.cc | 17 +++- > > gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ > > 4 files changed, 92 insertions(+), 46 deletions(-) > > create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c > > > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > > index efa1768f4d6..349f0a07d3e 100644 > > --- a/gcc/c-family/c-pretty-print.cc > > +++ b/gcc/c-family/c-pretty-print.cc > > @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) > > { > > pp_c_whitespace (pp); > > pp_c_left_paren (pp); > > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > > + /* If we're dealing with the GNU form of attributes, print this: > > + void (__attribute__((noreturn)) *f) (); > > + If it is the standard [[]] attribute, we'll print the attribute > > + in c_pretty_printer::abstract_declarator. */ > > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) > > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > > } > > else if (!c_dialect_cxx ()) > > pp_c_whitespace (pp); > > @@ -564,15 +569,26 @@ pp_c_parameter_type_list (c_pretty_printer *pp, tree t) > > void > > c_pretty_printer::abstract_declarator (tree t) > > { > > + bool fn_ptr_p = false; > > if (TREE_CODE (t) == POINTER_TYPE) > > { > > if (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE > > || TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) > > - pp_c_right_paren (this); > > + { > > + pp_c_right_paren (this); > > + fn_ptr_p = true; > > + } > > t = TREE_TYPE (t); > > } > > direct_abstract_declarator (t); > > + /* If this is the standard [[]] attribute, print > > + void (*)() [[noreturn]]; */ > > + if (fn_ptr_p && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > > Do you need to check fn_ptr_p? The attributes on a function type go here > whether or not we were dealing with a pointer. I probably don't need it! Here's a version without that: So far all dg.exp tests passed. -- >8 -- When getting the name of an attribute, we ought to use get_attribute_name, which handles both [[]] and __attribute__(()) forms. Failure to do so may result in an ICE, like here. pp_c_attributes_display wasn't able to print the [[]] form of attributes, so this patch teaches it to. When printing a pointer to function with a standard attribute, the attribute should be printed after the parameter-list. With this patch we print: aka 'void (*)(int) [[gnu::nocf_check]]' pp_c_attributes has been unused since its introduction in r56273 so this patch removes it. PR c++/106937 gcc/c-family/ChangeLog: * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print GNU attributes here. (c_pretty_printer::abstract_declarator): For a pointer to function, print the standard [[]] attributes here. (pp_c_attributes): Remove. (pp_c_attributes_display): Print the [[]] form if appropriate. Use get_attribute_name. Don't print a trailing space when printing the [[]] form. * c-pretty-print.h (pp_c_attributes): Remove. gcc/cp/ChangeLog: * error.cc: Include "attribs.h". (dump_type_prefix): Only print GNU attributes here. (dump_type_suffix): Print standard attributes here. gcc/testsuite/ChangeLog: * c-c++-common/pointer-to-fn1.c: New test. --- gcc/c-family/c-pretty-print.cc | 96 ++++++++++++--------- gcc/c-family/c-pretty-print.h | 1 - gcc/cp/error.cc | 16 +++- gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ 4 files changed, 86 insertions(+), 45 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc index efa1768f4d6..7519b0982b7 100644 --- a/gcc/c-family/c-pretty-print.cc +++ b/gcc/c-family/c-pretty-print.cc @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) { pp_c_whitespace (pp); pp_c_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); + /* If we're dealing with the GNU form of attributes, print this: + void (__attribute__((noreturn)) *f) (); + If it is the standard [[]] attribute, we'll print the attribute + in c_pretty_printer::abstract_declarator. */ + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); } else if (!c_dialect_cxx ()) pp_c_whitespace (pp); @@ -573,6 +578,13 @@ c_pretty_printer::abstract_declarator (tree t) } direct_abstract_declarator (t); + /* If this is the standard [[]] attribute, print + void (*)() [[noreturn]]; */ + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) + { + pp_space (this); + pp_c_attributes_display (this, TYPE_ATTRIBUTES (t)); + } } /* direct-abstract-declarator: @@ -850,32 +862,7 @@ c_pretty_printer::declaration (tree t) pp_c_init_declarator (this, t); } -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ - -void -pp_c_attributes (c_pretty_printer *pp, tree attributes) -{ - if (attributes == NULL_TREE) - return; - - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) - { - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); - if (TREE_VALUE (attributes)) - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); - - if (TREE_CHAIN (attributes)) - pp_separate_with (pp, ','); - } - pp_c_right_paren (pp); - pp_c_right_paren (pp); -} - -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes - marked to be displayed on disgnostic. */ +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ void pp_c_attributes_display (c_pretty_printer *pp, tree a) @@ -885,10 +872,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) if (a == NULL_TREE) return; + const bool std_p = cxx11_attribute_p (a); + for (; a != NULL_TREE; a = TREE_CHAIN (a)) { - const struct attribute_spec *as; - as = lookup_attribute_spec (TREE_PURPOSE (a)); + const struct attribute_spec *as + = lookup_attribute_spec (get_attribute_name (a)); if (!as || as->affects_type_identity == false) continue; if (c_dialect_cxx () @@ -896,26 +885,47 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) /* In C++ transaction_safe is printed at the end of the declarator. */ continue; if (is_first) - { - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - is_first = false; - } + { + if (std_p) + { + pp_c_left_bracket (pp); + pp_c_left_bracket (pp); + } + else + { + pp_c_ws_string (pp, "__attribute__"); + pp_c_left_paren (pp); + pp_c_left_paren (pp); + } + is_first = false; + } else - { - pp_separate_with (pp, ','); - } - pp_tree_identifier (pp, TREE_PURPOSE (a)); + pp_separate_with (pp, ','); + tree ns; + if (std_p && (ns = get_attribute_namespace (a))) + { + pp_tree_identifier (pp, ns); + pp_colon (pp); + pp_colon (pp); + } + pp_tree_identifier (pp, get_attribute_name (a)); if (TREE_VALUE (a)) - pp_c_call_argument_list (pp, TREE_VALUE (a)); + pp_c_call_argument_list (pp, TREE_VALUE (a)); } if (!is_first) { - pp_c_right_paren (pp); - pp_c_right_paren (pp); - pp_c_whitespace (pp); + if (std_p) + { + pp_c_right_bracket (pp); + pp_c_right_bracket (pp); + } + else + { + pp_c_right_paren (pp); + pp_c_right_paren (pp); + pp_c_whitespace (pp); + } } } diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h index be86bed4fee..92674ab4d06 100644 --- a/gcc/c-family/c-pretty-print.h +++ b/gcc/c-family/c-pretty-print.h @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); /* Declarations. */ void pp_c_tree_decl_identifier (c_pretty_printer *, tree); void pp_c_function_definition (c_pretty_printer *, tree); -void pp_c_attributes (c_pretty_printer *, tree); void pp_c_attributes_display (c_pretty_printer *, tree); void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); void pp_c_type_qualifier_list (c_pretty_printer *, tree); diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 4514c8bbb44..c5239f6caf2 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see #include "internal-fn.h" #include "gcc-rich-location.h" #include "cp-name-hint.h" +#include "attribs.h" #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') @@ -897,7 +898,12 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) { pp_cxx_whitespace (pp); pp_cxx_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); + /* If we're dealing with the GNU form of attributes, print this: + void (__attribute__((noreturn)) *f) (); + If it is the standard [[]] attribute, we'll print the attribute + in dump_type_suffix. */ + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (sub))) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); } if (TYPE_PTR_P (t)) pp_star (pp); @@ -1027,6 +1033,14 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) TREE_CODE (t) == FUNCTION_TYPE && (flags & TFF_POINTER)); dump_ref_qualifier (pp, t, flags); + /* If this is the standard [[]] attribute, print + void (*)() [[noreturn]]; */ + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) + { + pp_space (pp); + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); + pp->padding = pp_before; + } if (tx_safe_fn_type_p (t)) pp_cxx_ws_string (pp, "transaction_safe"); dump_exception_spec (pp, TYPE_RAISES_EXCEPTIONS (t), flags); diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c new file mode 100644 index 00000000000..975885462e9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c @@ -0,0 +1,18 @@ +/* PR c++/106937 */ +/* { dg-options "-fcf-protection" } */ +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ +/* Test printing a pointer to function with attribute. */ + +__attribute__((nocf_check)) typedef void (*FPA1)(); +[[gnu::nocf_check]] typedef void (*FPA2)(int); +typedef void (*FP1)(); +typedef void (*FP2)(int); + +void +g (FP1 f1, FP2 f2) +{ + FPA1 p1 = f1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } + FPA2 p2 = f2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } + FP1 p3 = p1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } + FP2 p4 = p2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } +} base-commit: 895dd027d5dda51a95d242aec8a49a6dfa5db58d -- 2.37.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-07 22:16 ` Marek Polacek @ 2022-10-10 14:49 ` Jason Merrill 2022-10-10 19:18 ` [PATCH v5] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-10-10 14:49 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Joseph Myers On 10/7/22 18:16, Marek Polacek wrote: > On Fri, Oct 07, 2022 at 05:56:18PM -0400, Jason Merrill wrote: >> On 10/7/22 17:08, Marek Polacek wrote: >>> On Fri, Oct 07, 2022 at 12:17:34PM -0400, Jason Merrill wrote: >>>> On 10/6/22 22:12, Marek Polacek wrote: >>>>> On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: >>>>>> On 10/4/22 19:06, Marek Polacek wrote: >>>>>>> On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: >>>>>>>> On 9/29/22 18:49, Marek Polacek wrote: >>>>>>>>> When getting the name of an attribute, we ought to use >>>>>>>>> get_attribute_name, which handles both [[ ]] and __attribute__(()) >>>>>>>>> forms. Failure to do so may result in an ICE, like here. >>>>>>>>> >>>>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>>>>> >>>>>>>> How do we print the attributes with this patch? Don't we also want to print >>>>>>>> the namespace, and use [[]] in the output? >>>>>>> >>>>>>> Good point, however: while the testcase indeed has an attribute >>>>>>> in the [[]] form in the typedef, here we're printing its "aka": >>>>>>> >>>>>>> warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} >>>>>>> >>>>>>> c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. >>>>>>> I could do that, but then we'd print >>>>>>> >>>>>>> aka 'void ([[nocf_check]] *)(void)' >>>>>>> >>>>>>> in the above, but that's invalid syntax! >>>>>> >>>>>> Indeed, it should be >>>>>> >>>>>> void (* [[gnu::nocf_check]])(void) >>>>> >>>>> Ok, let me fix that too, then. I've updated the description to show >>>>> what we print with the patch, and what was printed before. >>>> >>>> Oops, apparently I was wrongly assuming that the attribute appertained to >>>> the pointer. >>> >>> So was I :/. >>> >>>> Since it actually appertains to the function type, it should >>>> be >>>> >>>> void (*)(void) [[gnu::nocf_check]]. >>>> >>>> But for GCC attribute syntax, I think we want to leave the __attribute__ >>>> where it was before, as in the manual's example >>>> >>>> void (__attribute__((noreturn)) ****f) (void); >>> >>> Thanks, done here: >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>> >>> -- >8 -- >>> When getting the name of an attribute, we ought to use >>> get_attribute_name, which handles both [[]] and __attribute__(()) >>> forms. Failure to do so may result in an ICE, like here. >>> >>> pp_c_attributes_display wasn't able to print the [[]] form of >>> attributes, so this patch teaches it to. >>> >>> When printing a pointer to function with a standard attribute, the attribute >>> should be printed after the parameter-list. With this patch we print: >>> >>> aka 'void (*)(int) [[gnu::nocf_check]]' >>> >>> pp_c_attributes has been unused since its introduction in r56273 so >>> this patch removes it. >>> >>> PR c++/106937 >>> >>> gcc/c-family/ChangeLog: >>> >>> * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print >>> GNU attributes here. >>> (c_pretty_printer::abstract_declarator): For a pointer to function, >>> print the standard [[]] attributes here. >>> (pp_c_attributes): Remove. >>> (pp_c_attributes_display): Print the [[]] form if appropriate. Use >>> get_attribute_name. Don't print a trailing space when printing the >>> [[]] form. >>> * c-pretty-print.h (pp_c_attributes): Remove. >>> >>> gcc/cp/ChangeLog: >>> >>> * error.cc: Include "attribs.h". >>> (dump_type_prefix): Only print GNU attributes here. >>> (dump_type_suffix): Print standard attributes here. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * c-c++-common/pointer-to-fn1.c: New test. >>> --- >>> gcc/c-family/c-pretty-print.cc | 102 +++++++++++--------- >>> gcc/c-family/c-pretty-print.h | 1 - >>> gcc/cp/error.cc | 17 +++- >>> gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ >>> 4 files changed, 92 insertions(+), 46 deletions(-) >>> create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c >>> >>> diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc >>> index efa1768f4d6..349f0a07d3e 100644 >>> --- a/gcc/c-family/c-pretty-print.cc >>> +++ b/gcc/c-family/c-pretty-print.cc >>> @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) >>> { >>> pp_c_whitespace (pp); >>> pp_c_left_paren (pp); >>> - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); >>> + /* If we're dealing with the GNU form of attributes, print this: >>> + void (__attribute__((noreturn)) *f) (); >>> + If it is the standard [[]] attribute, we'll print the attribute >>> + in c_pretty_printer::abstract_declarator. */ >>> + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) >>> + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); >>> } >>> else if (!c_dialect_cxx ()) >>> pp_c_whitespace (pp); >>> @@ -564,15 +569,26 @@ pp_c_parameter_type_list (c_pretty_printer *pp, tree t) >>> void >>> c_pretty_printer::abstract_declarator (tree t) >>> { >>> + bool fn_ptr_p = false; >>> if (TREE_CODE (t) == POINTER_TYPE) >>> { >>> if (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE >>> || TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) >>> - pp_c_right_paren (this); >>> + { >>> + pp_c_right_paren (this); >>> + fn_ptr_p = true; >>> + } >>> t = TREE_TYPE (t); >>> } >>> direct_abstract_declarator (t); >>> + /* If this is the standard [[]] attribute, print >>> + void (*)() [[noreturn]]; */ >>> + if (fn_ptr_p && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) >> >> Do you need to check fn_ptr_p? The attributes on a function type go here >> whether or not we were dealing with a pointer. > > I probably don't need it! Here's a version without that: > > So far all dg.exp tests passed. > > -- >8 -- > When getting the name of an attribute, we ought to use > get_attribute_name, which handles both [[]] and __attribute__(()) > forms. Failure to do so may result in an ICE, like here. > > pp_c_attributes_display wasn't able to print the [[]] form of > attributes, so this patch teaches it to. > > When printing a pointer to function with a standard attribute, the attribute > should be printed after the parameter-list. With this patch we print: > > aka 'void (*)(int) [[gnu::nocf_check]]' > > pp_c_attributes has been unused since its introduction in r56273 so > this patch removes it. > > PR c++/106937 > > gcc/c-family/ChangeLog: > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print > GNU attributes here. > (c_pretty_printer::abstract_declarator): For a pointer to function, > print the standard [[]] attributes here. This change now applies to all types, whereas the dump_type_suffix change only applies to functions. Maybe move this change into direct_abstract_declarator? > (pp_c_attributes): Remove. > (pp_c_attributes_display): Print the [[]] form if appropriate. Use > get_attribute_name. Don't print a trailing space when printing the > [[]] form. This is no longer accurate. > * c-pretty-print.h (pp_c_attributes): Remove. > > gcc/cp/ChangeLog: > > * error.cc: Include "attribs.h". > (dump_type_prefix): Only print GNU attributes here. > (dump_type_suffix): Print standard attributes here. See below. > > gcc/testsuite/ChangeLog: > > * c-c++-common/pointer-to-fn1.c: New test. > --- > gcc/c-family/c-pretty-print.cc | 96 ++++++++++++--------- > gcc/c-family/c-pretty-print.h | 1 - > gcc/cp/error.cc | 16 +++- > gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ > 4 files changed, 86 insertions(+), 45 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > index efa1768f4d6..7519b0982b7 100644 > --- a/gcc/c-family/c-pretty-print.cc > +++ b/gcc/c-family/c-pretty-print.cc > @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) > { > pp_c_whitespace (pp); > pp_c_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > + /* If we're dealing with the GNU form of attributes, print this: > + void (__attribute__((noreturn)) *f) (); > + If it is the standard [[]] attribute, we'll print the attribute > + in c_pretty_printer::abstract_declarator. */ > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > } > else if (!c_dialect_cxx ()) > pp_c_whitespace (pp); > @@ -573,6 +578,13 @@ c_pretty_printer::abstract_declarator (tree t) > } > > direct_abstract_declarator (t); > + /* If this is the standard [[]] attribute, print > + void (*)() [[noreturn]]; */ > + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > + { > + pp_space (this); > + pp_c_attributes_display (this, TYPE_ATTRIBUTES (t)); > + } > } > > /* direct-abstract-declarator: > @@ -850,32 +862,7 @@ c_pretty_printer::declaration (tree t) > pp_c_init_declarator (this, t); > } > > -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ > - > -void > -pp_c_attributes (c_pretty_printer *pp, tree attributes) > -{ > - if (attributes == NULL_TREE) > - return; > - > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) > - { > - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); > - if (TREE_VALUE (attributes)) > - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); > - > - if (TREE_CHAIN (attributes)) > - pp_separate_with (pp, ','); > - } > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > -} > - > -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes > - marked to be displayed on disgnostic. */ > +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ > > void > pp_c_attributes_display (c_pretty_printer *pp, tree a) > @@ -885,10 +872,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > if (a == NULL_TREE) > return; > > + const bool std_p = cxx11_attribute_p (a); > + > for (; a != NULL_TREE; a = TREE_CHAIN (a)) > { > - const struct attribute_spec *as; > - as = lookup_attribute_spec (TREE_PURPOSE (a)); > + const struct attribute_spec *as > + = lookup_attribute_spec (get_attribute_name (a)); > if (!as || as->affects_type_identity == false) > continue; > if (c_dialect_cxx () > @@ -896,26 +885,47 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > /* In C++ transaction_safe is printed at the end of the declarator. */ > continue; > if (is_first) > - { > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - is_first = false; > - } > + { > + if (std_p) > + { > + pp_c_left_bracket (pp); > + pp_c_left_bracket (pp); > + } > + else > + { > + pp_c_ws_string (pp, "__attribute__"); > + pp_c_left_paren (pp); > + pp_c_left_paren (pp); > + } > + is_first = false; > + } > else > - { > - pp_separate_with (pp, ','); > - } > - pp_tree_identifier (pp, TREE_PURPOSE (a)); > + pp_separate_with (pp, ','); > + tree ns; > + if (std_p && (ns = get_attribute_namespace (a))) > + { > + pp_tree_identifier (pp, ns); > + pp_colon (pp); > + pp_colon (pp); > + } > + pp_tree_identifier (pp, get_attribute_name (a)); > if (TREE_VALUE (a)) > - pp_c_call_argument_list (pp, TREE_VALUE (a)); > + pp_c_call_argument_list (pp, TREE_VALUE (a)); > } > > if (!is_first) > { > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > - pp_c_whitespace (pp); > + if (std_p) > + { > + pp_c_right_bracket (pp); > + pp_c_right_bracket (pp); > + } > + else > + { > + pp_c_right_paren (pp); > + pp_c_right_paren (pp); > + pp_c_whitespace (pp); > + } > } > } > > diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h > index be86bed4fee..92674ab4d06 100644 > --- a/gcc/c-family/c-pretty-print.h > +++ b/gcc/c-family/c-pretty-print.h > @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); > /* Declarations. */ > void pp_c_tree_decl_identifier (c_pretty_printer *, tree); > void pp_c_function_definition (c_pretty_printer *, tree); > -void pp_c_attributes (c_pretty_printer *, tree); > void pp_c_attributes_display (c_pretty_printer *, tree); > void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); > void pp_c_type_qualifier_list (c_pretty_printer *, tree); > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc > index 4514c8bbb44..c5239f6caf2 100644 > --- a/gcc/cp/error.cc > +++ b/gcc/cp/error.cc > @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see > #include "internal-fn.h" > #include "gcc-rich-location.h" > #include "cp-name-hint.h" > +#include "attribs.h" > > #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') > #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') > @@ -897,7 +898,12 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) > { > pp_cxx_whitespace (pp); > pp_cxx_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > + /* If we're dealing with the GNU form of attributes, print this: > + void (__attribute__((noreturn)) *f) (); > + If it is the standard [[]] attribute, we'll print the attribute > + in dump_type_suffix. */ > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (sub))) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > } > if (TYPE_PTR_P (t)) > pp_star (pp); > @@ -1027,6 +1033,14 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) > TREE_CODE (t) == FUNCTION_TYPE > && (flags & TFF_POINTER)); > dump_ref_qualifier (pp, t, flags); > + /* If this is the standard [[]] attribute, print > + void (*)() [[noreturn]]; */ > + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > + { > + pp_space (pp); > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); > + pp->padding = pp_before; > + } This is too soon, attributes come after the exception-specifier. > if (tx_safe_fn_type_p (t)) > pp_cxx_ws_string (pp, "transaction_safe"); > dump_exception_spec (pp, TYPE_RAISES_EXCEPTIONS (t), flags); > diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > new file mode 100644 > index 00000000000..975885462e9 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > @@ -0,0 +1,18 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection" } */ > +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ > +/* Test printing a pointer to function with attribute. */ > + > +__attribute__((nocf_check)) typedef void (*FPA1)(); > +[[gnu::nocf_check]] typedef void (*FPA2)(int); > +typedef void (*FP1)(); > +typedef void (*FP2)(int); > + > +void > +g (FP1 f1, FP2 f2) > +{ > + FPA1 p1 = f1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } > + FPA2 p2 = f2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } > + FP1 p3 = p1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } > + FP2 p4 = p2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } > +} > > base-commit: 895dd027d5dda51a95d242aec8a49a6dfa5db58d ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-10 14:49 ` Jason Merrill @ 2022-10-10 19:18 ` Marek Polacek 2022-10-10 19:23 ` Jason Merrill 2022-10-11 7:40 ` Andreas Schwab 0 siblings, 2 replies; 14+ messages in thread From: Marek Polacek @ 2022-10-10 19:18 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches, Joseph Myers On Mon, Oct 10, 2022 at 10:49:34AM -0400, Jason Merrill wrote: > On 10/7/22 18:16, Marek Polacek wrote: > > On Fri, Oct 07, 2022 at 05:56:18PM -0400, Jason Merrill wrote: > > > On 10/7/22 17:08, Marek Polacek wrote: > > > > On Fri, Oct 07, 2022 at 12:17:34PM -0400, Jason Merrill wrote: > > > > > On 10/6/22 22:12, Marek Polacek wrote: > > > > > > On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: > > > > > > > On 10/4/22 19:06, Marek Polacek wrote: > > > > > > > > On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: > > > > > > > > > On 9/29/22 18:49, Marek Polacek wrote: > > > > > > > > > > When getting the name of an attribute, we ought to use > > > > > > > > > > get_attribute_name, which handles both [[ ]] and __attribute__(()) > > > > > > > > > > forms. Failure to do so may result in an ICE, like here. > > > > > > > > > > > > > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > > > > > > > > > > > How do we print the attributes with this patch? Don't we also want to print > > > > > > > > > the namespace, and use [[]] in the output? > > > > > > > > > > > > > > > > Good point, however: while the testcase indeed has an attribute > > > > > > > > in the [[]] form in the typedef, here we're printing its "aka": > > > > > > > > > > > > > > > > warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} > > > > > > > > > > > > > > > > c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. > > > > > > > > I could do that, but then we'd print > > > > > > > > > > > > > > > > aka 'void ([[nocf_check]] *)(void)' > > > > > > > > > > > > > > > > in the above, but that's invalid syntax! > > > > > > > > > > > > > > Indeed, it should be > > > > > > > > > > > > > > void (* [[gnu::nocf_check]])(void) > > > > > > > > > > > > Ok, let me fix that too, then. I've updated the description to show > > > > > > what we print with the patch, and what was printed before. > > > > > > > > > > Oops, apparently I was wrongly assuming that the attribute appertained to > > > > > the pointer. > > > > > > > > So was I :/. > > > > > > > > > Since it actually appertains to the function type, it should > > > > > be > > > > > > > > > > void (*)(void) [[gnu::nocf_check]]. > > > > > > > > > > But for GCC attribute syntax, I think we want to leave the __attribute__ > > > > > where it was before, as in the manual's example > > > > > > > > > > void (__attribute__((noreturn)) ****f) (void); > > > > > > > > Thanks, done here: > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > > > -- >8 -- > > > > When getting the name of an attribute, we ought to use > > > > get_attribute_name, which handles both [[]] and __attribute__(()) > > > > forms. Failure to do so may result in an ICE, like here. > > > > > > > > pp_c_attributes_display wasn't able to print the [[]] form of > > > > attributes, so this patch teaches it to. > > > > > > > > When printing a pointer to function with a standard attribute, the attribute > > > > should be printed after the parameter-list. With this patch we print: > > > > > > > > aka 'void (*)(int) [[gnu::nocf_check]]' > > > > > > > > pp_c_attributes has been unused since its introduction in r56273 so > > > > this patch removes it. > > > > > > > > PR c++/106937 > > > > > > > > gcc/c-family/ChangeLog: > > > > > > > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print > > > > GNU attributes here. > > > > (c_pretty_printer::abstract_declarator): For a pointer to function, > > > > print the standard [[]] attributes here. > > > > (pp_c_attributes): Remove. > > > > (pp_c_attributes_display): Print the [[]] form if appropriate. Use > > > > get_attribute_name. Don't print a trailing space when printing the > > > > [[]] form. > > > > * c-pretty-print.h (pp_c_attributes): Remove. > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > * error.cc: Include "attribs.h". > > > > (dump_type_prefix): Only print GNU attributes here. > > > > (dump_type_suffix): Print standard attributes here. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * c-c++-common/pointer-to-fn1.c: New test. > > > > --- > > > > gcc/c-family/c-pretty-print.cc | 102 +++++++++++--------- > > > > gcc/c-family/c-pretty-print.h | 1 - > > > > gcc/cp/error.cc | 17 +++- > > > > gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ > > > > 4 files changed, 92 insertions(+), 46 deletions(-) > > > > create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c > > > > > > > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > > > > index efa1768f4d6..349f0a07d3e 100644 > > > > --- a/gcc/c-family/c-pretty-print.cc > > > > +++ b/gcc/c-family/c-pretty-print.cc > > > > @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) > > > > { > > > > pp_c_whitespace (pp); > > > > pp_c_left_paren (pp); > > > > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > > > > + /* If we're dealing with the GNU form of attributes, print this: > > > > + void (__attribute__((noreturn)) *f) (); > > > > + If it is the standard [[]] attribute, we'll print the attribute > > > > + in c_pretty_printer::abstract_declarator. */ > > > > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) > > > > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > > > > } > > > > else if (!c_dialect_cxx ()) > > > > pp_c_whitespace (pp); > > > > @@ -564,15 +569,26 @@ pp_c_parameter_type_list (c_pretty_printer *pp, tree t) > > > > void > > > > c_pretty_printer::abstract_declarator (tree t) > > > > { > > > > + bool fn_ptr_p = false; > > > > if (TREE_CODE (t) == POINTER_TYPE) > > > > { > > > > if (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE > > > > || TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) > > > > - pp_c_right_paren (this); > > > > + { > > > > + pp_c_right_paren (this); > > > > + fn_ptr_p = true; > > > > + } > > > > t = TREE_TYPE (t); > > > > } > > > > direct_abstract_declarator (t); > > > > + /* If this is the standard [[]] attribute, print > > > > + void (*)() [[noreturn]]; */ > > > > + if (fn_ptr_p && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > > > > > > Do you need to check fn_ptr_p? The attributes on a function type go here > > > whether or not we were dealing with a pointer. > > > > I probably don't need it! Here's a version without that: > > > > So far all dg.exp tests passed. > > > > -- >8 -- > > When getting the name of an attribute, we ought to use > > get_attribute_name, which handles both [[]] and __attribute__(()) > > forms. Failure to do so may result in an ICE, like here. > > > > pp_c_attributes_display wasn't able to print the [[]] form of > > attributes, so this patch teaches it to. > > > > When printing a pointer to function with a standard attribute, the attribute > > should be printed after the parameter-list. With this patch we print: > > > > aka 'void (*)(int) [[gnu::nocf_check]]' > > > > pp_c_attributes has been unused since its introduction in r56273 so > > this patch removes it. > > > > PR c++/106937 > > > > gcc/c-family/ChangeLog: > > > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print > > GNU attributes here. > > (c_pretty_printer::abstract_declarator): For a pointer to function, > > print the standard [[]] attributes here. > > This change now applies to all types, whereas the dump_type_suffix change > only applies to functions. Maybe move this change into > direct_abstract_declarator? That's better, done. > > (pp_c_attributes): Remove. > > (pp_c_attributes_display): Print the [[]] form if appropriate. Use > > get_attribute_name. Don't print a trailing space when printing the > > [[]] form. > > This is no longer accurate. What specifically? I think it's still accurate. > > * c-pretty-print.h (pp_c_attributes): Remove. > > > > gcc/cp/ChangeLog: > > > > * error.cc: Include "attribs.h". > > (dump_type_prefix): Only print GNU attributes here. > > (dump_type_suffix): Print standard attributes here. > > See below. > > @@ -1027,6 +1033,14 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) > > TREE_CODE (t) == FUNCTION_TYPE > > && (flags & TFF_POINTER)); > > dump_ref_qualifier (pp, t, flags); > > + /* If this is the standard [[]] attribute, print > > + void (*)() [[noreturn]]; */ > > + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > > + { > > + pp_space (pp); > > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); > > + pp->padding = pp_before; > > + } > > This is too soon, attributes come after the exception-specifier. Ah, fixed then. Hopefully this is the correct patch. Thanks. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- When getting the name of an attribute, we ought to use get_attribute_name, which handles both [[]] and __attribute__(()) forms. Failure to do so may result in an ICE, like here. pp_c_attributes_display wasn't able to print the [[]] form of attributes, so this patch teaches it to. When printing a pointer to function with a standard attribute, the attribute should be printed after the parameter-list. With this patch we print: aka 'void (*)(int) [[gnu::nocf_check]]' or, in C++ with noexcept: aka 'void (*)(int) noexcept [[gnu::nocf_check]]' pp_c_attributes has been unused since its introduction in r56273 so this patch removes it. PR c++/106937 gcc/c-family/ChangeLog: * c-pretty-print.cc (pp_c_specifier_qualifier_list): Print only GNU attributes here. (c_pretty_printer::direct_abstract_declarator): Print the standard [[]] attributes here. (pp_c_attributes): Remove. (pp_c_attributes_display): Print the [[]] form if appropriate. Use get_attribute_name. Don't print a trailing space when printing the [[]] form. * c-pretty-print.h (pp_c_attributes): Remove. gcc/cp/ChangeLog: * error.cc: Include "attribs.h". (dump_type_prefix): Print only GNU attributes here. (dump_type_suffix): Print standard attributes here. gcc/testsuite/ChangeLog: * c-c++-common/pointer-to-fn1.c: New test. --- gcc/c-family/c-pretty-print.cc | 96 ++++++++++++--------- gcc/c-family/c-pretty-print.h | 1 - gcc/cp/error.cc | 16 +++- gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ 4 files changed, 86 insertions(+), 45 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc index efa1768f4d6..c99b2ceffe6 100644 --- a/gcc/c-family/c-pretty-print.cc +++ b/gcc/c-family/c-pretty-print.cc @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) { pp_c_whitespace (pp); pp_c_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); + /* If we're dealing with the GNU form of attributes, print this: + void (__attribute__((noreturn)) *f) (); + If it is the standard [[]] attribute, we'll print the attribute + in c_pretty_printer::direct_abstract_declarator/FUNCTION_TYPE. */ + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); } else if (!c_dialect_cxx ()) pp_c_whitespace (pp); @@ -595,6 +600,13 @@ c_pretty_printer::direct_abstract_declarator (tree t) case FUNCTION_TYPE: pp_c_parameter_type_list (this, t); direct_abstract_declarator (TREE_TYPE (t)); + /* If this is the standard [[]] attribute, print + void (*)() [[noreturn]]; */ + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) + { + pp_space (this); + pp_c_attributes_display (this, TYPE_ATTRIBUTES (t)); + } break; case ARRAY_TYPE: @@ -850,32 +862,7 @@ c_pretty_printer::declaration (tree t) pp_c_init_declarator (this, t); } -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ - -void -pp_c_attributes (c_pretty_printer *pp, tree attributes) -{ - if (attributes == NULL_TREE) - return; - - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) - { - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); - if (TREE_VALUE (attributes)) - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); - - if (TREE_CHAIN (attributes)) - pp_separate_with (pp, ','); - } - pp_c_right_paren (pp); - pp_c_right_paren (pp); -} - -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes - marked to be displayed on disgnostic. */ +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ void pp_c_attributes_display (c_pretty_printer *pp, tree a) @@ -885,10 +872,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) if (a == NULL_TREE) return; + const bool std_p = cxx11_attribute_p (a); + for (; a != NULL_TREE; a = TREE_CHAIN (a)) { - const struct attribute_spec *as; - as = lookup_attribute_spec (TREE_PURPOSE (a)); + const struct attribute_spec *as + = lookup_attribute_spec (get_attribute_name (a)); if (!as || as->affects_type_identity == false) continue; if (c_dialect_cxx () @@ -896,26 +885,47 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) /* In C++ transaction_safe is printed at the end of the declarator. */ continue; if (is_first) - { - pp_c_ws_string (pp, "__attribute__"); - pp_c_left_paren (pp); - pp_c_left_paren (pp); - is_first = false; - } + { + if (std_p) + { + pp_c_left_bracket (pp); + pp_c_left_bracket (pp); + } + else + { + pp_c_ws_string (pp, "__attribute__"); + pp_c_left_paren (pp); + pp_c_left_paren (pp); + } + is_first = false; + } else - { - pp_separate_with (pp, ','); - } - pp_tree_identifier (pp, TREE_PURPOSE (a)); + pp_separate_with (pp, ','); + tree ns; + if (std_p && (ns = get_attribute_namespace (a))) + { + pp_tree_identifier (pp, ns); + pp_colon (pp); + pp_colon (pp); + } + pp_tree_identifier (pp, get_attribute_name (a)); if (TREE_VALUE (a)) - pp_c_call_argument_list (pp, TREE_VALUE (a)); + pp_c_call_argument_list (pp, TREE_VALUE (a)); } if (!is_first) { - pp_c_right_paren (pp); - pp_c_right_paren (pp); - pp_c_whitespace (pp); + if (std_p) + { + pp_c_right_bracket (pp); + pp_c_right_bracket (pp); + } + else + { + pp_c_right_paren (pp); + pp_c_right_paren (pp); + pp_c_whitespace (pp); + } } } diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h index be86bed4fee..92674ab4d06 100644 --- a/gcc/c-family/c-pretty-print.h +++ b/gcc/c-family/c-pretty-print.h @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); /* Declarations. */ void pp_c_tree_decl_identifier (c_pretty_printer *, tree); void pp_c_function_definition (c_pretty_printer *, tree); -void pp_c_attributes (c_pretty_printer *, tree); void pp_c_attributes_display (c_pretty_printer *, tree); void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); void pp_c_type_qualifier_list (c_pretty_printer *, tree); diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 4514c8bbb44..da8c95c9b43 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see #include "internal-fn.h" #include "gcc-rich-location.h" #include "cp-name-hint.h" +#include "attribs.h" #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') @@ -897,7 +898,12 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) { pp_cxx_whitespace (pp); pp_cxx_left_paren (pp); - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); + /* If we're dealing with the GNU form of attributes, print this: + void (__attribute__((noreturn)) *f) (); + If it is the standard [[]] attribute, we'll print the attribute + in dump_type_suffix. */ + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (sub))) + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); } if (TYPE_PTR_P (t)) pp_star (pp); @@ -1030,6 +1036,14 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) if (tx_safe_fn_type_p (t)) pp_cxx_ws_string (pp, "transaction_safe"); dump_exception_spec (pp, TYPE_RAISES_EXCEPTIONS (t), flags); + /* If this is the standard [[]] attribute, print + void (*)() [[noreturn]]; */ + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) + { + pp_space (pp); + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); + pp->padding = pp_before; + } dump_type_suffix (pp, TREE_TYPE (t), flags); break; } diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c new file mode 100644 index 00000000000..975885462e9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c @@ -0,0 +1,18 @@ +/* PR c++/106937 */ +/* { dg-options "-fcf-protection" } */ +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ +/* Test printing a pointer to function with attribute. */ + +__attribute__((nocf_check)) typedef void (*FPA1)(); +[[gnu::nocf_check]] typedef void (*FPA2)(int); +typedef void (*FP1)(); +typedef void (*FP2)(int); + +void +g (FP1 f1, FP2 f2) +{ + FPA1 p1 = f1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } + FPA2 p2 = f2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } + FP1 p3 = p1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } + FP2 p4 = p2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } +} base-commit: 248c8aeebc49aae3fd96bd587367d12e7c8b3c3a -- 2.37.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-10 19:18 ` [PATCH v5] " Marek Polacek @ 2022-10-10 19:23 ` Jason Merrill 2022-10-11 7:40 ` Andreas Schwab 1 sibling, 0 replies; 14+ messages in thread From: Jason Merrill @ 2022-10-10 19:23 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Joseph Myers On 10/10/22 15:18, Marek Polacek wrote: > On Mon, Oct 10, 2022 at 10:49:34AM -0400, Jason Merrill wrote: >> On 10/7/22 18:16, Marek Polacek wrote: >>> On Fri, Oct 07, 2022 at 05:56:18PM -0400, Jason Merrill wrote: >>>> On 10/7/22 17:08, Marek Polacek wrote: >>>>> On Fri, Oct 07, 2022 at 12:17:34PM -0400, Jason Merrill wrote: >>>>>> On 10/6/22 22:12, Marek Polacek wrote: >>>>>>> On Thu, Oct 06, 2022 at 05:42:41PM -0400, Jason Merrill wrote: >>>>>>>> On 10/4/22 19:06, Marek Polacek wrote: >>>>>>>>> On Fri, Sep 30, 2022 at 09:12:24AM -0400, Jason Merrill wrote: >>>>>>>>>> On 9/29/22 18:49, Marek Polacek wrote: >>>>>>>>>>> When getting the name of an attribute, we ought to use >>>>>>>>>>> get_attribute_name, which handles both [[ ]] and __attribute__(()) >>>>>>>>>>> forms. Failure to do so may result in an ICE, like here. >>>>>>>>>>> >>>>>>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>>>>>>> >>>>>>>>>> How do we print the attributes with this patch? Don't we also want to print >>>>>>>>>> the namespace, and use [[]] in the output? >>>>>>>>> >>>>>>>>> Good point, however: while the testcase indeed has an attribute >>>>>>>>> in the [[]] form in the typedef, here we're printing its "aka": >>>>>>>>> >>>>>>>>> warning: initialization of 'FuncPointerWithNoCfCheck' {aka 'void (__attribute__((nocf_check)) *)(void)'} from incompatible pointer type 'FuncPointer' {aka 'void (*)(void)'} >>>>>>>>> >>>>>>>>> c-pretty-print.cc doesn't seem to know how to print an [[]] attribute. >>>>>>>>> I could do that, but then we'd print >>>>>>>>> >>>>>>>>> aka 'void ([[nocf_check]] *)(void)' >>>>>>>>> >>>>>>>>> in the above, but that's invalid syntax! >>>>>>>> >>>>>>>> Indeed, it should be >>>>>>>> >>>>>>>> void (* [[gnu::nocf_check]])(void) >>>>>>> >>>>>>> Ok, let me fix that too, then. I've updated the description to show >>>>>>> what we print with the patch, and what was printed before. >>>>>> >>>>>> Oops, apparently I was wrongly assuming that the attribute appertained to >>>>>> the pointer. >>>>> >>>>> So was I :/. >>>>> >>>>>> Since it actually appertains to the function type, it should >>>>>> be >>>>>> >>>>>> void (*)(void) [[gnu::nocf_check]]. >>>>>> >>>>>> But for GCC attribute syntax, I think we want to leave the __attribute__ >>>>>> where it was before, as in the manual's example >>>>>> >>>>>> void (__attribute__((noreturn)) ****f) (void); >>>>> >>>>> Thanks, done here: >>>>> >>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>> >>>>> -- >8 -- >>>>> When getting the name of an attribute, we ought to use >>>>> get_attribute_name, which handles both [[]] and __attribute__(()) >>>>> forms. Failure to do so may result in an ICE, like here. >>>>> >>>>> pp_c_attributes_display wasn't able to print the [[]] form of >>>>> attributes, so this patch teaches it to. >>>>> >>>>> When printing a pointer to function with a standard attribute, the attribute >>>>> should be printed after the parameter-list. With this patch we print: >>>>> >>>>> aka 'void (*)(int) [[gnu::nocf_check]]' >>>>> >>>>> pp_c_attributes has been unused since its introduction in r56273 so >>>>> this patch removes it. >>>>> >>>>> PR c++/106937 >>>>> >>>>> gcc/c-family/ChangeLog: >>>>> >>>>> * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print >>>>> GNU attributes here. >>>>> (c_pretty_printer::abstract_declarator): For a pointer to function, >>>>> print the standard [[]] attributes here. >>>>> (pp_c_attributes): Remove. >>>>> (pp_c_attributes_display): Print the [[]] form if appropriate. Use >>>>> get_attribute_name. Don't print a trailing space when printing the >>>>> [[]] form. >>>>> * c-pretty-print.h (pp_c_attributes): Remove. >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * error.cc: Include "attribs.h". >>>>> (dump_type_prefix): Only print GNU attributes here. >>>>> (dump_type_suffix): Print standard attributes here. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * c-c++-common/pointer-to-fn1.c: New test. >>>>> --- >>>>> gcc/c-family/c-pretty-print.cc | 102 +++++++++++--------- >>>>> gcc/c-family/c-pretty-print.h | 1 - >>>>> gcc/cp/error.cc | 17 +++- >>>>> gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ >>>>> 4 files changed, 92 insertions(+), 46 deletions(-) >>>>> create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c >>>>> >>>>> diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc >>>>> index efa1768f4d6..349f0a07d3e 100644 >>>>> --- a/gcc/c-family/c-pretty-print.cc >>>>> +++ b/gcc/c-family/c-pretty-print.cc >>>>> @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) >>>>> { >>>>> pp_c_whitespace (pp); >>>>> pp_c_left_paren (pp); >>>>> - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); >>>>> + /* If we're dealing with the GNU form of attributes, print this: >>>>> + void (__attribute__((noreturn)) *f) (); >>>>> + If it is the standard [[]] attribute, we'll print the attribute >>>>> + in c_pretty_printer::abstract_declarator. */ >>>>> + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) >>>>> + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); >>>>> } >>>>> else if (!c_dialect_cxx ()) >>>>> pp_c_whitespace (pp); >>>>> @@ -564,15 +569,26 @@ pp_c_parameter_type_list (c_pretty_printer *pp, tree t) >>>>> void >>>>> c_pretty_printer::abstract_declarator (tree t) >>>>> { >>>>> + bool fn_ptr_p = false; >>>>> if (TREE_CODE (t) == POINTER_TYPE) >>>>> { >>>>> if (TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE >>>>> || TREE_CODE (TREE_TYPE (t)) == FUNCTION_TYPE) >>>>> - pp_c_right_paren (this); >>>>> + { >>>>> + pp_c_right_paren (this); >>>>> + fn_ptr_p = true; >>>>> + } >>>>> t = TREE_TYPE (t); >>>>> } >>>>> direct_abstract_declarator (t); >>>>> + /* If this is the standard [[]] attribute, print >>>>> + void (*)() [[noreturn]]; */ >>>>> + if (fn_ptr_p && cxx11_attribute_p (TYPE_ATTRIBUTES (t))) >>>> >>>> Do you need to check fn_ptr_p? The attributes on a function type go here >>>> whether or not we were dealing with a pointer. >>> >>> I probably don't need it! Here's a version without that: >>> >>> So far all dg.exp tests passed. >>> >>> -- >8 -- >>> When getting the name of an attribute, we ought to use >>> get_attribute_name, which handles both [[]] and __attribute__(()) >>> forms. Failure to do so may result in an ICE, like here. >>> >>> pp_c_attributes_display wasn't able to print the [[]] form of >>> attributes, so this patch teaches it to. >>> >>> When printing a pointer to function with a standard attribute, the attribute >>> should be printed after the parameter-list. With this patch we print: >>> >>> aka 'void (*)(int) [[gnu::nocf_check]]' >>> >>> pp_c_attributes has been unused since its introduction in r56273 so >>> this patch removes it. >>> >>> PR c++/106937 >>> >>> gcc/c-family/ChangeLog: >>> >>> * c-pretty-print.cc (pp_c_specifier_qualifier_list): Only print >>> GNU attributes here. >>> (c_pretty_printer::abstract_declarator): For a pointer to function, >>> print the standard [[]] attributes here. >> >> This change now applies to all types, whereas the dump_type_suffix change >> only applies to functions. Maybe move this change into >> direct_abstract_declarator? > > That's better, done. > >>> (pp_c_attributes): Remove. >>> (pp_c_attributes_display): Print the [[]] form if appropriate. Use >>> get_attribute_name. Don't print a trailing space when printing the >>> [[]] form. >> >> This is no longer accurate. > > What specifically? I think it's still accurate. You're right, I was confused. >>> * c-pretty-print.h (pp_c_attributes): Remove. >>> >>> gcc/cp/ChangeLog: >>> >>> * error.cc: Include "attribs.h". >>> (dump_type_prefix): Only print GNU attributes here. >>> (dump_type_suffix): Print standard attributes here. >> >> See below. > >>> @@ -1027,6 +1033,14 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) >>> TREE_CODE (t) == FUNCTION_TYPE >>> && (flags & TFF_POINTER)); >>> dump_ref_qualifier (pp, t, flags); >>> + /* If this is the standard [[]] attribute, print >>> + void (*)() [[noreturn]]; */ >>> + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) >>> + { >>> + pp_space (pp); >>> + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); >>> + pp->padding = pp_before; >>> + } >> >> This is too soon, attributes come after the exception-specifier. > > Ah, fixed then. > > Hopefully this is the correct patch. Thanks. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK, thanks. > -- >8 -- > When getting the name of an attribute, we ought to use > get_attribute_name, which handles both [[]] and __attribute__(()) > forms. Failure to do so may result in an ICE, like here. > > pp_c_attributes_display wasn't able to print the [[]] form of > attributes, so this patch teaches it to. > > When printing a pointer to function with a standard attribute, the attribute > should be printed after the parameter-list. With this patch we print: > > aka 'void (*)(int) [[gnu::nocf_check]]' > > or, in C++ with noexcept: > > aka 'void (*)(int) noexcept [[gnu::nocf_check]]' > > pp_c_attributes has been unused since its introduction in r56273 so > this patch removes it. > > PR c++/106937 > > gcc/c-family/ChangeLog: > > * c-pretty-print.cc (pp_c_specifier_qualifier_list): Print only GNU > attributes here. > (c_pretty_printer::direct_abstract_declarator): Print the standard [[]] > attributes here. > (pp_c_attributes): Remove. > (pp_c_attributes_display): Print the [[]] form if appropriate. Use > get_attribute_name. Don't print a trailing space when printing the > [[]] form. > * c-pretty-print.h (pp_c_attributes): Remove. > > gcc/cp/ChangeLog: > > * error.cc: Include "attribs.h". > (dump_type_prefix): Print only GNU attributes here. > (dump_type_suffix): Print standard attributes here. > > gcc/testsuite/ChangeLog: > > * c-c++-common/pointer-to-fn1.c: New test. > --- > gcc/c-family/c-pretty-print.cc | 96 ++++++++++++--------- > gcc/c-family/c-pretty-print.h | 1 - > gcc/cp/error.cc | 16 +++- > gcc/testsuite/c-c++-common/pointer-to-fn1.c | 18 ++++ > 4 files changed, 86 insertions(+), 45 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/pointer-to-fn1.c > > diff --git a/gcc/c-family/c-pretty-print.cc b/gcc/c-family/c-pretty-print.cc > index efa1768f4d6..c99b2ceffe6 100644 > --- a/gcc/c-family/c-pretty-print.cc > +++ b/gcc/c-family/c-pretty-print.cc > @@ -466,7 +466,12 @@ pp_c_specifier_qualifier_list (c_pretty_printer *pp, tree t) > { > pp_c_whitespace (pp); > pp_c_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > + /* If we're dealing with the GNU form of attributes, print this: > + void (__attribute__((noreturn)) *f) (); > + If it is the standard [[]] attribute, we'll print the attribute > + in c_pretty_printer::direct_abstract_declarator/FUNCTION_TYPE. */ > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (pointee))) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (pointee)); > } > else if (!c_dialect_cxx ()) > pp_c_whitespace (pp); > @@ -595,6 +600,13 @@ c_pretty_printer::direct_abstract_declarator (tree t) > case FUNCTION_TYPE: > pp_c_parameter_type_list (this, t); > direct_abstract_declarator (TREE_TYPE (t)); > + /* If this is the standard [[]] attribute, print > + void (*)() [[noreturn]]; */ > + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > + { > + pp_space (this); > + pp_c_attributes_display (this, TYPE_ATTRIBUTES (t)); > + } > break; > > case ARRAY_TYPE: > @@ -850,32 +862,7 @@ c_pretty_printer::declaration (tree t) > pp_c_init_declarator (this, t); > } > > -/* Pretty-print ATTRIBUTES using GNU C extension syntax. */ > - > -void > -pp_c_attributes (c_pretty_printer *pp, tree attributes) > -{ > - if (attributes == NULL_TREE) > - return; > - > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - for (; attributes != NULL_TREE; attributes = TREE_CHAIN (attributes)) > - { > - pp_tree_identifier (pp, TREE_PURPOSE (attributes)); > - if (TREE_VALUE (attributes)) > - pp_c_call_argument_list (pp, TREE_VALUE (attributes)); > - > - if (TREE_CHAIN (attributes)) > - pp_separate_with (pp, ','); > - } > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > -} > - > -/* Pretty-print ATTRIBUTES using GNU C extension syntax for attributes > - marked to be displayed on disgnostic. */ > +/* Pretty-print ATTRIBUTES marked to be displayed on diagnostic. */ > > void > pp_c_attributes_display (c_pretty_printer *pp, tree a) > @@ -885,10 +872,12 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > if (a == NULL_TREE) > return; > > + const bool std_p = cxx11_attribute_p (a); > + > for (; a != NULL_TREE; a = TREE_CHAIN (a)) > { > - const struct attribute_spec *as; > - as = lookup_attribute_spec (TREE_PURPOSE (a)); > + const struct attribute_spec *as > + = lookup_attribute_spec (get_attribute_name (a)); > if (!as || as->affects_type_identity == false) > continue; > if (c_dialect_cxx () > @@ -896,26 +885,47 @@ pp_c_attributes_display (c_pretty_printer *pp, tree a) > /* In C++ transaction_safe is printed at the end of the declarator. */ > continue; > if (is_first) > - { > - pp_c_ws_string (pp, "__attribute__"); > - pp_c_left_paren (pp); > - pp_c_left_paren (pp); > - is_first = false; > - } > + { > + if (std_p) > + { > + pp_c_left_bracket (pp); > + pp_c_left_bracket (pp); > + } > + else > + { > + pp_c_ws_string (pp, "__attribute__"); > + pp_c_left_paren (pp); > + pp_c_left_paren (pp); > + } > + is_first = false; > + } > else > - { > - pp_separate_with (pp, ','); > - } > - pp_tree_identifier (pp, TREE_PURPOSE (a)); > + pp_separate_with (pp, ','); > + tree ns; > + if (std_p && (ns = get_attribute_namespace (a))) > + { > + pp_tree_identifier (pp, ns); > + pp_colon (pp); > + pp_colon (pp); > + } > + pp_tree_identifier (pp, get_attribute_name (a)); > if (TREE_VALUE (a)) > - pp_c_call_argument_list (pp, TREE_VALUE (a)); > + pp_c_call_argument_list (pp, TREE_VALUE (a)); > } > > if (!is_first) > { > - pp_c_right_paren (pp); > - pp_c_right_paren (pp); > - pp_c_whitespace (pp); > + if (std_p) > + { > + pp_c_right_bracket (pp); > + pp_c_right_bracket (pp); > + } > + else > + { > + pp_c_right_paren (pp); > + pp_c_right_paren (pp); > + pp_c_whitespace (pp); > + } > } > } > > diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h > index be86bed4fee..92674ab4d06 100644 > --- a/gcc/c-family/c-pretty-print.h > +++ b/gcc/c-family/c-pretty-print.h > @@ -119,7 +119,6 @@ void pp_c_space_for_pointer_operator (c_pretty_printer *, tree); > /* Declarations. */ > void pp_c_tree_decl_identifier (c_pretty_printer *, tree); > void pp_c_function_definition (c_pretty_printer *, tree); > -void pp_c_attributes (c_pretty_printer *, tree); > void pp_c_attributes_display (c_pretty_printer *, tree); > void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type); > void pp_c_type_qualifier_list (c_pretty_printer *, tree); > diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc > index 4514c8bbb44..da8c95c9b43 100644 > --- a/gcc/cp/error.cc > +++ b/gcc/cp/error.cc > @@ -36,6 +36,7 @@ along with GCC; see the file COPYING3. If not see > #include "internal-fn.h" > #include "gcc-rich-location.h" > #include "cp-name-hint.h" > +#include "attribs.h" > > #define pp_separate_with_comma(PP) pp_cxx_separate_with (PP, ',') > #define pp_separate_with_semicolon(PP) pp_cxx_separate_with (PP, ';') > @@ -897,7 +898,12 @@ dump_type_prefix (cxx_pretty_printer *pp, tree t, int flags) > { > pp_cxx_whitespace (pp); > pp_cxx_left_paren (pp); > - pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > + /* If we're dealing with the GNU form of attributes, print this: > + void (__attribute__((noreturn)) *f) (); > + If it is the standard [[]] attribute, we'll print the attribute > + in dump_type_suffix. */ > + if (!cxx11_attribute_p (TYPE_ATTRIBUTES (sub))) > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (sub)); > } > if (TYPE_PTR_P (t)) > pp_star (pp); > @@ -1030,6 +1036,14 @@ dump_type_suffix (cxx_pretty_printer *pp, tree t, int flags) > if (tx_safe_fn_type_p (t)) > pp_cxx_ws_string (pp, "transaction_safe"); > dump_exception_spec (pp, TYPE_RAISES_EXCEPTIONS (t), flags); > + /* If this is the standard [[]] attribute, print > + void (*)() [[noreturn]]; */ > + if (cxx11_attribute_p (TYPE_ATTRIBUTES (t))) > + { > + pp_space (pp); > + pp_c_attributes_display (pp, TYPE_ATTRIBUTES (t)); > + pp->padding = pp_before; > + } > dump_type_suffix (pp, TREE_TYPE (t), flags); > break; > } > diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > new file mode 100644 > index 00000000000..975885462e9 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > @@ -0,0 +1,18 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection" } */ > +/* { dg-additional-options "-std=c++11 -fpermissive" { target c++ } } */ > +/* Test printing a pointer to function with attribute. */ > + > +__attribute__((nocf_check)) typedef void (*FPA1)(); > +[[gnu::nocf_check]] typedef void (*FPA2)(int); > +typedef void (*FP1)(); > +typedef void (*FP2)(int); > + > +void > +g (FP1 f1, FP2 f2) > +{ > + FPA1 p1 = f1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } > + FPA2 p2 = f2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } > + FP1 p3 = p1; // { dg-warning {aka 'void \(__attribute__\(\(nocf_check\)\) \*\)\(\)'} } > + FP2 p4 = p2; // { dg-warning {aka 'void \(\*\)\(int\) \[\[gnu::nocf_check\]\]'} } > +} > > base-commit: 248c8aeebc49aae3fd96bd587367d12e7c8b3c3a ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-10 19:18 ` [PATCH v5] " Marek Polacek 2022-10-10 19:23 ` Jason Merrill @ 2022-10-11 7:40 ` Andreas Schwab 2022-10-11 20:03 ` Marek Polacek 1 sibling, 1 reply; 14+ messages in thread From: Andreas Schwab @ 2022-10-11 7:40 UTC (permalink / raw) To: Marek Polacek via Gcc-patches; +Cc: Jason Merrill, Marek Polacek, Joseph Myers On Okt 10 2022, Marek Polacek via Gcc-patches wrote: > diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > new file mode 100644 > index 00000000000..975885462e9 > --- /dev/null > +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > @@ -0,0 +1,18 @@ > +/* PR c++/106937 */ > +/* { dg-options "-fcf-protection" } */ FAIL: c-c++-common/pointer-to-fn1.c -Wc++-compat (test for excess errors) Excess errors: cc1: error: '-fcf-protection=full' is not supported for this target -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] c-family: ICE with [[gnu::nocf_check]] [PR106937] 2022-10-11 7:40 ` Andreas Schwab @ 2022-10-11 20:03 ` Marek Polacek 0 siblings, 0 replies; 14+ messages in thread From: Marek Polacek @ 2022-10-11 20:03 UTC (permalink / raw) To: Andreas Schwab; +Cc: Marek Polacek via Gcc-patches On Tue, Oct 11, 2022 at 09:40:45AM +0200, Andreas Schwab via Gcc-patches wrote: > On Okt 10 2022, Marek Polacek via Gcc-patches wrote: > > > diff --git a/gcc/testsuite/c-c++-common/pointer-to-fn1.c b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > > new file mode 100644 > > index 00000000000..975885462e9 > > --- /dev/null > > +++ b/gcc/testsuite/c-c++-common/pointer-to-fn1.c > > @@ -0,0 +1,18 @@ > > +/* PR c++/106937 */ > > +/* { dg-options "-fcf-protection" } */ > > FAIL: c-c++-common/pointer-to-fn1.c -Wc++-compat (test for excess errors) > Excess errors: > cc1: error: '-fcf-protection=full' is not supported for this target Thanks, patch posted. Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-10-11 20:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-29 22:49 [PATCH] c-family: ICE with [[gnu::nocf_check]] [PR106937] Marek Polacek 2022-09-30 13:12 ` Jason Merrill 2022-10-04 23:06 ` [PATCH v2] " Marek Polacek 2022-10-06 21:42 ` Jason Merrill 2022-10-07 2:12 ` [PATCH v3] " Marek Polacek 2022-10-07 16:17 ` Jason Merrill 2022-10-07 21:08 ` [PATCH v4] " Marek Polacek 2022-10-07 21:56 ` Jason Merrill 2022-10-07 22:16 ` Marek Polacek 2022-10-10 14:49 ` Jason Merrill 2022-10-10 19:18 ` [PATCH v5] " Marek Polacek 2022-10-10 19:23 ` Jason Merrill 2022-10-11 7:40 ` Andreas Schwab 2022-10-11 20:03 ` Marek Polacek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).