Bug 90905 - missing -Wreturn-local-addr returning a local std::string::c_str()
Summary: missing -Wreturn-local-addr returning a local std::string::c_str()
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wreturn-local-addr
  Show dependency treegraph
 
Reported: 2019-06-17 23:38 UTC by Martin Sebor
Modified: 2020-01-28 13:42 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-01-28 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2019-06-17 23:38:26 UTC
Compiling function f0 below on its own triggers a -Wreturn-local-addr for the return statement as one would expect.  But compiling the equivalent f1 fails to trigger the same warning.  Worse yet, compiling both functions in the samne translatin unit suppresses the warning for f0.

#include <string>

#if F0

const char s[] = "abc";

const char* f0 ()
{
  std::string str (s);
  return str.c_str ();
}

#endif

#if F1

const char *p = "def";

const char* f1 ()
{
  std::string str (p);
  return str.c_str ();
}

#endif
Comment 1 Martin Sebor 2019-06-17 23:58:41 UTC
An analogous test case with std::vector or other containers is, of course, not diagnosed at all because the warning only looks for locally declared variables and not pointers to allocated memory that has been freed.  That's a separate enhancement (pr90905).
Comment 2 Martin Sebor 2019-06-17 23:59:19 UTC
The separate enhancement is pr90906.
Comment 3 Marc Glisse 2019-06-18 06:59:28 UTC
> compiling both functions in the samne translatin unit suppresses the warning for f0.

It is quite common for extra code to change inlining decisions. You still get the warning at -O3.

> const char *p = "def";

Did you mean to write

const char * const p = "def";

(which does warn)? Otherwise, the compiler has no way to know this is a short string, so we end up with a function that returns either the address of a local variable or a pointer to deleted memory (although we don't have a nice PHI like that, even with -std=c++2a to disable extern templates, probably some suboptimal aliasing analysis).
Comment 4 Martin Sebor 2019-06-18 12:56:58 UTC
I meant for f1 not to see the string pointed to by p.  The warning usually triggers either for the "must return" and "may return" case but in f1 it's too complicated for it to see that the return pointer may be that to str._M_local_buf.  It might be possible to figure it out from the 'if (&str.D.19209._M_local_buf != _7)' below:

  <bb 9> [local count: 346356134]:
  # _23 = PHI <&str.D.19209._M_local_buf(8), _16(5)>
  __builtin_memcpy (_23, p.0_1, _15);

  <bb 10> [local count: 1073612976]:
  __dnew.6_20 = __dnew;
  MEM[(size_type *)&str + 8B] = __dnew.6_20;
  _21 = MEM[(char * *)&str];
  _22 = _21 + __dnew.6_20;
  MEM[(char_type &)_22] = 0;
  __dnew ={v} {CLOBBER};
  D.28812 ={v} {CLOBBER};
  D.28808 ={v} {CLOBBER};
  _7 = MEM[(char * *)&str];
  if (&str.D.19209._M_local_buf != _7)
    goto <bb 11>; [53.47%]
  else
    goto <bb 12>; [46.53%]

  <bb 11> [local count: 574060859]:
  _5 = str.D.19209._M_allocated_capacity;
  _3 = _5 + 1;
  operator delete (_7, _3);

  <bb 12> [local count: 1073612977]:
  str ={v} {CLOBBER};
  return _7;

}

I've been enhancing the warning to resolve pr71924 (and pr90549) so I might look into this if I get a chance.
Comment 5 Marc Glisse 2019-06-18 15:46:20 UTC
struct A { char*p; char c[13]; };
void f(A&a,bool b){
  a.p=b?a.c:(char*)__builtin_malloc(13);
  __builtin_memcpy(a.p, "hello world!", 12);
  a.p[12]=0;
}

gives

  if (b_4(D) != 0)
    goto <bb 3>; [67.00%]
  else
    goto <bb 4>; [33.00%]

  <bb 3> [local count: 719407023]:
  iftmp.0_9 = &a_8(D)->c;
  goto <bb 5>; [100.00%]

  <bb 4> [local count: 354334802]:
  iftmp.0_7 = __builtin_malloc (13);

  <bb 5> [local count: 1073741824]:
  # iftmp.0_2 = PHI <iftmp.0_9(3), iftmp.0_7(4)>
  a_8(D)->p = iftmp.0_2;
  __builtin_memcpy (iftmp.0_2, "hello world!", 12);
  _1 = a_8(D)->p;
  MEM[(char *)_1 + 12B] = 0;

Note in particular that the compiler fails to notice that memcpy cannot clobber the first field of p.

If I tweak the libstdc++ implementation to let it know that copying and writing the final 0 do not clobber the pointer, I can get

  # _47 = PHI <_59(9), &str.D.28972._M_local_buf(12), _59(8)>
  str ={v} {CLOBBER};
  return _47;

but that doesn't warn, although you just said that it warns for "maybe"s?
Comment 6 Martin Sebor 2019-06-18 16:55:34 UTC
With str being a local (non-reference) variable this should be diagnosed because of the str.D.28972._M_local_buf(12):

# _47 = PHI <_59(9), &str.D.28972._M_local_buf(12), _59(8)>
  str ={v} {CLOBBER};
  return _47;

In your example a is a reference argument but in this modified version:

  struct A { char *p; char c[13]; };

  void* f (struct A a, _Bool b)
  {
    a.p = b ? a.c : (char*)__builtin_malloc (13);
    __builtin_memcpy (a.p, "hello world!", 12);
    a.p[12] = 0;
    return a.p;
  }

and the IL:

  <bb 3> [local count: 354334802]:
  iftmp.0_7 = __builtin_malloc (13);

  <bb 4> [local count: 1073741824]:
  # iftmp.0_2 = PHI <iftmp.0_7(3), &a.c(2)>
  a.p = iftmp.0_2;
  __builtin_memcpy (iftmp.0_2, "hello world!", 12);
  _1 = a.p;
  MEM[(char *)_1 + 12B] = 0;
  return _1;

the only challenge with detecting the bug that I see is making a record of the rhs of the assignment to _1 = a.p (and others like that) and then checking the prior assignment to a.p (et al.).  With that in place the "may return" warning will trigger.
Comment 7 Marc Glisse 2019-06-18 17:45:46 UTC
(In reply to Martin Sebor from comment #6)
> With str being a local (non-reference) variable this should be diagnosed
> because of the str.D.28972._M_local_buf(12):
> 
> # _47 = PHI <_59(9), &str.D.28972._M_local_buf(12), _59(8)>
>   str ={v} {CLOBBER};
>   return _47;

the dump above was obtained from your testcase, compiled with a slightly hacked libstdc++

+++ basic_string.h	2019-06-18 17:40:06.435533019 +0200
@@ -214,7 +214,9 @@
       _M_set_length(size_type __n)
       {
 	_M_length(__n);
+	auto X = _M_data();
 	traits_type::assign(_M_data()[__n], _CharT());
+	if(X!=_M_data())__builtin_unreachable();
       }
 
       bool

+++ basic_string.tcc	2019-06-18 17:38:00.755534757 +0200
@@ -221,8 +221,12 @@
 	  }
 
 	// Check for out_of_range and length_error exceptions.
+	auto X = _M_data();
 	__try
-	  { this->_S_copy_chars(_M_data(), __beg, __end); }
+	  {
+	    this->_S_copy_chars(_M_data(), __beg, __end);
+	    if(X!=_M_data())__builtin_unreachable();
+	  }
 	__catch(...)
 	  {
 	    _M_dispose();

and I am really not seeing a warning. But that's probably because the path-isolation pass is too early (from a warning PoV), the IL doesn't look that nice at that time yet.
Comment 8 Martin Sebor 2019-06-19 15:37:15 UTC
Right.  The warning pass sees this:

  <bb 12> [local count: 1073612976]:
  __dnew ={v} {CLOBBER};
  D.29156 ={v} {CLOBBER};
  D.29152 ={v} {CLOBBER};
  if (&str.D.19306._M_local_buf != _23)
    goto <bb 13>; [53.47%]
  else
    goto <bb 14>; [46.53%]

  <bb 13> [local count: 574060859]:
  _5 = str.D.19306._M_allocated_capacity;
  _3 = _5 + 1;
  operator delete (_23, _3);

  <bb 14> [local count: 1073612977]:
  str ={v} {CLOBBER};
  return _23;

Maybe the alias oracle could be put to use here after all to improve things.