[Bug c++/108565] New: -Wuse-after-free false positive on a shared_ptr implementation triggered by -O2

[Bug c++/108565] New: -Wuse-after-free false positive on a shared_ptr implementation triggered by -O2

[egor_suvorov at mail dot ru]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108565

            Bug ID: 108565
           Summary: -Wuse-after-free false positive on a shared_ptr
                    implementation triggered by -O2
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: egor_suvorov at mail dot ru
  Target Milestone: ---

May or may not be a duplicate of
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106119

Consider the following code, which is a subset of a shared_ptr's
implementation:

struct shared_ptr {
    int *counter_ = nullptr;
    int *data_ = nullptr;
    shared_ptr(int *data)
        : counter_(data ? new int(1) : nullptr), data_(data) {
    }
    shared_ptr(const shared_ptr &other)
        : counter_(other.counter_), data_(other.data_) {
        if (counter_ != nullptr) {
            ++*counter_;
        }
    }
    ~shared_ptr() {
        if (counter_ != nullptr) {
            --*counter_;
            if (*counter_ == 0) {
                delete counter_;
                delete data_;
            }
        }
    }
};
void foo() {
    shared_ptr a(new int(10));  // should be non-nullptr
    shared_ptr b(a);
    shared_ptr c(new int(20));  // should be non-nullptr
}
int main() {
    foo();
}

`g++ -std=c++17 -Wuse-after-free -O1` compiles this code without any problems,
and it runs without memory leaks or double-frees. However, `g++ -std=c++17
-Wuse-after-free -O2` generates a bogus warning:

In destructor 'shared_ptr::~shared_ptr()',
    inlined from 'void foo()' at x.cpp:27:1:
x.cpp:15:15: warning: pointer used after 'void operator delete(void*, long long
unsigned int)' [-Wuse-after-free]
   15 |             --*counter_;
      |               ^~~~~~~~~
In destructor 'shared_ptr::~shared_ptr()',
    inlined from 'void foo()' at x.cpp:27:1:
x.cpp:17:24: note: call to 'void operator delete(void*, long long unsigned
int)' here
   17 |                 delete counter_;
      |                        ^~~~~~~~

Seems like GCC is not smart enough to understand the underlying logic of
'shared_ptr'. Any of the following transformations remove the warning for me:

* Replacing `new int(10)` or `new int(20)` with nullptr
* Removing the ternary operator from `counter_`'s initialization.
* Replacing `-O2` with `-O1` or `-O0`

I've tested it both on trunk via Godbolt (https://godbolt.org/z/6ed1GY9Y7), and
on my local GCC 12.2 (Windows, msys2) and GCC 12.1 (Ubuntu 22.04).

[Bug tree-optimization/108565] -Wuse-after-free false positive triggered by -O2 on a shared_ptr implementation

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108565

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The missed optimization is because we don't optimize:

  MEM[(int *)_28] = 2;
  _8 = operator new (4);

  <bb 3> [local count: 1073741825]:
  MEM[(int *)_8] = 20;
  MEM[(int *)_8] ={v} {CLOBBER};
  operator delete (_8, 4);
  _37 = MEM[(int *)_28];

Until after fre5 and then we miss that _37 == 2.

[Bug tree-optimization/108565] -Wuse-after-free false positive triggered by -O2 on a shared_ptr implementation

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108565

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |EH

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is also exceptions related, that is -fno-exceptions causes no warning to
show up and also the function optimized too.

Also using malloc/free works too:
```
#if 1
int *mymalloc(int t)
{
    int *a = (int*)__builtin_malloc(sizeof(int));
    *a = t;
    return a;
}
void myfree(int *a)
{
    __builtin_free(a);
}
#else
#include <new>
int *mymalloc(int t)
{
    return new int(t);
}
void myfree(int *a)
{
    delete a;
}
#endif
struct shared_ptr {
    int *counter_ = nullptr;
    int *data_ = nullptr;
    shared_ptr(int *data)
    {
       // try {
        counter_ = (data ? mymalloc(1) : nullptr);
       // }catch(...) {
      //      counter_ = nullptr;
           // throw;
       // }
        data_ = (data);
    }
    shared_ptr(const shared_ptr &other)
        : counter_(other.counter_), data_(other.data_) {
        if (counter_ != nullptr) {
            ++*counter_;
        }
    }
    ~shared_ptr() {
        if (counter_ != nullptr) {
            --*counter_;
            if (*counter_ == 0) {
                myfree(counter_);
                myfree(data_);
            }
        }
    }
};
void foo() {
    shared_ptr a(mymalloc(10));  // should be non-nullptr
    shared_ptr b(a);
    shared_ptr c(mymalloc(20));  // should be non-nullptr
}
int main() {
    foo();
}
```

[Bug tree-optimization/108565] -Wuse-after-free false positive triggered by -O2 on a shared_ptr implementation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108565

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-01-27
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jason at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
  <bb 2> [local count: 1073741824]:
  _4 = operator new (4);
  MEM[(int *)_4] = 10;
  a ={v} {CLOBBER};
  if (_4 != 0B)
    goto <bb 3>; [53.47%]
  else
    goto <bb 4>; [46.53%]

I suspect 'operator new' is not returns_nonnull for the variant that isn't
noexcept.  That would be my first stab at improving things.  Not sure
what the standard guarantees and where we can adjust things here?  We
currently rely on VRP assumptions to derive _4 != 0 from the store
preceeding it, but value-numbering doesn't (yet) have any of that wired in.

The issue with the CSE of MEM[(int *)_28] is that _28 escapes through
passing 'b' to the not inlined shared_ptr::~shared_ptr (&b) DTOR.  It
looks like modref is not powerful enough to see that this DTOR doesn't
make *this or **this escape (it "escapes" to operator delete maybe).
The late modref sees

modref analyzing 'shared_ptr::~shared_ptr()/7' (ipa=0)
Past summary:
  loads:
    Every base
  stores:
    Every base
  kills:
     Parm 0 param offset:0 offset:0 size:128 max_size:128
  Side effects
  Nondeterministic
  Global memory read
  Global memory written
  parm 0 flags: no_direct_escape

  Analyzing stmt: _10 = this_6(D)->data_;
    Analyzing flags of ssa name: _10
      Analyzing stmt: operator delete (_10, 4); [tail call]
      current flags of _10 no_direct_escape no_indirect_escape
not_returned_directly not_returned_indirectly
      Analyzing stmt: if (_10 != 0B)
      current flags of _10 no_direct_escape no_indirect_escape
not_returned_directly not_returned_indirectly
    flags of ssa name _10 no_direct_escape no_indirect_escape
not_returned_directly not_returned_indirectly
  current flags of this_6(D) no_direct_clobber no_direct_escape
no_indirect_escape not_returned_directly not_returned_indirectly

good!  But:

  Analyzing stmt: _1 = this_6(D)->counter_;
    Analyzing flags of ssa name: _1
      Analyzing stmt: operator delete (_1, 4);
      current flags of _1 no_direct_escape no_indirect_escape
not_returned_directly not_returned_indirectly
      Analyzing stmt: *_1 = _3;
      current flags of _1 no_direct_escape no_indirect_escape
not_returned_directly not_returned_indirectly
      Analyzing stmt: _2 = *_1;
        Analyzing flags of ssa name: _2
          Analyzing stmt: _3 = _2 + -1;
            Analyzing flags of ssa name: _3
              Analyzing stmt: if (_3 == 0)
              current flags of _3 no_direct_clobber no_indirect_clobber
no_direct_escape no_indirect_escape not_returned_directly
not_returned_indirectly no_direct_read no_indirect_read
              Analyzing stmt: *_1 = _3;
              ssa name saved to memory
              current flags of _3
            flags of ssa name _3
          current flags of _2
        flags of ssa name _2
      current flags of _1 no_direct_escape not_returned_directly
      Analyzing stmt: if (_1 != 0B)
      current flags of _1 no_direct_escape not_returned_directly
    flags of ssa name _1 no_direct_escape not_returned_directly
  current flags of this_6(D) no_direct_clobber no_direct_escape
not_returned_directly

not sure what it makes dropping he no_indirect_escape - it seems the
'ssa name saved to memory', but that should affect _3 which only
"escapes" to where it was loaded from?

With the modref analysis fixed points-to done before PRE should
improve and thus PRE VN should perform the desired transform earlier.

[Bug tree-optimization/108565] -Wuse-after-free false positive triggered by -O2 on a shared_ptr implementation

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108565

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> I suspect 'operator new' is not returns_nonnull for the variant that isn't
> noexcept.  That would be my first stab at improving things.  Not sure
> what the standard guarantees and where we can adjust things here?

The standard guarantees that the noexcept(false) operator new returns nonnull
or throws, it never returns null.

I thought that was already implicitly returns_nonnull, but maybe not the FE
just treats it as non-null for the new-expression, but that info doesn't make
it into the IR.

I don't think we can just add the attribute in the library because of:

       -fcheck-new
           Check that the pointer returned by "operator new" is non-null before
attempting to modify the storage allocated.  This check is normally unnecessary
because the C++ standard specifies that "operator new" only returns 0 if it is
declared "throw()", in which case the compiler always checks the return value
even without this option.  In all other cases, when "operator new" has a
non-empty exception specification, memory exhaustion is signalled by throwing
"std::bad_alloc".  See also new (nothrow).

[Bug tree-optimization/108565] -Wuse-after-free false positive triggered by -O2 on a shared_ptr implementation

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108565

--- Comment #5 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Teaching modref that THIS parameter of all destructors is nonescape looks like
interesting idea (and easy to implement).

Memory stores are currently indeed handled as "anyting may happen". modref does
dataflow at SSA graph.  Making it to walk from non-escaping stores to loads may
make sense, but it is also somewhat heavy but I guess we will eventually want
to have it.