Bug 98753 - -Wfree-nonheap-object on unreachable code with -O0
Summary: -Wfree-nonheap-object on unreachable code with -O0
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
Keywords: diagnostic
Depends on:
Blocks: Wfree-nonheap-object
  Show dependency treegraph
Reported: 2021-01-19 20:18 UTC by Christoph
Modified: 2022-11-03 22:38 UTC (History)
11 users (show)

See Also:
Build: 11.0.0 20210114 (experimental) [revision 08a4adcf2b6ded2fea97195c715757df61a23395] (SUSE Linux)
Known to work:
Known to fail: 11.0
Last reconfirmed: 2021-01-19 00:00:00

C++ file generated by Bison (15.31 KB, text/plain)
2021-01-19 20:18 UTC, Christoph
C++ file generated by Bison run with -E (121.70 KB, text/plain)
2021-01-20 07:40 UTC, Christoph
Reproducer test.cxx (7.80 KB, text/x-csrc)
2021-01-29 11:30 UTC, Boris Kolpackov
Reproducer test-simplified.cxx (1.02 KB, text/x-csrc)
2021-01-29 11:30 UTC, Boris Kolpackov

Note You need to log in before you can comment on or make changes to this bug.
Description Christoph 2021-01-19 20:18:46 UTC
Created attachment 50005 [details]
C++ file generated by Bison

While compiling CMake with GCC 11.0, I spot some -Wfree-nonheap-object warnings. These came from files generated by Bison. I regenerated the C++ files with the latest GNU Bison release and the problem persisted. So I asked on the Bison user list and one of the developers claims, that Bison is right.

I don't know whether it is a false positive or whether Bison has a bug.

Attached is the C++ file generated by Bison (cmCommandArgumentParser.cxx).

The warning I get from G++:

[ 54%] Building CXX object Source/CMakeFiles/CMakeLib.dir/LexerParser/cmCommandArgumentParser.cxx.o
cmCommandArgumentParser.cxx: In function ‘int cmCommandArgument_yyparse(yyscan_t)’:
cmCommandArgumentParser.cxx:1838:18: warning: ‘void free(void*)’ called on unallocated object ‘yyssa’ [-Wfree-nonheap-object]
cmCommandArgumentParser.cxx:1203:16: note: declared here

The explanation / reduced code from one of the Bison developers, why the code is correct:

> GCC 11 is wrong here.  The full story (taken from your file) is:
>> // Create.
>>     yy_state_t yyssa[YYINITDEPTH];
>>     yy_state_t *yyss = yyssa;
>> [...]
>> // Grow.
>>       yystacksize *= 2;
>>      {
>>         yy_state_t *yyss1 = yyss;
>>         union yyalloc *yyptr =
>>           YY_CAST (union yyalloc *,
>> (yystacksize))));
>>         if (! yyptr)
>>           goto yyexhaustedlab;
>>         YYSTACK_RELOCATE (yyss_alloc, yyss);
>>         if (yyss1 != yyssa)
>>           YYSTACK_FREE (yyss1);
>>       }
>> [...]
>> // Clean up
>>   if (yyss != yyssa)
>>     YYSTACK_FREE (yyss);
> Or, in words, we use a stack-allocated stack until it's too small and
> then we use a heap-allocated stack, and in that case, and that case
> only, we free it at the end.  Said another way: we never ever call
> free(yyssa).
> So GCC's warning is a false positive.

Source: https://lists.gnu.org/archive/html/help-bison/2021-01/msg00021.html

My GCC version is:

> gcc-11 -v
Using built-in specs.
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 --enable-languages=c,c++,objc,fortran,obj-c++,ada,go,d --enable-offload-targets=nvptx-none,amdgcn-amdhsa, --without-cuda-driver --enable-checking=release --disable-werror --with-gxx-include-dir=/usr/include/c++/11 --enable-ssp --disable-libssp --disable-libvtv --enable-cet=auto --disable-libcc1 --disable-plugin --with-bugurl=https://bugs.opensuse.org/ --with-pkgversion='SUSE Linux' --with-slibdir=/lib64 --with-system-zlib --enable-libstdcxx-allocator=new --disable-libstdcxx-pch --enable-libphobos --enable-version-specific-runtime-libs --with-gcc-major-version-only --enable-linker-build-id --enable-linux-futex --enable-gnu-indirect-function --program-suffix=-11 --without-system-libunwind --enable-multilib --with-arch-32=x86-64 --with-tune=generic --build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 11.0.0 20210114 (experimental) [revision 08a4adcf2b6ded2fea97195c715757df61a23395] (SUSE Linux)
Comment 1 Martin Sebor 2021-01-19 20:55:37 UTC
The attached file depends on a number of headers.  To reproduce a problem we need a standalone test case that can be compiled on its own.  One way to do that is to use the -E option to GCC to obtain the translation unit and attaching that.

That said, based on the description I suspect the warning is a false positive with the same root cause as pr54202.  Until a better solution is available in GCC the workaround is to suppress the warning, either by #pragma GCC diagnostic, or by using -Wno-free-nonheap-object on the command line.
Comment 2 Akim Demaille 2021-01-20 05:22:09 UTC

I can't tell if it's the same bug of not, but there have never complaints about this piece of code before GCC11, so something has most likely changed recently.  Note also that the 'free' is under an 'if' that clearly makes the diagnostic wrong (I mean, there's no need to solve the halting problem here, all the information is statically available).

Besides, I would like to stress something that was said in the other PR (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202#c6):

> 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'

Thiago Macieira is right: the warning claims "there is an error", but it only *suspects* that there is one.  The wording is misleading and should be fixed.

Comment 3 Richard Biener 2021-01-20 07:31:49 UTC
The issue is that we isolate a path that is impossible to take but on that
path we have p = &foo; free (p); and thus a "proved" mistake.  But in
reality it is guarded by an effective if (false) condition.  So it's not as
simple as you think.  (we also emit diagnostics on function bodies we do not
know are actually never called)
Comment 4 Christoph 2021-01-20 07:40:08 UTC
Created attachment 50010 [details]
C++ file generated by Bison run with -E

Including all dependencies, created by GCC 11 with added -E
Comment 5 Martin Sebor 2021-01-20 16:48:41 UTC
I can't reproduce the warning with the default options.  There are just two calls to free() in the dump.  In each instance its argument resolves to the yymsg pointer and not to the yyssa array as in the warning message in comment #0.  We would also need to see the command line options you use to compile the file (please review https://gcc.gnu.org/bugs/#need for the full details we ask for).

In GCC 11, -Wfree-nonheap-object was enhanced to validate every argument to every deallocation function.  Prior to GCC 11 it only considered a negligible subset of arguments (basically just straight addresses of variables).  The warning was prone to false positives then (as is evident from pr54202), and the enhancement hasn't changed that.

Different optimization options produce different intermediate representation.  Some result in constants substituted for what would otherwise be variables.  When a constant is substituted into an expression that it's not valid for it might trigger a warning because in the IL it's indistinguishable from a bug in the original source code.  There's nothing a warning designed to detect such invalid expressions can do about it.  Changing this message alone to say "free() may be called with non-heap object" wouldn't be appropriate without also changing all the other messages that are subject to the same problem (all flow-sensitive warnings are).

At least two solutions are theoretically possible: a) make the warning "smarter" than the optimization it depends on that does the substitution, and have it figure out that the invalid code was synthesized by it, doesn't occur in the source code, and cannot be reached in the program given the preconditions, or b) make the optimizations "smarter" either by not substituting constants into contexts where they're invalid, or by figuring out that these invalid expressions cannot be reached based on their preconditions.  The two sets of preconditions need not be the same.  Both approaches are worth exploring but both are hard and neither will ever be perfect.  Which is partly why GCC documents that "Warnings are diagnostic messages that report constructions that are not inherently erroneous but that are risky or suggest there may have been an error."  If the warning gets it wrong #pragma GCC diagnostic can be used to avoid the false positive.
Comment 6 Christoph 2021-01-20 17:00:46 UTC
Sorry for not providing the command line argument and the new output.

> gcc-11 -std=c++17 cmCommandArgumentParser_complete.cxx 
cmCommandArgumentParser.cxx: In function ‘int cmCommandArgument_yyparse(yyscan_t)’:
cmCommandArgumentParser.cxx:1838:10: warning: ‘void free(void*)’ called on unallocated object ‘yyssa’ [-Wfree-nonheap-object]
cmCommandArgumentParser.cxx:1203:16: note: declared here
/usr/lib64/gcc/x86_64-suse-linux/11/../../../../x86_64-suse-linux/bin/ld: /usr/lib64/gcc/x86_64-suse-linux/11/../../../../lib64/crt1.o: in function `_start':
/home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:110: undefined reference to `main'
Comment 7 Martin Sebor 2021-01-20 17:34:12 UTC
I can reproduce the warning with no optimization, thanks.  At -O0, there are three calls to free in the IL when the warning runs.  The second one that triggers it

  # .MEM_218 = VDEF <.MEM_216>
  free (yymsg.17_83);

is eliminated by copy propagation at -O1.  It's not eliminated at -O0 because the optimization isn't done then.  So the ultimate root cause of the problem is the same as in pr54202, except at -O0 (the test case there depends on inlining).

It would be possible to disable the warning at -O0 to avoid this false positive at the expense of some false negatives.  I'm not sure that would be a good solution based on a single report.  If more bugs like this are reported we might reconsider.  Let me keep this bug open until then.
Comment 8 Akim Demaille 2021-01-20 17:34:54 UTC
Hi Richard,

(In reply to Richard Biener from comment #3)
> The issue is that we isolate a path that is impossible to take but on that
> path we have p = &foo; free (p); and thus a "proved" mistake.  But in
> reality it is guarded by an effective if (false) condition.  So it's not as
> simple as you think.

Point taken, thanks (though I'm not sure I understand why it would explore branches below an 'if (false)', but I definitely don't know the details).

> (we also emit diagnostics on function bodies we do not
> know are actually never called)

I will not dispute that this is a hard job (I was just pointing that it should not impossible in that precise case).  But it just emphasizes again that the *wording* is wrong.

> warning: ‘void free(void*)’ called on unallocated object ‘yyssa’

This is plain false.  Free is provably *never* called with yyssa.  The wording should stop being so affirmative, so that the users don't get the wrong impression.

Comment 9 Akim Demaille 2021-01-20 17:40:16 UTC
Hi Martin,

Thanks for the detailed explanation.

(In reply to Martin Sebor from comment #5)
> Changing this message alone to say "free() may be called with non-heap
> object" wouldn't be appropriate without also changing all the other messages
> that are subject to the same problem (all flow-sensitive warnings are).

And I definitely believe this should be done.  All the messages
that claim "you _have_ this problem" should be reworded to
"you _may have_ this problem"–unless of course that the problem
can be proven to exist.

But I agree this is beyond the scope of this issue.

Comment 10 Sergei Trofimovich 2021-01-26 19:37:53 UTC
Smaller example built from a similar code to cmake:

  // gcc-11.0.0 -c bug.c
  // `void free(void*)` called on unallocated object `b` [-Wfree-nonheap-object]
  #include <stdlib.h>

  int main(void) {
    char b[1234];
    char * p = b;
  #if 0 /* enabled in debug only */
    p = (char*)malloc(42);
    if (p != b)
        free (p);

$ LANG=C gcc-11.0.0 -c bug.c
bug.c: In function 'main':
bug.c:12:9: warning: 'free' called on unallocated object 'b' [-Wfree-nonheap-object]
   12 |         free (p);
      |         ^~~~~~~~
bug.c:6:10: note: declared here
    6 |     char b[1234];
      |          ^

gcc version 11.0.0 20210112 (experimental) (commit cfaaa6a1ca744c1a93fa08a3e7ab2a821383cac1)
Comment 11 Boris Kolpackov 2021-01-29 11:29:21 UTC
I believe I am encountering a similar false positive so I will attach the test cases to this bug (but let me know if I should instead attach them to 54202 or create a new bug). The attached test.cxx is a cleaned-up original code while test-simplified.cxx is its further simplified version (thanks to Matthew Krupcale for coming up with both):

g++ -O1 -c test.cxx 

In member function ‘void small_allocator<T, N, B>::deallocate(void*, std::size_t) [with T = f()::expr; long unsigned int N = 1; B = small_allocator_buffer<f()::expr, 1>]’,
    inlined from ‘std::vector<_Tp, _Alloc>::pointer std::vector<_Tp, _Alloc>::_M_allocate_and_copy(std::vector<_Tp, _Alloc>::size_type, _ForwardIterator, _ForwardIterator) [with _ForwardIterator = const f()::expr*; _Tp = f()::expr; _Alloc = small_allocator<f()::expr, 1, small_allocator_buffer<f()::expr, 1> >]’ at /home/boris/work/build2/tests/modules/gcc2/gcc-install/include/c++/11.0.0/bits/alloc_traits.h:341:23,
    inlined from ‘void std::vector<_Tp, _Alloc>::reserve(std::vector<_Tp, _Alloc>::size_type) [with _Tp = f()::expr; _Alloc = small_allocator<f()::expr, 1, small_allocator_buffer<f()::expr, 1> >]’ at /home/boris/work/build2/tests/modules/gcc2/gcc-install/include/c++/11.0.0/bits/vector.tcc:85:36,
    inlined from ‘void small_vector<T, N>::reserve(std::size_t) [with T = f()::expr; long unsigned int N = 1]’ at test.cxx:234:26,
    inlined from ‘small_vector<T, N>::small_vector() [with T = f()::expr; long unsigned int N = 1]’ at test.cxx:163:15,
    inlined from ‘void f()’ at test.cxx:1472:27:
test.cxx:121:27: warning: ‘void operator delete(void*)’ called on unallocated object ‘exprs’ [-Wfree-nonheap-object]
Comment 12 Boris Kolpackov 2021-01-29 11:30:28 UTC
Created attachment 50080 [details]
Reproducer test.cxx
Comment 13 Boris Kolpackov 2021-01-29 11:30:59 UTC
Created attachment 50081 [details]
Reproducer test-simplified.cxx
Comment 14 Boris Kolpackov 2021-01-29 11:36:29 UTC
If this cannot be fixed for 11 (and judging by the age of 54202 I feel it's not likely), perhaps it makes sense not to enable this warning by default? Now that C++ operator delete() is covered by this check (see bug 57111), it's reasonable to expect more false positives like these.
Comment 15 Martin Sebor 2021-01-29 16:48:13 UTC
The problem in attachment 50081 [details] is different from the one reported in comment #0.  The warning there is issued for the call to operator delete in the IL below as a result of the the test in bb 4 not being folded to false.  The reason for that is that the call to std::__uninitialized_copy_a.isra() is not inlined.  Forcing it to expand inline (e.g., by bumping up the -finline-limit= value) ends up with the desired result.   The inability to propagate constant values across out-of-line function calls is a general problem, not something specific to this warning (or any others), and as far as I know doing something about it isn't on anyone's radar.

What the IL does suggest though is that functions like __uninitialized_copy_a() might benefit from an annotation indicating that they have no effects unless the range delineated by their arguments is nonempty.  With that, the call below could be eliminated altogether even without inlining.  I'm not sure if it would be realistic to expect the IPA optimizer to do this without an annotation.

GCC doesn't have such an annotation yet so the only way to avoid the warning here is by changing the code or compiler options.  Using #pragma GCC diagnostic to suppress the warning should work.  An alternative might be to explore providing an inline specialization of the std::uninitialized_copy template for the small_vector::iterator (which would mean defining it as a user-defined type) that returns when the range is empty.  I didn't take the time to try that.

Yet another possibility is to change libstdc++ to either declare std::__uninitialized_copy_a() inline, or change it to an inline wrapper around an std::__uninitialized_copy_a_impl() that returns immediately when the range is empty or calls the _impl() otherwise.  Both seem to work.

  <bb 2> [local count: 1073741824]:
  sv_sv_ints ={v} {CLOBBER};
  MEM[(struct _Vector_impl *)&sv_sv_ints + 48B].D.19786.buf_ = &sv_sv_ints.D.20517;
  MEM[(struct _Vector_impl_data *)&sv_sv_ints + 56B] ={v} {CLOBBER};
  MEM[(struct _Vector_impl_data *)&sv_sv_ints + 56B]._M_start = 0B;
  MEM[(struct _Vector_impl_data *)&sv_sv_ints + 56B]._M_finish = 0B;
  MEM[(struct _Vector_impl_data *)&sv_sv_ints + 56B]._M_end_of_storage = 0B;
  MEM[(struct buffer_type *)&sv_sv_ints].free_ = 0;
  std::__uninitialized_copy_a.isra (0B, 0B, &MEM[(struct buffer_type *)&sv_sv_ints].data_);

  <bb 3> [local count: 354334802]:
  _29 = MEM[(struct vector *)&sv_sv_ints + 48B].D.20479._M_impl.D.19787._M_finish;
  _30 = MEM[(struct vector *)&sv_sv_ints + 48B].D.20479._M_impl.D.19787._M_start;
  if (_29 != _30)
    goto <bb 9>; [89.00%]
    goto <bb 15>; [11.00%]

  <bb 4> [count: 0]:
  _48 = __builtin_eh_pointer (11);
  __cxa_begin_catch (_48);
  _57 = MEM[(struct small_allocator *)&sv_sv_ints + 48B].buf_;
  if (&sv_sv_ints == _57)
    goto <bb 5>; [0.00%]
    goto <bb 6>; [0.00%]

  <bb 5> [count: 0]:
  MEM[(struct buffer_type *)&sv_sv_ints].free_ = 1;
  goto <bb 7>; [0.00%]

  <bb 6> [count: 0]:
  operator delete (&MEM[(struct buffer_type *)&sv_sv_ints].data_);

  <bb 7> [count: 0]:
  __cxa_rethrow ();