Re: [PATCH v4] c-family: ICE with [[gnu::nocf_check]] [PR106937]

[Jason Merrill <jason@redhat.com>]

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