| Message ID | 20191121131128.7563-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] let function definition inherit prototype attributes | expand |
On 21/11/2019 13:11, Luc Van Oostenryck wrote: > It's common to declare a function with the attribute > 'pure' or 'noreturn' and to omit the attribute in the > function definition. It mak somehow sense since the > information conveyed by these attributes are destined > to the function users not the function itself. > > So, when checking declaration/definition, let the > current symbol inherit any function attributes present > in previous declarations. > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > > The patch is also available for review & testing at: > git://github.com/lucvoo/sparse-dev.git fun-attr-inherit > > Changes since v1: > * the old name was 'allow omitted function attribute in definition' > * change the approach: instead of filtering out these attributes at > check-time, let's inherit them at declaration-time. Yep, I noticed that you didn't push the last patch #5. I had also thought that this would be a better approach, or rather that the test should be that the 'attributes list' given on the function definition should be a subset of the list given on the declaration. (so that the 'extra' attributes would be inherited by the function). This would imply, of course, that sparse should issue a warning/error when the attribute list on the definition contained some attributes that were not present in the declaration. Well, it seems reasonable anyway! ;-) It seems gcc has other ideas: $ cat -n junk.c 1 2 extern void exit (int __status) __attribute__ ((__noreturn__)); 3 4 void func0(int a) __attribute__ ((pure)); 5 6 __attribute__ ((pure)) __attribute__ ((__noreturn__)) 7 void func0(int a) 8 { 9 exit(0); 10 } $ So, sparse (with this patch applied) gets it: $ ./sparse junk.c junk.c:7:6: error: symbol 'func0' redeclared with different type (originally declared at junk.c:4) - different modifiers $ But gcc, not so much: $ gcc -c junk.c $ gcc -Wall -c junk.c $ gcc -Wall -Wextra -c junk.c junk.c: In function ‘func0’: junk.c:7:16: warning: unused parameter ‘a’ [-Wunused-parameter] void func0(int a) ^ $ So, I don't know ... ATB, Ramsay Jones > > > symbol.c | 9 +++++++++ > ...ion-attribute-omitted.c => function-redecl-funattr.c} | 3 +-- > validation/function-redecl2.c | 3 --- > 3 files changed, 10 insertions(+), 5 deletions(-) > rename validation/{function-attribute-omitted.c => function-redecl-funattr.c} (75%) > > diff --git a/symbol.c b/symbol.c > index 90149e5ae..bafa7c432 100644 > --- a/symbol.c > +++ b/symbol.c > @@ -588,6 +588,14 @@ struct symbol *befoul(struct symbol *type) > return NULL; > } > > +static void inherit_declaration(struct symbol *sym, struct symbol *prev) > +{ > + unsigned long mods = prev->ctype.modifiers; > + > + // inherit function attributes > + sym->ctype.modifiers |= mods & MOD_FUN_ATTR; > +} > + > void check_declaration(struct symbol *sym) > { > int warned = 0; > @@ -598,6 +606,7 @@ void check_declaration(struct symbol *sym) > continue; > if (sym->scope == next->scope) { > sym->same_symbol = next; > + inherit_declaration(sym, next); > return; > } > /* Extern in block level matches a TOPLEVEL non-static symbol */ > diff --git a/validation/function-attribute-omitted.c b/validation/function-redecl-funattr.c > similarity index 75% > rename from validation/function-attribute-omitted.c > rename to validation/function-redecl-funattr.c > index 43b301d8f..b1e2fb195 100644 > --- a/validation/function-attribute-omitted.c > +++ b/validation/function-redecl-funattr.c > @@ -9,6 +9,5 @@ void __noreturn n(void); > void n(void) { while (1) ; } > > /* > - * check-name: function-attribute-omitted > - * check-known-to-fail > + * check-name: function-redecl-funattr > */ > diff --git a/validation/function-redecl2.c b/validation/function-redecl2.c > index 3435aa00c..ef396137a 100644 > --- a/validation/function-redecl2.c > +++ b/validation/function-redecl2.c > @@ -25,7 +25,4 @@ void func2(int a) > > /* > * check-name: function-redecl2 > - * > - * check-known-to-fail > - * > */ >
On Fri, Nov 22, 2019 at 04:23:27PM +0000, Ramsay Jones wrote: > > > On 21/11/2019 13:11, Luc Van Oostenryck wrote: > > It's common to declare a function with the attribute > > 'pure' or 'noreturn' and to omit the attribute in the > > function definition. It mak somehow sense since the > > information conveyed by these attributes are destined > > to the function users not the function itself. > > > > So, when checking declaration/definition, let the > > current symbol inherit any function attributes present > > in previous declarations. > > > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > --- > > > > The patch is also available for review & testing at: > > git://github.com/lucvoo/sparse-dev.git fun-attr-inherit > > > > Changes since v1: > > * the old name was 'allow omitted function attribute in definition' > > * change the approach: instead of filtering out these attributes at > > check-time, let's inherit them at declaration-time. > > Yep, I noticed that you didn't push the last patch #5. I had also > thought that this would be a better approach, or rather that the > test should be that the 'attributes list' given on the function > definition should be a subset of the list given on the declaration. > (so that the 'extra' attributes would be inherited by the function). > > This would imply, of course, that sparse should issue a warning/error > when the attribute list on the definition contained some attributes > that were not present in the declaration. Well, it seems reasonable > anyway! ;-) Yes, sure. Ultimatly, maybe the disctinction should be made between the attributes: 1) that matter for the user of the function: in this case the attributes should be already present in the declaration. 2) that doesn't matter for the user: in this case, it also doesn't matter if already present in the declaration or not. Note: some properties, may only matter for the user and not to the definition. 'pure' & 'noreturn' are not really in this case because, ideally, the definition should check that it is indeed pure or doesn't return (or, at least, may not always return). So, simply inheriting the attributes like done here is too simple. > It seems gcc has other ideas: > > $ cat -n junk.c > 1 > 2 extern void exit (int __status) __attribute__ ((__noreturn__)); > 3 > 4 void func0(int a) __attribute__ ((pure)); > 5 > 6 __attribute__ ((pure)) __attribute__ ((__noreturn__)) > 7 void func0(int a) > 8 { > 9 exit(0); > 10 } > $ > > So, sparse (with this patch applied) gets it: > > $ ./sparse junk.c > junk.c:7:6: error: symbol 'func0' redeclared with different type (originally declared at junk.c:4) - different modifiers > $ > > But gcc, not so much: > > $ gcc -c junk.c > $ gcc -Wall -c junk.c > $ gcc -Wall -Wextra -c junk.c > junk.c: In function ‘func0’: > junk.c:7:16: warning: unused parameter ‘a’ [-Wunused-parameter] > void func0(int a) > ^ > $ > > So, I don't know ... In my case, with gcc 8.3, the result is different: $ gcc -c junk.c junk.c:4:1: warning: 'pure' attribute on function returning 'void' [-Wattributes] void func0(int a) __attribute__ ((pure)); ^~~~ junk.c:8:1: warning: 'pure' attribute on function returning 'void' [-Wattributes] { ^ junk.c:8:1: warning: ignoring attribute 'noreturn' because it conflicts with attribute 'pure' [-Wattributes] junk.c:4:6: note: previous declaration here void func0(int a) __attribute__ ((pure)); ^~~~~ junk.c:8:1: warning: ignoring attribute 'noreturn' because it conflicts with attribute 'pure' [-Wattributes] { ^ junk.c:4:6: note: previous declaration here void func0(int a) __attribute__ ((pure)); ^~~~~ but this doesn't invalid your point. A priori, though, it seems to me that simply ignoring these function attributes when checking the declaration/definition compatibility, like it was done in the previous patch, is inferior. I'll need to think a bit more about all this. Thanks for your feedback. -- Luc
On Fri, Nov 22, 2019 at 11:16:58PM +0100, Luc Van Oostenryck wrote: > On Fri, Nov 22, 2019 at 04:23:27PM +0000, Ramsay Jones wrote: > > > So, sparse (with this patch applied) gets it: > > > > $ ./sparse junk.c > > junk.c:7:6: error: symbol 'func0' redeclared with different type (originally declared at junk.c:4) - different modifiers > > $ > > > > But gcc, not so much: > > > > $ gcc -c junk.c > > $ gcc -Wall -c junk.c > > $ gcc -Wall -Wextra -c junk.c > > junk.c: In function ‘func0’: > > junk.c:7:16: warning: unused parameter ‘a’ [-Wunused-parameter] > > void func0(int a) > > ^ > > $ > > > > So, I don't know ... [...] > A priori, though, it seems to me that simply ignoring these function > attributes when checking the declaration/definition compatibility, > like it was done in the previous patch, is inferior. > I'll need to think a bit more about all this. I'll push this version. I think it's quite sane, better than the current situation and it's normal for sparse to be more picky than GCC about these things. Best regards, -- Luc
diff --git a/symbol.c b/symbol.c index 90149e5ae..bafa7c432 100644 --- a/symbol.c +++ b/symbol.c @@ -588,6 +588,14 @@ struct symbol *befoul(struct symbol *type) return NULL; } +static void inherit_declaration(struct symbol *sym, struct symbol *prev) +{ + unsigned long mods = prev->ctype.modifiers; + + // inherit function attributes + sym->ctype.modifiers |= mods & MOD_FUN_ATTR; +} + void check_declaration(struct symbol *sym) { int warned = 0; @@ -598,6 +606,7 @@ void check_declaration(struct symbol *sym) continue; if (sym->scope == next->scope) { sym->same_symbol = next; + inherit_declaration(sym, next); return; } /* Extern in block level matches a TOPLEVEL non-static symbol */ diff --git a/validation/function-attribute-omitted.c b/validation/function-redecl-funattr.c similarity index 75% rename from validation/function-attribute-omitted.c rename to validation/function-redecl-funattr.c index 43b301d8f..b1e2fb195 100644 --- a/validation/function-attribute-omitted.c +++ b/validation/function-redecl-funattr.c @@ -9,6 +9,5 @@ void __noreturn n(void); void n(void) { while (1) ; } /* - * check-name: function-attribute-omitted - * check-known-to-fail + * check-name: function-redecl-funattr */ diff --git a/validation/function-redecl2.c b/validation/function-redecl2.c index 3435aa00c..ef396137a 100644 --- a/validation/function-redecl2.c +++ b/validation/function-redecl2.c @@ -25,7 +25,4 @@ void func2(int a) /* * check-name: function-redecl2 - * - * check-known-to-fail - * */
It's common to declare a function with the attribute 'pure' or 'noreturn' and to omit the attribute in the function definition. It mak somehow sense since the information conveyed by these attributes are destined to the function users not the function itself. So, when checking declaration/definition, let the current symbol inherit any function attributes present in previous declarations. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- The patch is also available for review & testing at: git://github.com/lucvoo/sparse-dev.git fun-attr-inherit Changes since v1: * the old name was 'allow omitted function attribute in definition' * change the approach: instead of filtering out these attributes at check-time, let's inherit them at declaration-time. symbol.c | 9 +++++++++ ...ion-attribute-omitted.c => function-redecl-funattr.c} | 3 +-- validation/function-redecl2.c | 3 --- 3 files changed, 10 insertions(+), 5 deletions(-) rename validation/{function-attribute-omitted.c => function-redecl-funattr.c} (75%)