| Message ID | 20190524035737.14107-1-alexhenrie24@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR c/86407 - Add option to ignore fndecl attributes on function pointers | expand |
On Thu, 23 May 2019, 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. > + > 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); > + } I'm not sure we really need a new warning for this. > *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 */ > + But this is a declaration? > +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; > +} >
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; > +} >
On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: > > I'm not sure we really need a new warning for this. On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > > 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. Then is it preferable to simply silence Wattributes in this case? On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: > > > +int __attribute__((__ms_hook_prologue__)) func(); /* no warnings */ > > + > > But this is a declaration? On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > > 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? As far as I can tell, "fndecl" is a misnomer: these attributes are more accurately called "function definition attributes", i.e. attributes that affect the assembly code of the function but do not affect its calling convention. On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > > 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). Okay, I will add tests for the other function attributes that should behave in the same way, commenting out the tests that will require more work to pass. The end goal is to include __ms_hook_prologue__ in the WINAPI macro on Wine without causing spurious warnings. This will go a long way towards making Wine compatible with current and future Windows programs. Thank you for help. -Alex
On May 24, 2019 5:49:38 PM GMT+02:00, Alex Henrie <alexhenrie24@gmail.com> wrote: >On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> >wrote: >> >> I'm not sure we really need a new warning for this. > >On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: >> >> 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. > >Then is it preferable to simply silence Wattributes in this case? > >On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> >wrote: >> >> > +int __attribute__((__ms_hook_prologue__)) func(); /* no warnings >*/ >> > + >> >> But this is a declaration? > >On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: >> >> 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? > >As far as I can tell, "fndecl" is a misnomer: these attributes are >more accurately called "function definition attributes", i.e. >attributes that affect the assembly code of the function but do not >affect its calling convention. Yes. The others are attributes also applicable to function types to properly support attributed indirect calls. >On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: >> >> 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). > >Okay, I will add tests for the other function attributes that should >behave in the same way, commenting out the tests that will require >more work to pass. > >The end goal is to include __ms_hook_prologue__ in the WINAPI macro on >Wine without causing spurious warnings. This will go a long way >towards making Wine compatible with current and future Windows >programs. Thank you for help. > >-Alex
On Sat, May 25, 2019 at 12:34 AM Richard Biener <rguenther@suse.de> wrote: > > On May 24, 2019 5:49:38 PM GMT+02:00, Alex Henrie <alexhenrie24@gmail.com> wrote: > >As far as I can tell, "fndecl" is a misnomer: these attributes are > >more accurately called "function definition attributes", i.e. > >attributes that affect the assembly code of the function but do not > >affect its calling convention. > > Yes. The others are attributes also applicable to function types to properly support attributed indirect calls. I'm happy to rename ix86_handle_fndecl_attribute to ix86_handle_fndef_attribute then. Should I do that in this patch or a separate patch? -Alex
On Sat, 25 May 2019, Alex Henrie wrote: > On Sat, May 25, 2019 at 12:34 AM Richard Biener <rguenther@suse.de> wrote: > > > > On May 24, 2019 5:49:38 PM GMT+02:00, Alex Henrie <alexhenrie24@gmail.com> wrote: > > >As far as I can tell, "fndecl" is a misnomer: these attributes are > > >more accurately called "function definition attributes", i.e. > > >attributes that affect the assembly code of the function but do not > > >affect its calling convention. > > > > Yes. The others are attributes also applicable to function types to properly support attributed indirect calls. > > I'm happy to rename ix86_handle_fndecl_attribute to > ix86_handle_fndef_attribute then. Should I do that in this patch or a > separate patch? I don't see this fixes anything. It's pretty well-know that attributes affecting call semantics have to be accepted both at type and decl position and that they should be added to the type attributes. Attributes that only affect code generation for the function itself should be added to decl attributes and do not necessarily need to be accepted in type position.
On 5/24/19 9:49 AM, Alex Henrie wrote: > On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: >> >> I'm not sure we really need a new warning for this. > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: >> >> 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. > > Then is it preferable to simply silence Wattributes in this case? This case being PR86407? I'd say yes. I think one general problem to solve is the missing suppression for typedefs. This could be useful for other warnings as well. Another is providing a knob to control the warning when one kind of an attribute is used with an entity of a different kind (function vs type, or variable vs type). Yet another might be to control warnings for individual attributes. > > On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: >> >>> +int __attribute__((__ms_hook_prologue__)) func(); /* no warnings */ >>> + >> >> But this is a declaration? > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: >> >> 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? > > As far as I can tell, "fndecl" is a misnomer: these attributes are > more accurately called "function definition attributes", i.e. > attributes that affect the assembly code of the function but do not > affect its calling convention. That's one possible definition but there are examples that don't fit it (at least not very neatly). Attribute malloc attaches only to fndecl and not its type but doesn't affect the code for a function definition. FWIW, I think this is just a bug -- attribute malloc should apply to a function type for the same reason the closely related attribute alloc_size does. Attribute constructor also attaches to a fndecl even though it doesn't affect the function's codegen but that of its caller (the runtime). In this case, though, I'd say that's fine. Should it be classified as a function definition attribute? Attributes cold and hot also apply to a fndecl and not its type but they affect both the function's definition and its callers. I think this also makes sense. Should they be classified as function definition attributes? > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: >> >> 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). > > Okay, I will add tests for the other function attributes that should > behave in the same way, commenting out the tests that will require > more work to pass. > > The end goal is to include __ms_hook_prologue__ in the WINAPI macro on > Wine without causing spurious warnings. This will go a long way > towards making Wine compatible with current and future Windows > programs. Thank you for help. Yes, I understand the goal. I'm not sure that the proposed change is the right way to do it. It seems to me that a more targeted bug to fix with the broadest benefit is that _Pragma GCC diagnostic (or some such mechanism) cannot be used to suppress the warning for typedefs. Martin
On Tue, 28 May 2019, Martin Sebor wrote: > On 5/24/19 9:49 AM, Alex Henrie wrote: > > On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: > > > > > > I'm not sure we really need a new warning for this. > > > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > > > > > > 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. > > > > Then is it preferable to simply silence Wattributes in this case? > > This case being PR86407? I'd say yes. I think one general problem > to solve is the missing suppression for typedefs. This could be > useful for other warnings as well. Another is providing a knob to > control the warning when one kind of an attribute is used with > an entity of a different kind (function vs type, or variable vs > type). Yet another might be to control warnings for individual > attributes. > > > > > On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: > > > > > > > +int __attribute__((__ms_hook_prologue__)) func(); /* no warnings */ > > > > + > > > > > > But this is a declaration? > > > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > > > > > > 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? > > > > As far as I can tell, "fndecl" is a misnomer: these attributes are > > more accurately called "function definition attributes", i.e. > > attributes that affect the assembly code of the function but do not > > affect its calling convention. > > That's one possible definition but there are examples that don't > fit it (at least not very neatly). > > Attribute malloc attaches only to fndecl and not its type but > doesn't affect the code for a function definition. FWIW, I > think this is just a bug -- attribute malloc should apply > to a function type for the same reason the closely related > attribute alloc_size does. Yeah, we have existing attributes that do not apply well to indirect calls due to mistakes made in the past. Of course for things like 'malloc' this isn't a correctness issue. > Attribute constructor also attaches to a fndecl even though it > doesn't affect the function's codegen but that of its caller > (the runtime). In this case, though, I'd say that's fine. > Should it be classified as a function definition attribute? I think the issue is that internally we know what a fndecl is but that doesn't match what the C standard says is a function declaration. So it's important to not mix terms for GCC internals and terms used in diagnostics which generally should use terms from the language standard. > Attributes cold and hot also apply to a fndecl and not its type > but they affect both the function's definition and its callers. > I think this also makes sense. Should they be classified as > function definition attributes? > > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > > > > > > 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). > > > > Okay, I will add tests for the other function attributes that should > > behave in the same way, commenting out the tests that will require > > more work to pass. > > > > The end goal is to include __ms_hook_prologue__ in the WINAPI macro on > > Wine without causing spurious warnings. This will go a long way > > towards making Wine compatible with current and future Windows > > programs. Thank you for help. > > Yes, I understand the goal. I'm not sure that the proposed change > is the right way to do it. It seems to me that a more targeted bug > to fix with the broadest benefit is that _Pragma GCC diagnostic (or > some such mechanism) cannot be used to suppress the warning for > typedefs. > > Martin >
On Tue, May 28, 2019 at 1:05 PM Martin Sebor <msebor@gmail.com> wrote: > > On 5/24/19 9:49 AM, Alex Henrie wrote: > > Then is it preferable to simply silence Wattributes in this case? > > This case being PR86407? I'd say yes. > Attribute malloc attaches only to fndecl and not its type but > doesn't affect the code for a function definition. FWIW, I > think this is just a bug -- attribute malloc should apply > to a function type for the same reason the closely related > attribute alloc_size does. > > Attribute constructor also attaches to a fndecl even though it > doesn't affect the function's codegen but that of its caller > (the runtime). In this case, though, I'd say that's fine. > Should it be classified as a function definition attribute? > > Attributes cold and hot also apply to a fndecl and not its type > but they affect both the function's definition and its callers. > I think this also makes sense. Should they be classified as > function definition attributes? Those are very good points. The more information the optimizer has about how the function works, the better it can optimize the code near the function call. This would even apply to ms_hook_prologue because the optimizer should definitely not inline indirect calls to hookable functions even if it would inline a similar non-hookable function. At this point I think I'm convinced that any attribute that applies to a function should also be allowed on a function pointer without any warnings. We can start by turning off the warnings for the "fndecl" attributes and then clean up the other attributes as time goes on. Sound good? -Alex On Tue, May 28, 2019 at 1:05 PM Martin Sebor <msebor@gmail.com> wrote: > > On 5/24/19 9:49 AM, Alex Henrie wrote: > > On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: > >> > >> I'm not sure we really need a new warning for this. > > > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > >> > >> 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. > > > > Then is it preferable to simply silence Wattributes in this case? > > This case being PR86407? I'd say yes. I think one general problem > to solve is the missing suppression for typedefs. This could be > useful for other warnings as well. Another is providing a knob to > control the warning when one kind of an attribute is used with > an entity of a different kind (function vs type, or variable vs > type). Yet another might be to control warnings for individual > attributes. > > > > > On Fri, May 24, 2019 at 2:01 AM Richard Biener <rguenther@suse.de> wrote: > >> > >>> +int __attribute__((__ms_hook_prologue__)) func(); /* no warnings */ > >>> + > >> > >> But this is a declaration? > > > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > >> > >> 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? > > > > As far as I can tell, "fndecl" is a misnomer: these attributes are > > more accurately called "function definition attributes", i.e. > > attributes that affect the assembly code of the function but do not > > affect its calling convention. > > That's one possible definition but there are examples that don't > fit it (at least not very neatly). > > Attribute malloc attaches only to fndecl and not its type but > doesn't affect the code for a function definition. FWIW, I > think this is just a bug -- attribute malloc should apply > to a function type for the same reason the closely related > attribute alloc_size does. > > Attribute constructor also attaches to a fndecl even though it > doesn't affect the function's codegen but that of its caller > (the runtime). In this case, though, I'd say that's fine. > Should it be classified as a function definition attribute? > > Attributes cold and hot also apply to a fndecl and not its type > but they affect both the function's definition and its callers. > I think this also makes sense. Should they be classified as > function definition attributes? > > > On Fri, May 24, 2019 at 9:23 AM Martin Sebor <msebor@gmail.com> wrote: > >> > >> 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). > > > > Okay, I will add tests for the other function attributes that should > > behave in the same way, commenting out the tests that will require > > more work to pass. > > > > The end goal is to include __ms_hook_prologue__ in the WINAPI macro on > > Wine without causing spurious warnings. This will go a long way > > towards making Wine compatible with current and future Windows > > programs. Thank you for help. > > Yes, I understand the goal. I'm not sure that the proposed change > is the right way to do it. It seems to me that a more targeted bug > to fix with the broadest benefit is that _Pragma GCC diagnostic (or > some such mechanism) cannot be used to suppress the warning for > typedefs. > > Martin
On Thu, 30 May 2019, Alex Henrie wrote: > At this point I think I'm convinced that any attribute that applies to > a function should also be allowed on a function pointer without any > warnings. We can start by turning off the warnings for the "fndecl" > attributes and then clean up the other attributes as time goes on. This is inherently a property of the attribute in question. The issue is not whether it applies to function pointers; it's whether it applies to function types. For example, the "section" or "alias" attributes are attributes that apply to a declaration, but not a type. Because they apply to variables as well as functions, they are meaningful on function pointers - but the meaning is *not* the same as applying them to the pointed-to function. The "flatten" attribute, however, seems only meaningful for functions, not variables, not function types and not function pointers. We should try to work out for each attribute exactly what construct it appertains to - which for many but not all function attributes is indeed the type of the function rather than the function itself. Then move to making such attributes work on types. But for attributes such as "flatten" that logically appertain to the declaration not its type, we should continue to diagnose them on function pointers or types.
On Thu, May 30, 2019 at 6:59 PM Joseph Myers <joseph@codesourcery.com> wrote: > > On Thu, 30 May 2019, Alex Henrie wrote: > > > At this point I think I'm convinced that any attribute that applies to > > a function should also be allowed on a function pointer without any > > warnings. We can start by turning off the warnings for the "fndecl" > > attributes and then clean up the other attributes as time goes on. > > This is inherently a property of the attribute in question. The issue is > not whether it applies to function pointers; it's whether it applies to > function types. > > For example, the "section" or "alias" attributes are attributes that apply > to a declaration, but not a type. Because they apply to variables as well > as functions, they are meaningful on function pointers - but the meaning > is *not* the same as applying them to the pointed-to function. > > The "flatten" attribute, however, seems only meaningful for functions, not > variables, not function types and not function pointers. > > We should try to work out for each attribute exactly what construct it > appertains to - which for many but not all function attributes is indeed > the type of the function rather than the function itself. Then move to > making such attributes work on types. But for attributes such as > "flatten" that logically appertain to the declaration not its type, we > should continue to diagnose them on function pointers or types. In Wine we need a way to (without warnings) put ms_hook_prologue into a macro that is applied to functions, function pointers, and function pointer typedefs. It sounds like you're saying that you will not accept a patch that silences or splits off warnings about using ms_hook_prologue with function pointers and function pointer typedefs. So how do you think Wine's problem should be solved? It seems to me that any information about the target of a function pointer, even the flatten attribute or the ms_hook_prologue attribute, provides information that could be useful for optimizing the code around the indirect function call. That sounds like a compelling argument for allowing these attributes in more places without warnings. -Alex
On Thu, 30 May 2019, Alex Henrie wrote: > On Thu, May 30, 2019 at 6:59 PM Joseph Myers <joseph@codesourcery.com> wrote: > > > > On Thu, 30 May 2019, Alex Henrie wrote: > > > > > At this point I think I'm convinced that any attribute that applies to > > > a function should also be allowed on a function pointer without any > > > warnings. We can start by turning off the warnings for the "fndecl" > > > attributes and then clean up the other attributes as time goes on. > > > > This is inherently a property of the attribute in question. The issue is > > not whether it applies to function pointers; it's whether it applies to > > function types. > > > > For example, the "section" or "alias" attributes are attributes that apply > > to a declaration, but not a type. Because they apply to variables as well > > as functions, they are meaningful on function pointers - but the meaning > > is *not* the same as applying them to the pointed-to function. > > > > The "flatten" attribute, however, seems only meaningful for functions, not > > variables, not function types and not function pointers. > > > > We should try to work out for each attribute exactly what construct it > > appertains to - which for many but not all function attributes is indeed > > the type of the function rather than the function itself. Then move to > > making such attributes work on types. But for attributes such as > > "flatten" that logically appertain to the declaration not its type, we > > should continue to diagnose them on function pointers or types. > > In Wine we need a way to (without warnings) put ms_hook_prologue into > a macro that is applied to functions, function pointers, and function > pointer typedefs. It sounds like you're saying that you will not > accept a patch that silences or splits off warnings about using > ms_hook_prologue with function pointers and function pointer typedefs. > So how do you think Wine's problem should be solved? I think ms_hook_prologue should be allowed to apply to function types and function decls. If you say it should apply to function pointers then I suppose you want to have it apply to the pointed to function of the function pointer - but that's not possible without an indirection via a function pointer typedef IIRC. I also have the following which _may_ motivate that attributes currently not applying to function types (because they only affect function definitions) should also apply there: typedef int (myfun) (int *) __attribute__((nonnull(1))); myfun x; int x(int *p) { return p != (int*)0; } this applies nonnull to the function definition of 'x' but I put the attribute on the typedef. I didn't manage to do without the myfun x; declaration. > It seems to me that any information about the target of a function > pointer, even the flatten attribute or the ms_hook_prologue attribute, > provides information that could be useful for optimizing the code > around the indirect function call. That sounds like a compelling > argument for allowing these attributes in more places without > warnings. Sure. Can you write down the three cases after macro expansion here to clarify what you need? Esp. say what the attribute should apply to. Just silencing the warning without actually achieving what you want would be bad I think ;) Richard.
On Fri, May 31, 2019 at 1:38 AM Richard Biener <rguenther@suse.de> wrote: > > On Thu, 30 May 2019, Alex Henrie wrote: > > > In Wine we need a way to (without warnings) put ms_hook_prologue into > > a macro that is applied to functions, function pointers, and function > > pointer typedefs. It sounds like you're saying that you will not > > accept a patch that silences or splits off warnings about using > > ms_hook_prologue with function pointers and function pointer typedefs. > > So how do you think Wine's problem should be solved? > > I think ms_hook_prologue should be allowed to apply to function types > and function decls. If you say it should apply to function pointers > then I suppose you want to have it apply to the pointed to function > of the function pointer - but that's not possible without an indirection > via a function pointer typedef IIRC. No, if ms_hook_prologue is applied to a function pointer, it shouldn't do anything except maybe trigger some optimization of the code around the indirect function call. > I also have the following which _may_ motivate that attributes > currently not applying to function types (because they only > affect function definitions) should also apply there: > > typedef int (myfun) (int *) __attribute__((nonnull(1))); > myfun x; > int x(int *p) { return p != (int*)0; } > > this applies nonnull to the function definition of 'x' but > I put the attribute on the typedef. I didn't manage to > do without the myfun x; declaration. That is a great example and another compelling reason to allow "fndecl" attributes in more places. > > It seems to me that any information about the target of a function > > pointer, even the flatten attribute or the ms_hook_prologue attribute, > > provides information that could be useful for optimizing the code > > around the indirect function call. That sounds like a compelling > > argument for allowing these attributes in more places without > > warnings. > > Sure. Can you write down the three cases after macro expansion > here to clarify what you need? Esp. say what the attribute should > apply to. Just silencing the warning without actually achieving > what you want would be bad I think ;) Essentially, the following needs to compile without warnings: #define WINAPI __attribute__((__stdcall__)) \ __attribute__((__ms_hook_prologue__)) typedef unsigned int (WINAPI *APPLICATION_RECOVERY_CALLBACK)(void*); void WINAPI foo() { APPLICATION_RECOVERY_CALLBACK bar; unsigned int (WINAPI *baz)(void*); } -Alex
On Fri, 31 May 2019, Alex Henrie wrote: > On Fri, May 31, 2019 at 1:38 AM Richard Biener <rguenther@suse.de> wrote: > > > > On Thu, 30 May 2019, Alex Henrie wrote: > > > > > In Wine we need a way to (without warnings) put ms_hook_prologue into > > > a macro that is applied to functions, function pointers, and function > > > pointer typedefs. It sounds like you're saying that you will not > > > accept a patch that silences or splits off warnings about using > > > ms_hook_prologue with function pointers and function pointer typedefs. > > > So how do you think Wine's problem should be solved? > > > > I think ms_hook_prologue should be allowed to apply to function types > > and function decls. If you say it should apply to function pointers > > then I suppose you want to have it apply to the pointed to function > > of the function pointer - but that's not possible without an indirection > > via a function pointer typedef IIRC. > > No, if ms_hook_prologue is applied to a function pointer, it shouldn't > do anything except maybe trigger some optimization of the code around > the indirect function call. > > > I also have the following which _may_ motivate that attributes > > currently not applying to function types (because they only > > affect function definitions) should also apply there: > > > > typedef int (myfun) (int *) __attribute__((nonnull(1))); > > myfun x; > > int x(int *p) { return p != (int*)0; } > > > > this applies nonnull to the function definition of 'x' but > > I put the attribute on the typedef. I didn't manage to > > do without the myfun x; declaration. > > That is a great example and another compelling reason to allow > "fndecl" attributes in more places. > > > > It seems to me that any information about the target of a function > > > pointer, even the flatten attribute or the ms_hook_prologue attribute, > > > provides information that could be useful for optimizing the code > > > around the indirect function call. That sounds like a compelling > > > argument for allowing these attributes in more places without > > > warnings. > > > > Sure. Can you write down the three cases after macro expansion > > here to clarify what you need? Esp. say what the attribute should > > apply to. Just silencing the warning without actually achieving > > what you want would be bad I think ;) > > Essentially, the following needs to compile without warnings: > > #define WINAPI __attribute__((__stdcall__)) \ > __attribute__((__ms_hook_prologue__)) > > typedef unsigned int (WINAPI *APPLICATION_RECOVERY_CALLBACK)(void*); > > void WINAPI foo() > { > APPLICATION_RECOVERY_CALLBACK bar; > unsigned int (WINAPI *baz)(void*); > } OK, so it's being that attributes with effect only on function bodies are harmless on function types / indirect calls. Of course in case the user expects sth like a thunk to be generated for calls through such type then a warning that the attribute has no effect is warranted. I'd vote for splitting -Wattributes to distinguish t.c:2:1: warning: ‘ms_ho_prologue’ attribute directive ignored [-Wattributes] void __attribute__((__ms_ho_prologue__)) bar () {} ^~~~ t.c:4:1: warning: ‘ms_hook_prologue’ attribute only applies to functions [-Wattributes] typedef void __attribute__((__ms_hook_prologue__)) (*fn_t)(); so you could disable the second while retaining the first. You could be also more careful in the source where you place the attributes... Richard.
On Thu, 30 May 2019, Alex Henrie wrote: > In Wine we need a way to (without warnings) put ms_hook_prologue into > a macro that is applied to functions, function pointers, and function > pointer typedefs. It sounds like you're saying that you will not > accept a patch that silences or splits off warnings about using > ms_hook_prologue with function pointers and function pointer typedefs. > So how do you think Wine's problem should be solved? By actually implementing the required semantics, not by silencing warnings that say you're using an attribute in a location for which no semantics for that attribute are known. 1. An attribute appertaining to a function pointer (an object whose type is pointer to function) is something different from an attribute appertaining to the type of the pointed-to function. For example, the "section" attribute appertains to declarations. If you apply it to the declaration of a function, it puts that function in that section. If you apply it to the declaration of an object whose type is pointer to function, it puts that object in that section (it does not say what section the pointed-to function is in). 2. The "Attribute Syntax" section in the manual describes how to determine to what construct an attribute in the GNU syntax appertains. Please note that this is *not* always the same construct to which a C++ / C2x attribute in the same place would appertain (I might look at C2x attribute implementation and other C2x features implementation around August/September, not before then). You should first be familiar with how the C standard defines the syntax for declarators and their corresponding types, in order to follow the extensions for attributes included in declarators. 3. There are certain cases where GCC is deliberately lax about attributes being in the syntactically correct places, which in particular makes it possible to change an attribute from being a declaration attribute to one on function types without breaking existing source code expecting it to be on function declarations. See the type_required and function_type_required members of struct attribute_spec. Specifying an attribute as being a function type attribute like that means it can be specified in a position where syntactically it should appertain to the declaration of the function pointer object, and be automatically made to appertain to the pointed-to type instead - but that only works if all the code in GCC that looks to see whether some declaration has the attribute present, and acts accordingly, is changed to check for the attribute on the type instead. 4. So suppose you have a particular attribute, currently only accepted on function declarations, that you wish to be accepted on function types. You need to find all the places checking for that attribute on declarations within GCC, to implement its semantics, and change them (a) to check the type of the declaration instead and (b) to work with the appropriate type when there isn't a declaration at all, as for indirect calls. (What's involved in this may depend on whether the code just checks the list of attributes on the declaration, or whether there is some other bit set on the tree that may or may not be present for types at all.) You also need to change the relevant table of attributes to set type_required and function_type_required instead of decl_required. And you need to change the attribute handler accordingly so that it works when given a type rather than a declaration.
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. + 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; +}