GCC 4.7 has a warning about freeing non-heap objects that is way too eager. Compiling the following code in C or C++: ====== #include <stdlib.h> typedef struct Data { int refcount; } Data; extern const Data shared_null; Data *allocate() { return (Data *)(&shared_null); } void dispose(Data *d) { if (d->refcount == 0) free(d); } void f() { Data *d = allocate(); dispose(d); } ==== Produces the following warning: test.c: In function 'f' test.c:17:13: warning: attempt to free a non-heap object 'shared_null' [-Wfree-nonheap-object] The warning is overeager because it says "attempt to free" without indicating that it's only a possibility. GCC cannot prove that the call to free() will happen with that particular pointer, as the value of shared_null.refcount is not known. The warning should either: a) be modified to indicate it's only a possibility and the compiler can't prove it; b) be issued only when the compiler is sure that the free will happen on non-heap objects. Or both, by having two warnings: one for when it's sure and one for when it isn't.
Confirmed. This way it becomes an always may-be warning because a function may not actually be executed (apart from main()). Almost all warnings behave this way btw. GCC is in this case seeing free (&shared_null); and considers this a good thing to warn on. So would you consider printing warning: possible attempt to free a non-heap object 'shared_null' a fix? Unconditionally, as we really cannot prove a line of code _will_ be executed at runtime (we can replace the free by an abort call though ...).
To be honest, I don't want false-positive warnings. The code and data are constructed so that it never frees the non-heap object (it has a reference count of -1). If the driver to this warning can't be improved to be certain, I'd recommend at least changing the text, like the -Wuninitialized one: 'varname' may be used uninitialized in this function When GCC warnings are assertive, like the "will break strict aliasing" one, we go an extra mile to try and fix them.
(In reply to comment #2) > To be honest, I don't want false-positive warnings. The code and data are > constructed so that it never frees the non-heap object (it has a reference > count of -1). If the driver to this warning can't be improved to be certain, > I'd recommend at least changing the text, like the -Wuninitialized one: > > 'varname' may be used uninitialized in this function > > When GCC warnings are assertive, like the "will break strict aliasing" one, we > go an extra mile to try and fix them. Note that even for the uninitialized use case we warn for functions that may be never executed at runtime. So - are you happy with the definitive warning if the free () call happens unconditionally when the function is entered?
(In reply to comment #3) > Note that even for the uninitialized use case we warn for functions > that may be never executed at runtime. So - are you happy with the > definitive warning if the free () call happens unconditionally when > the function is entered? I'm not sure I follow your reasoning. Please bear with me. If GCC can prove that the function will be called with a non-heap object, print the warning, even if the function in question never gets executed. That is, after inlining, code like: extern Data shared_null; void dispose() { free(&shared_null); } *should* print the warning, regardless of whether dispose() ever gets run. My point was that the code that GCC was seeing, after inlining, was: void f() { if (shared_null.refcount == 0) free(&shared_null); } In which case, the call to free() isn't unconditional. In this case, the warning should either be suppressed, or indicate that it's only a possibility instead of being assertive.
Even the printf format warnings are this way too ....
ping. If you can't fix GCC so that it can prove that the free is on a non-heap object, then please change the warning to indicate that GCC may be wrong. For example: warning: free() may be called with non-heap object 'name'
All late warnings are susceptible to the same problem. The only way to tell from the IL that the free call isn't unconditional is from the CFG. While that could be used to issue either a "maybe" kind of a warning or a definitive one per function, it would just turn most instances of these warnings into the "maybe" kind. Which is how all warnings need to be viewed regardless. What might help more than rewording every warning to say "this may be undefined" is printing the chain of conditions that need to be satisfied in order for the warning to trigger. The infrastructure to do this is all there, it's just a matter of taking advantage of it: when a warning is issued, walk the shortest path from the entry to the basic block with the problem statement and print each conditional along the way. With that, we should end up with output very close to that of the analyzer: pr54202.c:17:9: warning: ‘free’ of ‘&shared_null’ which points to memory not on the heap [CWE-590] [-Wanalyzer-free-of-non-heap] 17 | free(d); | ^~~~~~~ ‘f’: events 1-3 | | 16 | if (d->refcount == 0) | | ^ | | | | | (1) following ‘true’ branch... | 17 | free(d); | | ~~~~~~~ | | | | | (2) ...to here | | (3) call to ‘free’ here | Removing basic block 5 f () { int _2; <bb 2> [local count: 1073741824]: _2 = shared_null.refcount; if (_2 == 0) <<< condition goto <bb 3>; [33.00%] else goto <bb 4>; [67.00%] <bb 3> [local count: 354334800]: free (&shared_null); [tail call] <<< conditional invalid call <bb 4> [local count: 1073741824]: return; }
The original testcase by Thiago still fails with GCC 11. Unfortunately, GCC 11 decided to turn -Wfree-nonheap-object on by default (!), resulting in false positives for Qt users (containers in Qt use this pattern). For instance https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qlinkedlist.h?h=5.15 struct QT_DEPRECATED_VERSION_5_15 QLinkedListData { QLinkedListData *n, *p; QtPrivate::RefCount ref; // set to -1 for shared_null int size; uint sharable : 1; Q_CORE_EXPORT static const QLinkedListData shared_null; }; template <typename T> inline QLinkedList<T>::~QLinkedList() { if (!d->ref.deref()) freeData(d); // never called on shared_null because of the check } template <typename T> void QLinkedList<T>::freeData(QLinkedListData *x) { // ... delete x; // -Wfree-nonheap-object here }
A simpler example not involving any globals, causing Wfree-nonheap-object warning since GCC11 https://godbolt.org/z/MYn1zrjE4 === struct Foo { void allocate(int n); void deallocate(); int *_data; }; void Foo::allocate(int n) { _data = new int[n] - 1; } void Foo::deallocate() { delete[] (_data+1); }
(In reply to Serdar Sanli from comment #9) > A simpler example not involving any globals, causing Wfree-nonheap-object > warning since GCC11 This is actually a bug in the example: it's invalid to decrement a pointer to the beginning of an object as done in Foo::allocate.
Here is another example (no external includes/etc.): typedef long unsigned int size_t; extern void *malloc (size_t __size) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__malloc__)) __attribute__ ((__alloc_size__ (1))) __attribute__ ((__warn_unused_result__)); extern void free (void *__ptr) __attribute__ ((__nothrow__ , __leaf__)); typedef struct { long count, flags; } Foo; Foo *G(Foo *x, long flags, int unused) { if (x) { // originally a file descriptor x->count = 0; flags = 0; } else { if (!(x = (Foo *)malloc(sizeof *x))) return (Foo *)0; flags = 1; } x->flags = flags; return x; } Foo *F(Foo *x, long flags) { if (x) { // originally a pathname path x->count = 0; flags = 0; } else { if (!(x = (Foo *)malloc(sizeof *x))) return (Foo *)0; flags = 1; } x->flags = flags; return G(x, flags, 123); } void release(Foo *x) { if (x && x->flags) free(x); } int main(void) { Foo x; if (F(&x, 0)) release(&x); return 0; } In the above, variations due to compiler optimization levels make it even more confusing: -O1 does not warn, -O2 does warn, -O3 again does not warn (on both gcc-12.3 and gcc-13.2 on Gentoo Linux, anyway). O3 optimizes the whole main away. clang never warns on this at any -O level, nor with clang --analyze. What is particularly bad about this warning is that AFAICT there is no way to massage the code to reliably silence it while also preserving the intended effect. Now it's even on by default. This actively discourages an otherwise clean arrangement in the C programming language when working with anyone else who takes warns too seriously. Any such warning should have "may be" language not language that sounds like the compiler has "proved an actuality". Might the same argument apply to many warnings? Maybe. Two words is not so bad, though. Other warns may have workarounds like annotations to silence them reliably without sacrificing functionality. Maybe I'm missing one here?