Re: [PATCH] PR c/86407 - Add option to ignore fndecl attributes on function pointers

[Martin Sebor <msebor@gmail.com>]

On 5/23/19 9:57 PM, Alex Henrie wrote:
> ---
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86407#c6
> ---
>   gcc/c-family/c.opt                     |  4 ++++
>   gcc/c/c-decl.c                         |  4 +++-
>   gcc/config/i386/i386-options.c         | 12 ++++++++++--
>   gcc/testsuite/c-c++-common/pr86407-1.c | 23 +++++++++++++++++++++++
>   gcc/testsuite/c-c++-common/pr86407-2.c | 25 +++++++++++++++++++++++++
>   5 files changed, 65 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/c-c++-common/pr86407-1.c
>   create mode 100644 gcc/testsuite/c-c++-common/pr86407-2.c
> 
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 046d489f7eb..3b99c96e53d 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -776,6 +776,10 @@ Wsizeof-array-argument
>   C ObjC C++ ObjC++ Var(warn_sizeof_array_argument) Warning Init(1)
>   Warn when sizeof is applied on a parameter declared as an array.
>   
> +Wstrict-function-attributes
> +C C++ Var(warn_strict_function_attributes) Init(1) Warning
> +Warn when function definition attributes are applied to function pointers.

I thought I'd added my comments about the patch to the bug but
I guess I never saved it.

My first question is: what is the meaning of "function definition
attributes?"  Presumably those that affect only the definition of
a function and not its callers or other users?

I don't think GCC makes a formal distinction between function
attributes that affect only function definitions vs those that
affect its users, or both.  It might be a useful distinction
to introduce, perhaps even for functions as well as variables,
but as it is, users (as well as GCC developers) are on our own
to figure it out.

If such a distinction were to be introduced I think we then would
need to go through all attributes and decide which is which, make
sure it makes sense, and document it for each.  This might be
a useful exercise for other reasons as well: we should also
document whether an attribute attaches to the declaration or to
its type (this determines whether a pointer to a function can be
declared to have the same attribute as the function itself).

Finally, with this as a prerequisite, if we decided that a warning
like this would be useful, tests to verify that it works for all
the definition attributes and not for the rest would need to be
added (i.e., in addition to ms_hook_prologue).

Martin

> +
>   Wstringop-overflow
>   C ObjC C++ LTO ObjC++ Warning Alias(Wstringop-overflow=, 2, 0)
>   Warn about buffer overflow in string manipulation functions like memcpy
> diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
> index 181a8c2e9aa..427d573c8d3 100644
> --- a/gcc/c/c-decl.c
> +++ b/gcc/c/c-decl.c
> @@ -6192,7 +6192,9 @@ grokdeclarator (const struct c_declarator *declarator,
>   	      inner_decl = inner_decl->declarator;
>   	    if (inner_decl->kind == cdk_id)
>   	      attr_flags |= (int) ATTR_FLAG_DECL_NEXT;
> -	    else if (inner_decl->kind == cdk_function)
> +	    else if (inner_decl->kind == cdk_function
> +		     || (inner_decl->kind == cdk_pointer
> +			 && TREE_CODE (type) == FUNCTION_TYPE))
>   	      attr_flags |= (int) ATTR_FLAG_FUNCTION_NEXT;
>   	    else if (inner_decl->kind == cdk_array)
>   	      attr_flags |= (int) ATTR_FLAG_ARRAY_NEXT;
> diff --git a/gcc/config/i386/i386-options.c b/gcc/config/i386/i386-options.c
> index 0f236626005..6b50f143b05 100644
> --- a/gcc/config/i386/i386-options.c
> +++ b/gcc/config/i386/i386-options.c
> @@ -3489,8 +3489,16 @@ ix86_handle_fndecl_attribute (tree *node, tree name, tree args, int,
>   {
>     if (TREE_CODE (*node) != FUNCTION_DECL)
>       {
> -      warning (OPT_Wattributes, "%qE attribute only applies to functions",
> -               name);
> +      if (FUNCTION_POINTER_TYPE_P (TREE_TYPE (*node)))
> +	{
> +	  warning (OPT_Wstrict_function_attributes,
> +		   "%qE attribute only applies to function definitions", name);
> +	}
> +      else
> +	{
> +	  warning (OPT_Wattributes,
> +		   "%qE attribute only applies to functions", name);
> +	}
>         *no_add_attrs = true;
>       }
>   
> diff --git a/gcc/testsuite/c-c++-common/pr86407-1.c b/gcc/testsuite/c-c++-common/pr86407-1.c
> new file mode 100644
> index 00000000000..37993d8c051
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/pr86407-1.c
> @@ -0,0 +1,23 @@
> +/* PR c/86407 */
> +/* Test a function attribute that is not a calling convention and does not
> +   affect the function type. There should be no warnings when using the
> +   attribute on a function declaration, function pointer, or function pointer
> +   typedef, but there should be a warning when using the attribute on
> +   non-function-pointer variables and typedefs. */
> +
> +/* { dg-options "-Wstrict-function-attributes -Wno-ignored-attributes" } */
> +
> +int __attribute__((__ms_hook_prologue__)) func(); /* no warnings */
> +
> +int (__attribute__((__ms_hook_prologue__)) *func_ptr)(); /* { dg-warning "'ms_hook_prologue' attribute only applies to function definitions" } */
> +
> +typedef int (__attribute__((__ms_hook_prologue__)) *FUNC_PTR)(); /* { dg-warning "'ms_hook_prologue' attribute only applies to function definitions" } */
> +
> +int __attribute__((__ms_hook_prologue__)) *var_ptr; /* { dg-warning "'ms_hook_prologue' attribute only applies to functions" } */
> +
> +typedef int __attribute__((__ms_hook_prologue__)) *VAR_PTR; /* { dg-warning "'ms_hook_prologue' attribute only applies to functions" } */
> +
> +int main(int argc, char **argv)
> +{
> +    return 0;
> +}
> diff --git a/gcc/testsuite/c-c++-common/pr86407-2.c b/gcc/testsuite/c-c++-common/pr86407-2.c
> new file mode 100644
> index 00000000000..6840b0180dd
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/pr86407-2.c
> @@ -0,0 +1,25 @@
> +/* PR c/86407 */
> +/* Test a function attribute that is not a calling convention and does not
> +   affect the function type. A function pointer without the attribute may point
> +   to a function with the attribute and vice-versa. */
> +
> +/* { dg-options "-Wno-strict-function-attributes -Wno-ignored-attributes" } */
> +
> +int ordinary_func()
> +{
> +    return 0;
> +}
> +
> +int __attribute__((__ms_hook_prologue__)) special_func()
> +{
> +    return 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    int (*ordinary_func_ptr)() = special_func;
> +
> +    int (__attribute__((__ms_hook_prologue__)) *special_func_ptr)() = ordinary_func;
> +
> +    return 0;
> +}
>