From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 461983858D3C; Wed, 12 Apr 2023 09:29:38 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 461983858D3C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1681291778; bh=nzdd8bywN20e9wTPFV3oUh5Z8iMD2+isU7hYgjAVAGs=; h=From:To:Subject:Date:In-Reply-To:References:From; b=YBGBY1zTgcSbfkbbV0II78H2zzjCTiEeJ5chXkkSr+E1V7OL+6N1yaxAPzHw0UUXx j/h4e6Wz/VfG6g3TmVlcHGMpP5oDH/e9g9PnLf3zNLdnhHw5XLJDUh81ujFJxmL5rd PrtLfJubzBCE9B+igiF9FUD8JJze6dcjyv+dmm/w= From: "redi at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug libstdc++/109442] Dead local copy of std::vector not removed from function Date: Wed, 12 Apr 2023 09:29:36 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: libstdc++ X-Bugzilla-Version: 13.0 X-Bugzilla-Keywords: missed-optimization X-Bugzilla-Severity: normal X-Bugzilla-Who: redi at gcc dot gnu.org X-Bugzilla-Status: RESOLVED X-Bugzilla-Resolution: WONTFIX X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: bug_status resolution Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D109442 Jonathan Wakely changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |WONTFIX --- Comment #5 from Jonathan Wakely --- (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 destroyi= ng > > the elements, precisely because it's a no-op. So we don't actually dest= roy > > the elements, which means the compiler might think they're still initia= lized > > and so could be inspected. > >=20 > > 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. > >=20 > > 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. >=20 > I don't think that would help. The issue is the compiler thinks that >=20 > operator delete (_37, _49); >=20 > uses the memory at _37 and thus the stores >=20 > *_37 =3D _24; >=20 > and >=20 > __builtin_memmove (_37, pretmp_63, _23); >=20 > are not dead. But if the library did _37->~_Tp() to destroy the element at _37 then it wo= uld 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. 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 readi= ng uninitialized storage, so any value is acceptable. So the stores to _37 sho= uld be DSE-able 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. >=20 > I see ~_Vector_base uses >=20 > typedef __gnu_cxx::__alloc_traits<_Tp_alloc_type> _Tr; > if (__p) > _Tr::deallocate(_M_impl, __p, __n); >=20 > 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 >=20 > _GLIBCXX_OPERATOR_DELETE(_GLIBCXX_SIZED_DEALLOC(__p, __n)); >=20 > 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). >=20 > Indeed in preprocessed source the above expands to >=20 > ::operator delete((__p), (__n) * sizeof(_Tp)); >=20 > rather than >=20 > delete[] __p; Yes, that's correct. Using a delete expression here would be completely wro= ng. > (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. >=20 > When I replace the above operator invocation in the library with >=20 > 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. >=20 > 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.=