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.

Hi,

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.

Cheers!

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)

Created attachment 50010 [details]
C++ file generated by Bison run with -E

Including all dependencies, created by GCC 11 with added -E

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.

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

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.

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.

Cheers!

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.

Cheers

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);
  #endif
    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)

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]

Created attachment 50080 [details]
Reproducer test.cxx

Created attachment 50081 [details]
Reproducer test-simplified.cxx

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.

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%]
  else
    goto <bb 15>; [11.00%]

  <bb 4> [count: 0]:
<L5>:
  _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%]
  else
    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 ();