Bug 18487 - Warnings for pure and const functions that are not actually pure or const
Summary: Warnings for pure and const functions that are not actually pure or const
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.0.0
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
: 33048 37064 64720 81518 101119 109792 (view as bug list)
Depends on:
Blocks:
 
Reported: 2004-11-14 21:37 UTC by Kazu Hirata
Modified: 2023-05-09 18:07 UTC (History)
13 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-10-24 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kazu Hirata 2004-11-14 21:37:23 UTC
It would be nice if GCC can issue warnings for pure or const functions
that are not actually pure or const.

int foo (int) __attribute__ ((pure));
int bar (int) __attribute__ ((const));

int g;

int
foo (int a)
{
  g = a; /* A pure function is not supposed to write to memory.  */
  return 0;
}

int
bar (int a)
{
  return g; /* A const function is not supposed to read memory.  */
}
Comment 1 Andrew Pinski 2004-11-14 21:57:50 UTC
Confirmed.
Comment 2 Daniel Berlin 2004-11-16 22:43:56 UTC
Actually, I think this is a remarkably bad idea, and would like to close this as
wontfix.

Pure and const are things that are not easily verifiable by the compiler in a
lot of common cases (it may get false negatives), and it's something the user is
affirmatively asserting.

In short, you shouldn't be saying it's pure or const if it's not pure or const.
If the compiler could tell whether you were right or not in all cases, you
wouldn't need the attributes in the first place.

False negatives appear commonly because left to it's own devices, no external
function could be marked pure or const by the compiler (because it has no idea
about what they do), and any function that called external functions could get
false negatives (because if the compiler couldn't rely on your assertions about
const or pure, these functions would also necessarily be not const or pure).
If the compiler is allowed to rely on your assertions when trying to verify it
for purposes of this warning, then the warning would miss cases where you got it
wrong anyway, because it would rely on your wrong assertion :P.

So basically, this is not something we can verify in anything but the most
trivial cases, we almost always end up relying on what the user tells us.   And
if the user is marking trivial functions const or pure when they clearly aren't,
they deserve what they get :).





Comment 3 Kazu Hirata 2004-11-17 00:09:16 UTC
This is hard.
Comment 4 Andrew Pinski 2007-08-11 17:54:54 UTC
*** Bug 33048 has been marked as a duplicate of this bug. ***
Comment 5 Behdad Esfahbod 2007-08-13 05:40:53 UTC
(In reply to comment #2)

> If the compiler could tell whether you were right or not in all cases, you
> wouldn't need the attributes in the first place.

This is not completely true though: the compiler cannot tell by just seeing the prototype.  So, even if the compiler could recognize all pure and const functions when compiling them, that doesn't help when you need to mark the prototypes of those functions as pure/const to help the compiler compiling other programs using them.
Comment 6 Andrew Pinski 2008-08-08 21:01:12 UTC
*** Bug 37064 has been marked as a duplicate of this bug. ***
Comment 7 Andrew Pinski 2015-01-21 22:56:24 UTC
*** Bug 64720 has been marked as a duplicate of this bug. ***
Comment 8 Andrew Pinski 2015-01-21 22:58:13 UTC
I don't think I agree with closing this as won't fix as shown now we have three duplicated bugs asking the same thing.
Comment 9 David Malcolm 2015-01-22 00:44:25 UTC
(In reply to Andrew Pinski from comment #8)
> I don't think I agree with closing this as won't fix as shown now we have
> three duplicated bugs asking the same thing.

I agree; reopening.

It may not be possible to implement a perfect test for this, but it seems to me that it's possible to implement a warning that catches a subset of cases without false positives (at the cost of having false negatives).

As a first iteration of an implementation:

  for all fns labelled as pure/const:
    for all basic blocks that are guaranteed to be in a
         path through the function:
      for all stmts in BB:
        if stmt is non-pure/non-const:
          issue a warning

(yes, this would miss some things e.g.
   if (COND)
     non-pure
   else
     non-pure
but could be cheap and effective)
Comment 10 David Malcolm 2015-01-22 00:47:44 UTC
fizzbooze: you were asking on IRC about where the existing implementation is; see gcc/ipa-pure-const.c - though I believe that merely covers tracking the user-provided flags interprocedurally; I don't think it covers walking individual stmts and detecting side-effects.
Comment 11 Jakub Jelinek 2015-01-22 07:06:33 UTC
It would have to be a warning, and would necessarily not catch many things, e.g. if you use inline asm, it is hard to find out if it is non-pure, as has been mentioned earlier, dead code or possible dead code should not be warned about, and if you call other functions, even if they aren't declared pure, it might be that they don't write anything with the given arguments or arranged through other means.
Comment 12 Jakub Jelinek 2015-01-22 07:17:11 UTC
And then there is the case of endless loops in such functions (either unconditional, or ones the compiler is not able to detect), exit calls, both either directly in the const/pure function or in some function it calls.  So in all, I'm afraid such a warning would diagnose only the most trivial cases.
Comment 13 Martin Sebor 2016-10-24 19:50:09 UTC
Let me confirm this request.  I think diagnosing even just the simple cases would be worthwhile.  The -Wsuggest-atribute= option isn't perfect either but it is presumably useful nonetheless, as are many other warnings that are similarly limited and prone to both false negatives and false positives.
Comment 14 Andrew Pinski 2017-07-22 18:34:06 UTC
*** Bug 81518 has been marked as a duplicate of this bug. ***
Comment 15 Martin Sebor 2017-07-22 22:19:29 UTC
*** Bug 81518 has been marked as a duplicate of this bug. ***
Comment 16 Eric Gallager 2019-07-24 04:20:27 UTC
(In reply to Jakub Jelinek from comment #12)
> And then there is the case of endless loops in such functions (either
> unconditional, or ones the compiler is not able to detect), exit calls, both
> either directly in the const/pure function or in some function it calls.  So
> in all, I'm afraid such a warning would diagnose only the most trivial cases.

That would still be better than nothing.
Comment 17 Daniel Berlin 2019-07-24 10:31:58 UTC
Not sure how i ended up on the CC list for this one, but i actually
would disagree it would be better than nothing.
Features that can only be made to work a small amount and are
incapable of being improved tend to be the ones users complain about
the most.
In cases where you can't even get to 30% or 40%, let alone 80%, it's
often better to do nothing.

On Tue, Jul 23, 2019 at 9:20 PM egallager at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18487
>
> --- Comment #16 from Eric Gallager <egallager at gcc dot gnu.org> ---
> (In reply to Jakub Jelinek from comment #12)
> > And then there is the case of endless loops in such functions (either
> > unconditional, or ones the compiler is not able to detect), exit calls, both
> > either directly in the const/pure function or in some function it calls.  So
> > in all, I'm afraid such a warning would diagnose only the most trivial cases.
>
> That would still be better than nothing.
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 18 Dávid Bolvanský 2019-08-16 16:19:32 UTC
a.c

int foo(void) __attribute__((const));


int main(void) {
    return foo();
}

b.c

#include <stdio.h>

int foo(void) {
    puts("BUM");
    return 0;
}

gcc a.c b.c -Wall -Wextra -flto -o bum


It would be nice to get a warning.
Comment 19 Eric Gallager 2019-08-18 04:34:58 UTC
(In reply to Dávid Bolvanský from comment #18)
> a.c
> 
> int foo(void) __attribute__((const));
> 
> 
> int main(void) {
>     return foo();
> }
> 
> b.c
> 
> #include <stdio.h>
> 
> int foo(void) {
>     puts("BUM");
>     return 0;
> }
> 
> gcc a.c b.c -Wall -Wextra -flto -o bum
> 
> 
> It would be nice to get a warning.

The LTO case would probably be tougher to get right than the simple case...
Comment 20 Martin Sebor 2020-05-05 19:08:02 UTC
Working on a solution.
Comment 21 Federico Kircheis 2021-01-15 20:30:38 UTC
FWIW I was going to open a similar ticket that there are no diagnostic for [[gnu::const]] and [[gnu::pure]] used incorrectly, so I would like to add my opinion.

I think it would be very valuable when the attribute is used correctly, as it can help the compiler optimize the code, but more importantly help the reader reason about it (and verify the correctness of a program).

Currently those attributes are problematic when maintaining code.
If for example a [[gnu::pure]] function gets refactored and it's not pure anymore, then the code might misbehave, as the compiler might optimize incorrectly, and tracking down the bug is not easy.

For example, suppose we have 

----
// bar.hpp
[[gnu::const]] int get_value();

// bar.cpp
int get_value(){ return 42;}

// foo.cpp
#include "bar.hpp"
int foo(){
    int i = get_value();
    int j = get_value();
    return i+j;
}
----

The the compiler can and will optimize the second call of `get_value`.

But if the code changes, and one forgets to change the declaration:

----
// bar.hpp
[[gnu::const]] int get_value();

// bar.cpp
int get_value(){static int i = 0; return ++i;}


// foo.cpp
#include "bar.hpp"
    int i = get_value();
    int j = get_value();
    return i+j;
}
----

The compiler will still optimize the call to get_value, (unless it is able to see the definition of get_value and see that there are side effects).

I think a warning that even only works for trivial case is much better than nothing, because at least I know I can safely use the attribute for some functions as a contract to the caller, and have it checked.

If the compiler has false positives I can at least recheck the offending functions and evaluate if leaving the attribute, or removing it.
Comment 22 Andrew Pinski 2021-06-18 17:05:55 UTC
*** Bug 101119 has been marked as a duplicate of this bug. ***
Comment 23 Andrew Pinski 2021-06-18 17:07:02 UTC
In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101119#c1 it was decided this was not to be fixed.
Comment 24 Martin Sebor 2021-06-18 17:52:55 UTC
Andrew, please don't close bugs that others are working on (i.e., in an ASSIGNED state with an active Assignee).  Thanks!
Comment 25 Daniel Berlin 2021-09-04 12:55:42 UTC
This seems like a bad idea, and is impossible in general.

The whole point of the attributes is to tell the compiler things are pure/const in cases it can't already prove.

It can already prove a lot, and doesn't need help in most of the simple examples being given (in other bugs). 

You are basically going to warn in the cases the compiler can't prove it (IE sees something it thinks makes the function not pure/const), and those are *exactly* the cases the attribute exists for - where the compiler doesn't know, but you do.
Comment 26 Federico Kircheis 2021-09-04 20:27:12 UTC
As multiple people commented this Ticket, I do not know to who the least message is sent, but I would like to give again my opinion on it, as I would really like to use those attributes in non-toy projects.

> This seems like a bad idea

I think there are valid use-cases for those warnings.


> and is impossible in general

Let me quote myself:

> ... a warning that even only works for trivial case is much better than nothing, because at least I know I can safely use the attribute for some functions as a contract to the caller, and have it checked.

There are now two possible outcomes if a compiler emits a warning.

1)
I look at the definition, and *gasp*, the compiler is actually right.
The function was pure before, but the last changes made it impure.
Either I did not realize it, or I forgot to change the function declaration.
Thank you GCC for making me aware of the issue, I'll fix it.

2) 
I look at the definition an think that GCC is wrong.
I know better, and the function is pure.
I can either try to simplify the function in such a way that GCC does not complain anymore (which might be a good idea), or I can use a pragma to ignore this one warning (and comment why it's ignored), or remove the attribute altogether, as GCC might call the function multiple times if it thinks it's impure (see example at the end).
In the first approach, I can still benefit from warnings if the function changes again.
In the second case I cant but at least, I can still grep in the entire codebase and check periodically which warnings have been disabled locally, just like I do for other warnings.
In the third case yes, I would probably report a bug with a minimal example.
This (hopefully), would improve GCC analysis capabilities.


> The whole point of the attributes is to tell the compiler things are pure/const in cases it can't already prove.

That does not mean that it is not useful to let it do the check, *especially if it can prove that the attribute is used incorrectly*, but even if it can't prove anything.
And also see the example at the end why this is not completely true.

> It can already prove a lot, and doesn't need help in most of the simple examples being given (in other bugs). 


But programmers (at least for the most use-cases I've seen) needs that type of support.
I would like to know if a function has side effects.
It's great if the compiler can see it automatically, but when reading and writing code, especially code not written by me or maintained by multiple authors, we might want to restrict the functionality of some functions.

For side-effect free functions, the attributes const and pure are great, but using them is more harmful, because if used wrongly it introduces UB, thus

1) they do not really document if a function is pure, as there is no tooling checking if the statement is true
2) they introduce bugs that no-one can explain (see at the end).

Thus a comment "this function is pure", is by contrast much better, as it does not introduce UB, but we all know that those kind of commends do not age well.
Thus at the end, they get ignored because not trustworthy, and one need always to look at the implementation.

> You are basically going to warn in the cases the compiler can't prove it [...]

And for many use-cases it is fine.




Also the second example I gave:

----
// bar.hpp
[[gnu::const]] int get_value();

// bar.cpp
int get_value(){static int i = 0; return ++i;}


// foo.cpp
int foo(){
    int i = get_value();
    int j = get_value();
    return i+j;
}
----

The compiler will still optimize the call to get_value, (unless it is able to see the definition of get_value and see that there are side effects).

Thus, if the function is marked pure, the compiler

* will not call it a second time if it does not see the implementation of `get_value`
* will call it a second time if it sees the implementation of `get_value` and notices it is not pure.

This is one of those bugs that no-one can explain, as simply moving code (making a function, for example, inline, or move it to another file), or changing optimization level, changes the behavior of the program.


Thus, given main.cpp

----
[[gnu::const]] int foo();

// foo.cpp
int main(){
    int i = foo();
    int j = foo();
    return i+j;
}
----


how many times is GCC going to call foo?

If GCC thinks that the function is pure, then only once.
If it thinks it is not pure, twice.

I have no idea what GCC thinks, because there are no diagnostics for it!
And look, it does not even matter if foo is pure or not, it matters if GCC thinks if it is pure or not. 

I can similarly tell GCC to inline functions, but if GCC doesn't at least it will tell me he didn't.(warning: 'always_inline' function might not be inlinable [-Wattributes])



We can of course say "those attributes are only for those people that really know better", but as the compiler is already checking if a function is pure or not (as described, if it sees the implementation it does not elide the second call), some information is already available.
I fail to see how not making this information visible to the end user who is doing an error, and making thus those attributes available for more programmers, is a bad thing.

I'm unable to judge how difficult it is (I suppose a lot as the bug report is of 2004), and this might be the reason GCC will never be able to output the information, but please do not dismiss it because "it works as designed", as me (and suppose other reporters) believe it could be more useful than it currently is, without invalidating it's original use-case.
Comment 27 Federico Kircheis 2021-09-04 20:34:04 UTC
Edit: sorry, my last comment about what GCC thinks is wrong.

GCC seems to follow the gnu::pure/gnu::const directive to the letter, it does not ignore it when it sees the implementation of the function, thus my comment about information are already available can be ignored.
Comment 28 Federico Kircheis 2021-09-04 20:40:01 UTC
>Edit: sorry, my last comment about what GCC thinks is wrong.

Unless it is going to inline the function call, in that case the attributes are as-if ignored (at least the case I've tested with GCC 11.2).
Comment 29 Daniel Berlin 2021-09-04 20:57:42 UTC
Let me try to explain a different way:
The only functions GCC can warn about are those that don’t need the
attributes in the first place. The way any warning would work is to detect
whether it is pure/const, and then see how the user marked it. So anything
it can properly detect as right or wrong didn’t need an attribute to begin
with - the compiler could already tell if it was pure/const

Rather than tell the user they got it wrong, you might as well tell the
user to remove the attribute because it isn’t necessary and won’t be
necessary.

This is precisely why attributes are meant for when you are sure you know
more than the compiler can tell, and *no other time *. It is a tool for
experts.
Giving a bunch of really contrived examples where users may update things
wrong doesn’t seem like a good motivation to make a warning that can only
possibly have a really high false positive rate.
The same logic applies to a lot of expert-use-only attributes.  It is
assumed you know what you are doing, because the compiler can’t tell you
you are wrong accurately




On Sat, Sep 4, 2021 at 4:40 PM federico.kircheis at gmail dot com <
gcc-bugzilla@gcc.gnu.org> wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18487
>
> --- Comment #28 from Federico Kircheis <federico.kircheis at gmail dot
> com> ---
> >Edit: sorry, my last comment about what GCC thinks is wrong.
>
> Unless it is going to inline the function call, in that case the
> attributes are
> as-if ignored (at least the case I've tested with GCC 11.2).
>
> --
> You are receiving this mail because:
> You are on the CC list for the bug.
Comment 30 Federico Kircheis 2021-09-04 22:25:46 UTC
It seems to me we are not going to agree as we tend to repeat ourselves, lets see if we go around and around in circles or if it is more like a spiral ;)



Your view is more about the compiler, how it is interpreting the attributes and thus why it is unneeded, mine is more about the developers writing (but most importantly) reading it.


> The only functions GCC can warn about are those that don’t need the
attributes in the first place. The way any warning would work is to detect
whether it is pure/const, and then see how the user marked it. So anything
it can properly detect as right or wrong didn’t need an attribute to begin
with - the compiler could already tell if it was pure/const


My knowledge about how GCC (or other compilers) works, is very limited, but If the function is implemented in another
  * translation unit
  * library
  * pre-compiled library
  * pre-compiled library created by another compiler
does GCC know it can avoid calling it multiple times?


Whole-program-optimization might help in some of those cases (I admit I have no idea; can the linker remove multiple function calls and replace them with a variable?), but depending on the project size it might add up a lot in term of compile-times.
So even for simple functions, where GCC can clearly determine its purity, it can be useful adding the attribute.


And even assuming that whole-program-optimization helps in most of those cases (which do not depend on the complexity or length of a function) how does someone know if adding those attributes to a function that is pure makes sense or not?

Adding pure to `inline int answer_of_life(){return 42;}` might not make any difference (both for programmers and compiler, because of it's simplicity and because inline), but where should the line be drawn?

Should I mark my functions (with something else as you are suggesting too it might do more harm than good), add for all those dummy tests, and check in the generated assembly if GCC recognizes them as pure and elides the second call?
There must be surely be a better way, but I currently know no other.


> Rather than tell the user they got it wrong, you might as well tell the
user to remove the attribute because it isn’t necessary and won’t be
necessary.

No, removing it as unnecessary would be wrong.
Then you cannot tell anymore the difference between functions that are pure by accident and by design.
And you cannot prevent anymore a pure-function to getting nonpure, except by reading the code.
It is useful for programmers (yes, even they look at the code), even for those function where GCC does not need the attribute.

> Giving a bunch of really contrived examples where users may update things
wrong doesn’t seem like a good motivation to make a warning that can only
possibly have a really high false positive rate.

Just adding a "printf" statement for debugging, or increasing/decreasing a global counter invalidates the pure attributes.
Thus by trying to understand/analyze a bug, another is added.


> It is a tool for experts.

And I see no harm in making it more developer-friendly.
Why would that be a bad idea? As you claimed previously.

Because it is difficult to implement?
I do not know if it is, but that would not make it a bad idea.

Because of false positives?
Developers can handle them, case-by-case by documenting and disabling (or ignoring) the diagnostic, or globally by not turning the diagnostic on.
Just like any other diagnostic.

Because it adds nothing from a compiler perspective?
I'm still not convinced that it has no added value, especially when interacting with "extern" code/libraries.

But it definitively has some value for developers.
It's part of the API of a function, just like declaring the member function of a class const (or the parameter of a function).
Adding const might even avoid some optimization, and leads to code-duplication when one needs overloads (like for operator[] in container-like classes), but from a developer perspective it's great. It helps to catch errors.
Of course one could never use it, for the compiler it would be the same.
And it would not invalidate it's original use-case, thus it would still be possible to use those attributes like today if someone wants to, they would not even need to change a thing.
Comment 31 Martin Sebor 2022-01-26 17:04:43 UTC
I'm not working on this anymore.
Comment 32 Andrew Pinski 2023-05-09 18:07:00 UTC
*** Bug 109792 has been marked as a duplicate of this bug. ***