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@xxxxxxxxx>
> > ---
> >
> > 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
