[Bug ipa/109914] New: --suggest-attribute=pure misdiagnoses static functions

[Bug ipa/109914] New: --suggest-attribute=pure misdiagnoses static functions

[bruno at clisp dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

            Bug ID: 109914
           Summary: --suggest-attribute=pure misdiagnoses static functions
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bruno at clisp dot org
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---

Created attachment 55125
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55125&action=edit
test case

GCC's --suggest-attribute=pure diagnoses static functions, even though the
'pure' attribute is useless for static functions (after all, the compiler has
deduced the property on its own). This is leading to my having to litter code
with '__attribute__ (pure)' declarations merely to pacify GCC. GCC should treat
the 'pure' attribute like other attributes (e.g., malloc, const), and should
issue the diagnostic only for non-static functions where the attribute is in
fact useful.

How to reproduce:
$ gcc --version
gcc (GCC) 13.1.0
...
$ gcc -Wsuggest-attribute=pure -O2 -S file-has-acl.c
file-has-acl.c: In function ‘have_xattr’:
file-has-acl.c:3385:14: warning: function might be candidate for attribute
‘pure’ if it is known to return normally [-Wsuggest-attribute=pure]
 3385 | static _Bool have_xattr (char const *attr, char const *listbuf, ssize_t
listsize)
      |              ^~~~~~~~~~

[Bug ipa/109914] --suggest-attribute=pure misdiagnoses static functions

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 109915 has been marked as a duplicate of this bug. ***

[Bug ipa/109914] --suggest-attribute=pure misdiagnoses static functions

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

--- Comment #2 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The reason why gcc warns is that it is unable to prove that the function is
always finite. This means that it can not auto-detect pure attribute since
optimizing the call out may turn infinite program to finite one. 
So adding the attribute would still help compiler to know that the loops are
indeed finite.

[Bug ipa/109914] --suggest-attribute=pure misdiagnoses static functions

[bruno at clisp dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

--- Comment #3 from Bruno Haible <bruno at clisp dot org> ---
(In reply to Jan Hubicka from comment #2)
> The reason why gcc warns is that it is unable to prove that the function is
> always finite. This means that it can not auto-detect pure attribute since
> optimizing the call out may turn infinite program to finite one. 
> So adding the attribute would still help compiler to know that the loops are
> indeed finite.

Thanks for explaining. So, the warning asks the developer not only to add an
__attribute__((__pure__)) marker, but also to verify that the function
terminates. In this case, it does, but it took me a minute of reflection to
convince myself.

For what purpose shall the developer make this effort? The documentation
https://gcc.gnu.org/onlinedocs/gcc-13.1.0/gcc/Common-Function-Attributes.html
says that it's to allow the compiler to do common subexpression elimination.
But in this case, the compiler could easily find out that it cannot do common
subexpression elimination anyway, because:
  - The only caller of this function (have_xattr) is file_has_acl.
  - In this function, there are three calls to have_xattr.
  - Each of them is executed only at most once. Control flow analysis shows
this.
  - Each of them has different argument lists: The first argument is a string
literal in each case, namely "system.nfs4_acl", "system.posix_acl_access",
"system.posix_acl_default" respectively.
So, there is no possibility for common subexpression elimination here, even if
the function was marked "pure".

Therefore it is pointless to suggest to the developer that it would be a gain
to mark the function as "pure" and that it is worth spending brain cycles on
that.

[Bug ipa/109914] --suggest-attribute=pure misdiagnoses static functions

[eggert at cs dot ucla.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

Paul Eggert <eggert at cs dot ucla.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |eggert at cs dot ucla.edu

--- Comment #4 from Paul Eggert <eggert at cs dot ucla.edu> ---
(In reply to Jan Hubicka from comment #2)
> The reason why gcc warns is that it is unable to prove that the function is
> always finite.

I don't see why finiteness matters. If a pure function returns the first time
it's called, and if there's no change to visible state after that, it should
return the second time with the same value. And if the pure function didn't
return the first time evaluation won't even get to the second time. So common
subexpression elimination (which is the point of 'pure') will work even if a
pure function does not return.

C23 has standardized this stuff with the [[reproducible]] attribute, and as far
as I can see (the wording is admittedly murky) C23 does not impose a finiteness
constraint on reproducible functions. If my reading of C23 is correct, GCC
should not impose finiteness constraints on reproducible functions when it gets
around to implementing [[reproducible]], and if [[reproducible]] and
__attribute__((pure)) are supposed to be the same thing then GCC should drop
the  finiteness constraint on pure functions as well.


I agree with Bruno's main point that none of this stuff should matter for
static functions. --suggest-attribute=* warnings are useless chatter for static
functions.


(I ran into this GCC bug when building recent versions of the TZDB code, which
is why I found this bug report.)

[Bug ipa/109914] --suggest-attribute=pure misdiagnoses static functions

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

--- Comment #5 from Jan Hubicka <hubicka at ucw dot cz> ---
> (In reply to Jan Hubicka from comment #2)
> > The reason why gcc warns is that it is unable to prove that the function is
> > always finite.
> 
> I don't see why finiteness matters. If a pure function returns the first time
> it's called, and if there's no change to visible state after that, it should
> return the second time with the same value. And if the pure function didn't
> return the first time evaluation won't even get to the second time. So common
> subexpression elimination (which is the point of 'pure') will work even if a
> pure function does not return.

yes, however both const and pure attributes allows compiler to also
remove the call if return value is unused.  So here finiteness matters.
> 
> C23 has standardized this stuff with the [[reproducible]] attribute, and as far
> as I can see (the wording is admittedly murky) C23 does not impose a finiteness
> constraint on reproducible functions. If my reading of C23 is correct, GCC
> should not impose finiteness constraints on reproducible functions when it gets
> around to implementing [[reproducible]], and if [[reproducible]] and
> __attribute__((pure)) are supposed to be the same thing then GCC should drop
> the  finiteness constraint on pure functions as well.

Interesting, I did not notice that. These notions are not the same.

However ipa-mod-ref internally knows if function has side effects (i.e.
can be infinite) and if it is reproducible (i.e. will do same thing when
called again) and we could use this for redundancy elimination if that
pass is extended to remove redundant calls.

So I guess adding reproducible attribute in addition to pure and const
makes sense.
> 
> 
> I agree with Bruno's main point that none of this stuff should matter for
> static functions. --suggest-attribute=* warnings are useless chatter for static
> functions.
It helps the compiler to solve the halting problem.

[Bug ipa/109914] --suggest-attribute=pure misdiagnoses static functions

[eggert at cs dot ucla.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109914

--- Comment #6 from Paul Eggert <eggert at cs dot ucla.edu> ---
(In reply to Jan Hubicka from comment #5)
> yes, however both const and pure attributes allows compiler to also
> remove the call if return value is unused.  So here finiteness matters.
Thanks for mentioning that. I now see that there are other differences between
const/pure and unsequenced/reproducible: see N2956
<https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2956.htm#gcc>, which says
that the GCC notions are stricter than the corresponding C23 notions. N2956
doesn't mention finiteness as one of the strictness differences, though, I
guess because the C23 standardizers didn't notice that part of the GCC
documentation.

So my idea that "[[reproducible]] and __attribute__((pure)) are supposed to be
the same thing" is incorrect. Similarly, [[unsequenced]] and
__attribute__((const) are not the same thing. Oh well. We may need to change
Gnulib because of these discrepancies.

>> I agree with Bruno's main point that none of this stuff should matter for
>> static functions. --suggest-attribute=* warnings are useless chatter for
>> static functions.
> It helps the compiler to solve the halting problem.
This isn't a halting-problem situation. If GCC cannot deduce that a static
function halts, and if no calls to that function discard the return value (so
the optimization you mentioned can't apply), then suggesting to the developer
to add __attribute__((pure)) merely wastes developers' time, and developers
either disable the warning or ignore it, neither of which is good. So Bruno's
suggestion of suppressing the false positive for his test case still seems to
be a good one.