Bug 93644 - [10/11/12 Regression] spurious -Wreturn-local-addr with PHI of PHI
Summary: [10/11/12 Regression] spurious -Wreturn-local-addr with PHI of PHI
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 10.0
: P2 normal
Target Milestone: 11.3
Assignee: Jeffrey A. Law
URL:
Keywords: diagnostic
: 95044 (view as bug list)
Depends on:
Blocks: Wreturn-local-addr
  Show dependency treegraph
 
Reported: 2020-02-09 20:42 UTC by jim
Modified: 2021-07-28 07:04 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-02-09 00:00:00


Attachments
derived from gnulib's lib/careadlinkat.c (693 bytes, text/plain)
2020-02-09 20:42 UTC, jim
Details
another instance of a -Wreturn-local-addr false alarm (408 bytes, text/plain)
2020-12-17 01:59 UTC, eggert
Details
another minimal example to demonstrate the false alarm (405 bytes, text/plain)
2021-05-03 14:27 UTC, Jochen Roemmler
Details

Note You need to log in before you can comment on or make changes to this bug.
Description jim 2020-02-09 20:42:42 UTC
Created attachment 47808 [details]
derived from gnulib's lib/careadlinkat.c

Using -Wreturn-local-addr can now evoke this false positive:

$ gcc -c -O2 -Wreturn-local-addr k.c
k.c: In function 'careadlinkat':
cc1: warning: function may return address of local variable [-Wreturn-local-addr]
k.c:24:8: note: declared here
   24 |   char stack_buf[1024];
      |        ^~~~~~~~~

I was able to easily narrow down that this behavior changed on gcc/git master built from:
2019-07-19 (no warning) to
2019-07-23 (warning emitted).
--

I was surprised not to be able to convince the compiler that the code is ok OR even to suppress the warning.
Per discussion in the thread at https://lists.gnu.org/r/coreutils/2020-02/msg00006.html, I first tried this:

>> Would an `assure (buf != stack_buf)` before the `return buf`
>> indicate that constraint to gcc with minimal runtime overhead?

I then tried to suppress that warning in the affected file by adding these lines:

/* Without this pragma, gcc 10.0.1 20200205 reports that
   the "function may return address of local variable".  */
# pragma GCC diagnostic ignored "-Wreturn-local-addr"

But, surprisingly, even with that pragma, the warning was still emitted.
I ended up having to disable this normally-high-signal warning for all of coreutils/lib.

The attached file is derived from gnulib's lib/careadlinkat.c.
Comment 1 Martin Sebor 2020-02-09 21:50:19 UTC
The warning is issued because a PHI operand of the the return statement references another PHI node one of whose operands is yet another PHI node that includes the local variable stack_buf among its operands:

  <bb 6> [local count: 79230146]:
  # buffer_17 = PHI <&stack_buf(4), buffer_38(D)(5)>
  ...
  <bb 7> [local count: 1073741824]:
  # buf_20 = PHI <buffer_17(6), buf_57(25)>
  ...
  <bb 29> [local count: 79230145]:
  # _25 = PHI <0B(10), buf_20(15), 0B(24), 0B(28), b_63(14), b_61(17), 0B(9), buf_20(16), buf_20(18)>
  stack_buf ={v} {CLOBBER};
  return _25;

A simplified test case is below.  It warns for the same reason (but with fewer indirections):

  # buffer_2 = PHI <&stack_buf(2), buffer_7(D)(3)>
  ...
  # _4 = PHI <0B(4), _11(7), 0B(5), buffer_2(6)>
  return _4;


There's no way to tell from the IL in either of these test cases that one of the final PHI operands cannot point to stack_buf so the warning triggers.  Short of not considering PHI arguments that are themselves PHIs  and trading off false positives for false negatives I don't see how to avoid the warning in this case.


$ cat pr93644.c && gcc -O2 -S -Wall -fdump-tree-isolate-paths=/dev/stdout pr93644.c
typedef __SIZE_TYPE__ size_t;

char *
careadlinkat (char *buffer, size_t buffer_size)
{
  char stack_buf[1024];
  if (!buffer_size)
    {
      buffer = stack_buf;
      buffer_size = sizeof stack_buf;
    }

  char *buf = buffer;
  size_t buf_size = buffer_size;

  extern int link_length;
  if (link_length < 0)
    return 0;

  size_t link_size = link_length;
  if (link_size < buf_size)
    {
      if (buf == stack_buf)
	return __builtin_malloc (link_size);

      return buf;
    }

  return 0;
}

;; Function careadlinkat (careadlinkat, funcdef_no=0, decl_uid=1932, cgraph_uid=1, symbol_order=0)

pr93644.c: In function ‘careadlinkat’:
cc1: warning: function may return address of local variable [-Wreturn-local-addr]
pr93644.c:6:8: note: declared here
    6 |   char stack_buf[1024];
      |        ^~~~~~~~~
careadlinkat (char * buffer, size_t buffer_size)
{
  size_t link_size;
  char stack_buf[1024];
  int link_length.0_1;
  char * _4;
  char * _11;

  <bb 2> [local count: 1073741824]:
  if (buffer_size_6(D) == 0)
    goto <bb 4>; [50.00%]
  else
    goto <bb 3>; [50.00%]

  <bb 3> [local count: 536870912]:

  <bb 4> [local count: 1073741824]:
  # buffer_2 = PHI <&stack_buf(2), buffer_7(D)(3)>
  # buffer_size_3 = PHI <1024(2), buffer_size_6(D)(3)>
  link_length.0_1 = link_length;
  if (link_length.0_1 < 0)
    goto <bb 8>; [12.76%]
  else
    goto <bb 5>; [87.24%]

  <bb 5> [local count: 936732369]:
  link_size_9 = (size_t) link_length.0_1;
  if (buffer_size_3 > link_size_9)
    goto <bb 6>; [71.00%]
  else
    goto <bb 8>; [29.00%]

  <bb 6> [local count: 665079983]:
  if (&stack_buf == buffer_2)
    goto <bb 7>; [17.43%]
  else
    goto <bb 8>; [82.57%]

  <bb 7> [local count: 115923441]:
  _11 = __builtin_malloc (link_size_9);

  <bb 8> [local count: 1073741824]:
  # _4 = PHI <0B(4), _11(7), 0B(5), buffer_2(6)>
  stack_buf ={v} {CLOBBER};
  return _4;

}
Comment 2 Marc Glisse 2020-02-09 22:08:16 UTC
  # buffer_2 = PHI <&stack_bufD.1939(3), buffer_7(D)(9)>

  buffer_18 = ASSERT_EXPR <buffer_2, buffer_2 != &stack_bufD.1939>;

Can't we deduce from this

  buffer_18 = buffer_7(D)

? Of course that's not a general solution, but it looks like a sensible optimization, and in the reduced testcase it should remove &stack_buf from the possible return values.
Comment 3 Martin Sebor 2020-02-09 23:05:29 UTC
The most likely reason the warning isn't suppressed by #pragma GCC diagnostic is that the statement it's issued for has no source line number associated with it.  That's also why the warning doesn't point to such a statement (only the note does).  The details of this bug are in pr90735.
Comment 4 Martin Sebor 2020-02-09 23:09:09 UTC
pr90735 suggests using the location of the closing curly brace of the function instead.  Another alternative might be to use the location associated with the note.
Comment 5 Jeffrey A. Law 2020-02-10 22:22:24 UTC
WRT c#2, yes it's a reasonable deduction, but the warning and the point where we have the asserts are in two different passes.  ie, the info isn't really available when we need it.



What we might be able to do is see the assert:

  buffer_16 = ASSERT_EXPR <buffer_3, buffer_3 != &stack_buf>;

Then look at the DEF point for buffer_3:

  # buffer_3 = PHI <&stack_buf(3), buffer_8(D)(9)>

Which means at the point of the assertion that buffer_16 can only have the value buffer_8 and replace the assert with

buffer_16 = buffer_8

That would in turn allow the use of buffer_16 in the problematic PHI to be replaced with buffer_8 and avoid the problem

We have to be careful and ensure that doesn't break the SSA form (it's safe in this example because buffer_8 is the default def).
Comment 6 Jeffrey A. Law 2020-02-10 22:31:03 UTC
I think we might be able to do this in remove_range_assertions
Comment 7 Jeffrey A. Law 2020-02-19 19:07:55 UTC
So optimizing things in remove_range_assertions works for the reduced testcase, but not for the original testcase.  There's a couple of deeper issues that have to be figured out for the original testcase. 

The trick we were trying to utilize is if we could safely copy-propagate a value which we knew not to be a stack address for an object which might have been a stack address, then we avoid the false positive.  This has a nice property that it likely improves optimization.

But it's insufficient for the full testcase.  First, loops get in the way. 

We might have something like this at the top of a loop:


x = phi (a, b)
b = <something>


If we have an subsequent assert that x != b, we can't then replace uses of x with a.  Why?  Because the assert refers to the value of b on the current iteration while the PHI refers to the value of b on the previous iteration.  This issue doesn't show up in the reduced testcase, but does in the full testcase.

Second, we have to deal with the cascading nature of the asserts.  If we go back to the original testcase the PHI in question looks like this just before removal of the ASSERT_EXPRs in VRP1:

  # _25 = PHI <0B(10), buf_79(16), 0B(26), 0B(30), b_94(15), b_92(19), 0B(9), buf_80(17), buf_80(39)>

Just looking at chain for buf_79 we have:

  # buffer_17 = PHI <&stack_buf(5), buffer_38(D)(34)>

  # buf_20 = PHI <buffer_17(6), buf_90(32)>

  # buf_29 = PHI <buf_20(37)>

  buf_79 = ASSERT_EXPR <buf_29, buf_29 != &stack_buf>;


We can see that buf_79 has 3 possible values:

&stack_buf, buffer_38 and buf_90 (buf_90 is loop carried)

The ASSERT only excludes &stack_buf so we can't copy propagate buffer_38 or buf_90 for the uses of buf_79.  And we (of course) lose the knowledge that buf_79 can't be &stack_buf when we drop the ASSERT_EXPRs.


We may still want to use the trick Marc noted in c#2.  It applies something like 20k times during a bootstrap and fixes Wreturn-local-addr-9 in the testsuite.  But it doesn't seem appropriate for stage4.

I find myself wondering if we could tackle this on the alias analysis side by looking at the PTA solution and see if any of the objects point into the stack.  I also wonder if PTA would benefit from the VRP analysis which excluded the stack object for certain addresses.  All blue sky stuff though.
Comment 8 Martin Sebor 2020-05-11 14:28:01 UTC
*** Bug 95044 has been marked as a duplicate of this bug. ***
Comment 9 Bruno Haible 2020-08-17 21:27:46 UTC
We now have a generic workaround to this bug:

If the bug occurs in a function foo:
1. Rename foo to foo_internal, mark it as '__attribute__ ((__noinline__)) static' and add a 'char stack_buf[1024]' parameter.
2. In the function foo_internal, drop the stack-allocated buffer and use the new parameter instead.
3. Create a new function foo, with the same signature as before, that merely allocates a 'char stack_buf[1024]' on the stack and passes it to foo_internal.

For an example, see https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=2a3468c9f263596815a3383c0157ba9a81cf2d24
Comment 10 eggert 2020-11-26 22:01:40 UTC
The generic workaround that Bruno describes ran into problems in Gnulib, as it's enabled only when compiled with -DGCC_LINT, and some users don't compile it that way. So we now have a more elaborate workaround:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=5a8a1598e1243599feb302f0f75d947553f2918f

that causes GCC to issue warnings like the following when the file is not compiled with -DGCC_LINT:

careadlinkat.c:58:4: warning: #warning "GCC might issue a bogus -Wreturn-local-addr warning here." [-Wcpp]
   58 | #  warning "GCC might issue a bogus -Wreturn-local-addr warning here."
      |    ^~~~~~~
careadlinkat.c:59:4: warning: #warning "See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644>." [-Wcpp]
   59 | #  warning "See <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644>."
      |    ^~~~~~~
careadlinkat.c: In function ‘careadlinkat’:
careadlinkat.c:193:10: warning: function may return address of local variable [-Wreturn-local-addr]
  193 |   return readlink_stk (fd, filename, buffer, buffer_size, alloc,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  194 |                        preadlinkat, stack_buf);
      |                        ~~~~~~~~~~~~~~~~~~~~~~~
careadlinkat.c:192:8: note: declared here
  192 |   char stack_buf[STACK_BUF_SIZE];
      |        ^~~~~~~~~


but obviously this is awkward and it would be better if the bug were fixed.
Comment 11 eggert 2020-12-17 01:59:42 UTC
Created attachment 49783 [details]
another instance of a -Wreturn-local-addr false alarm

I ran into a different instance of the bug today, while working on another Gnulib source file lib/canonicalize.c. A stripped-down test case attached. To reproduce the problem:

$ gcc -O2 -S return-local-addr.i 
return-local-addr.i: In function ‘canonicalize_filename_mode’:
cc1: warning: function may return address of local variable [-Wreturn-local-addr]
return-local-addr.i:28:25: note: declared here
   28 |   struct scratch_buffer rname_buffer;
      |                         ^~~~~~~~~~~~

This is with GCC 10.2.1 20201125 (Red Hat 10.2.1-9) on x86-64.
Comment 12 eggert 2020-12-17 02:05:24 UTC
There are really two bugs here:

(A) GCC emits the false alarm.

(B) there's no way to shut off the false alarm, not even with '# pragma GCC diagnostic ignored "-Wreturn-local-addr"'.

Although this bug report's replies have been about (A), even fixing (B) would be a real help with Gnulib.

Should I file a separate bug report for (B)? I assume (B)'s easier to fix.
Comment 13 Jeffrey Walton 2020-12-17 05:52:06 UTC
On Wed, Dec 16, 2020 at 9:05 PM eggert at cs dot ucla.edu
<gcc-bugzilla@gcc.gnu.org> wrote:
> ...
> (B) there's no way to shut off the false alarm, not even with '# pragma GCC
> diagnostic ignored "-Wreturn-local-addr"'.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53431
Comment 14 Martin Sebor 2021-02-12 00:55:09 UTC
I looks like this might be another opportunity to use the predicate analysis from tree-ssa-uninit.c (once it's generalized).
Comment 15 Jakub Jelinek 2021-04-27 11:38:37 UTC
GCC 11.1 has been released, retargeting bugs to GCC 11.2.
Comment 16 Jochen Roemmler 2021-05-03 14:27:58 UTC
Created attachment 50739 [details]
another minimal example to demonstrate the false alarm

This is another instance of the test case. A senseless, yet perfectly valid piece of C code. It creates the false positive warning if compiled like this:

gcc -Wreturn-local-addr -O1 -fisolate-erroneous-paths-dereference -c pr93644_2.c

pr93644_2.c: In function ‘buildVname’:
pr93644_2.c:28:12: warning: function may return address of local variable [-Wreturn-local-addr]
   28 |     return vname;
      |            ^~~~~
pr93644_2.c:18:17: note: declared here
   18 |     char        buf[256];
      |                 ^~~


I'm using gcc version 11.1.1 20210428 (Red Hat 11.1.1-1) (GCC) 
However, I can silence the warning using

# pragma GCC diagnostic ignored "-Wreturn-local-addr"
Comment 17 Richard Biener 2021-07-28 07:04:41 UTC
GCC 11.2 is being released, retargeting bugs to GCC 11.3