public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [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).