[Bug libstdc++/109442] Dead local copy of std::vector not removed from function

["rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
                 CC|                            |jason at gcc dot gnu.org
         Resolution|WONTFIX                     |---

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> (In reply to Richard Biener from comment #4)
> > (In reply to Jonathan Wakely from comment #3)
> > > Ah, maybe the problem is that the library code manually elides destroying
> > > the elements, precisely because it's a no-op. So we don't actually destroy
> > > the elements, which means the compiler might think they're still initialized
> > > and so could be inspected.
> > > 
> > > If the library explicitly does vec[i].~T() for every i then would that help?
> > > The compiler would know there are no valid elements in the storage, and so
> > > nothing operator delete could inspect.
> > > 
> > > We could continue to elide destroying the elements when !defined
> > > __OPTIMIZE__ so that we don't run a loop that does nothing, but with
> > > optimization enabled rely on the compiler to remove that loop.
> > 
> > I don't think that would help.  The issue is the compiler thinks that
> > 
> > operator delete (_37, _49);
> > 
> > uses the memory at _37 and thus the stores
> > 
> > *_37 = _24;
> > 
> > and
> > 
> > __builtin_memmove (_37, pretmp_63, _23);
> > 
> > are not dead.
> 
> But if the library did _37->~_Tp() to destroy the element at _37 then it
> would be dead, and accessing the element outside its lifetime would be
> undefined. The bytes can only be accessed as char, unsigned char or
> std::byte after that.
> 
> > IIRC 'operator delete' (_ZdlPvm in this case), can be
> > overridden by the user and can inspect the memory state before "releasing"
> > the storage?
> 
> That would be insane for operator delete to do that. Maybe possible, but
> insane.

Well, the standard doesn't prohibit "insanity" so we have to generate
correct code even in those cases, no?  After all this is what PR101480
was about ("insane" stuff) done even to new/delete _expressions_.

Would it be correct (by the standards wording) to extend the current
handling of new/delete expressions - which is that 'new' returns a pointer
not aliasing to any other pointer and that 'delete' does not read from
the memory pointed to by the first argument and this pointer also does not
escape to direct calls of operator new/delete?

Basically in gimple.cc:gimple_call_fnspec we currenty do

  /* If the call is to a replaceable operator delete and results
     from a delete expression as opposed to a direct call to
     such operator, then we can treat it as free.  */
  if (fndecl
      && DECL_IS_OPERATOR_DELETE_P (fndecl)
      && DECL_IS_REPLACEABLE_OPERATOR (fndecl)
      && gimple_call_from_new_or_delete (stmt))
    return ". o ";
  /* Similarly operator new can be treated as malloc.  */
  if (fndecl
      && DECL_IS_REPLACEABLE_OPERATOR_NEW_P (fndecl)
      && gimple_call_from_new_or_delete (stmt))
    return "m ";

where the comments do not exactly match the behavior (we also allow
arbitrary side-effects on global memory).  That we model 'delete'
as possibly writing to the released storage is so it serves as barrier for
earlier loads/stores to avoid sinking them across.

> In any case, the stores to _37 would be dead now, right? So even if operator
> delete inspects the memory as raw bytes using memcmp or similar, it's
> reading uninitialized storage, so any value is acceptable. So the stores to
> _37 should be DSE-able now.

There's noting in the IL indicating that though (but I haven't updated
libstd++ a while to create a updated preprocessed source - will do so now).

> > This also seems to be a form not handled by fndecl_dealloc_argno
> > even though it's marked as DECL_IS_OPERATOR_DELETE_P and
> > DECL_IS_REPLACEABLE_OPERATOR - but the actual call stmt is not marked
> > as such.  That's to catch a new/delete _expression_ and not a direct
> > call to the operator - ISTR we need the semantics guaranteed by the
> > standard for new/delete expressions here.
> > 
> > I see ~_Vector_base uses
> > 
> >  typedef __gnu_cxx::__alloc_traits<_Tp_alloc_type> _Tr;
> >  if (__p)
> >    _Tr::deallocate(_M_impl, __p, __n);
> > 
> > but I fail to trace that further (in the preprocessed source), the
> > line info on the delete stmt above points to new_allocator.h:168
> > which is
> > 
> >         _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n));
> > 
> > which looks like a direct invocation of operator delete rather than
> > a delete expression.  So the compiler rightfully(?) refuses to apply
> > strict semantics ('delete' is _just_ like free with no other side-effects,
> > a 'new' / 'delete' pair may be elided).
> > 
> > Indeed in preprocessed source the above expands to
> > 
> >  ::operator delete((__p), (__n) * sizeof(_Tp));
> > 
> > rather than
> > 
> >   delete[] __p;
> 
> Yes, that's correct. Using a delete expression here would be completely
> wrong.

I see.  That's unfortunate (we'll never be able to elide the actual
allocation then).

> > (or what the correct syntax with explicit size would be).  In theory
> > we could implement an attribute specifying a operator new or delete
> > invocation acts like a new or delete expression and use that in the
> > library and make sure that CALL_FROM_NEW_OR_DELETE_P is set on the
> > generated CALL_EXPRs.
> > 
> > When I replace the above operator invocation in the library with
> > 
> >   delete[] (char *)__p;
> 
> That would make std::vector incorrect though.
> 
> > then the dead stores are elided but since I didn't track down the call
> > to 'operator new' which suffers from a similar problem the new/delete
> > pair isn't elided yet.
> > 
> > So in the end it seems this is a library/C++ frontend issue.
> 
> Then there's no bug. std::vector is correct, and using a delete[] expression
> would be a bug.

OK, then how do we arrange things so the compiler knows it can completely
elide the unused std::vector?  Or do you say it's not valid to do that?