[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #21 from Jan Hubicka --- This patch attempts to add __builtin_operator_new/delete. So far they are not optimized, which will need to be done by extra flag of BUILT_IN_ code. also the decl.cc code can be refactored to be less of cut and I guess has_builtin hack to return proper value needs to be moved to C++ FE. However the immediate problem I run into is that libstdc++ testuiste fails due to lack of std::nothrow overrides. I wonder how to get that working?
Re: [Bug libstdc++/109442] Dead local copy of std::vector not removed from function
This patch attempts to add __builtin_operator_new/delete. So far they
are not optimized, which will need to be done by extra flag of BUILT_IN_
code. also the decl.cc code can be refactored to be less of cut
and I guess has_builtin hack to return proper value needs to be moved
to C++ FE.
However the immediate problem I run into is that libstdc++ testuiste
fails due to lack of std::nothrow overrides. I wonder how to get that
working?
diff --git a/gcc/c-family/c-lex.cc b/gcc/c-family/c-lex.cc
index ff5ce2bf729..602b097059c 100644
--- a/gcc/c-family/c-lex.cc
+++ b/gcc/c-family/c-lex.cc
@@ -533,6 +533,10 @@ c_common_has_builtin (cpp_reader *pfile)
if (!name)
return 0;
+ if (!strcmp (name, "__builtin_operator_new")
+ || !strcmp (name, "__builtin_operator_delete"))
+return 201802L;
+
return names_builtin_p (name);
}
diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 3fc8835154d..90b100ca3dc 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If not see
#include "omp-general.h"
#include "omp-offload.h" /* For offload_vars. */
#include "opts.h"
+#include "print-tree.h"
#include "langhooks-def.h" /* For lhd_simulate_record_decl */
/* Possible cases of bad specifiers type used by bad_specifiers. */
@@ -5048,13 +5049,27 @@ cxx_init_decl_processing (void)
DECL_IS_MALLOC (opnew) = 1;
DECL_SET_IS_OPERATOR_NEW (opnew, true);
DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
+tree builtin_opnew = build_cp_library_fn
(get_identifier("__builtin_operator_new"),
+ NEW_EXPR, newtype, 0);
+DECL_IS_MALLOC (builtin_opnew) = 1;
+DECL_SET_IS_OPERATOR_NEW (builtin_opnew, true);
+DECL_IS_REPLACEABLE_OPERATOR (builtin_opnew) = 1;
+SET_DECL_ASSEMBLER_NAME (builtin_opnew, DECL_ASSEMBLER_NAME (opnew));
+pushdecl (builtin_opnew);
opnew = push_cp_library_fn (VEC_NEW_EXPR, newtype, 0);
DECL_IS_MALLOC (opnew) = 1;
DECL_SET_IS_OPERATOR_NEW (opnew, true);
DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
+
tree opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
DECL_SET_IS_OPERATOR_DELETE (opdel, true);
DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+tree builtin_opdel = build_cp_library_fn
(get_identifier("__builtin_operator_delete"),
+ DELETE_EXPR, deltype,
ECF_NOTHROW);
+DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+pushdecl (builtin_opdel);
opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
DECL_SET_IS_OPERATOR_DELETE (opdel, true);
DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
@@ -5072,6 +5087,12 @@ cxx_init_decl_processing (void)
opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
DECL_SET_IS_OPERATOR_DELETE (opdel, true);
DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+ builtin_opdel = build_cp_library_fn
(get_identifier("__builtin_operator_delete"),
+DELETE_EXPR, deltype, ECF_NOTHROW);
+ DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+ DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+ SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+ pushdecl (builtin_opdel);
opdel = push_cp_library_fn (VEC_DELETE_EXPR, deltype, ECF_NOTHROW);
DECL_SET_IS_OPERATOR_DELETE (opdel, true);
DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
@@ -5094,6 +5115,13 @@ cxx_init_decl_processing (void)
DECL_IS_MALLOC (opnew) = 1;
DECL_SET_IS_OPERATOR_NEW (opnew, true);
DECL_IS_REPLACEABLE_OPERATOR (opnew) = 1;
+ builtin_opnew = build_cp_library_fn
(get_identifier("__builtin_operator_new"),
+VEC_NEW_EXPR, newtype, 0);
+ DECL_IS_MALLOC (builtin_opnew) = 1;
+ DECL_SET_IS_OPERATOR_NEW (builtin_opnew, true);
+ DECL_IS_REPLACEABLE_OPERATOR (builtin_opnew) = 1;
+ SET_DECL_ASSEMBLER_NAME (builtin_opnew, DECL_ASSEMBLER_NAME (opnew));
+ pushdecl (builtin_opnew);
opnew = push_cp_library_fn (VEC_NEW_EXPR, newtype, 0);
DECL_IS_MALLOC (opnew) = 1;
DECL_SET_IS_OPERATOR_NEW (opnew, true);
@@ -5107,6 +5135,12 @@ cxx_init_decl_processing (void)
opdel = push_cp_library_fn (DELETE_EXPR, deltype, ECF_NOTHROW);
DECL_SET_IS_OPERATOR_DELETE (opdel, true);
DECL_IS_REPLACEABLE_OPERATOR (opdel) = 1;
+ builtin_opdel = build_cp_library_fn
(get_identifier("__builtin_operator_delete"),
+DELETE_EXPR, deltype, ECF_NOTHROW);
+ DECL_SET_IS_OPERATOR_DELETE (builtin_opdel, true);
+ DECL_IS_REPLACEABLE_OPERATOR (builtin_opdel) = 1;
+ SET_DECL_ASSEMBLER_NAME (builtin_opdel, DECL_ASSEMBLER_NAME (opdel));
+ pushdecl
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
--- Comment #20 from Jonathan Wakely ---
(In reply to Jan Hubicka from comment #19)
> Similar argument can IMO be used for eliding unused memory allocations. It
> is kind of up to std::vector implementation on how many
> allocations/deallocations it does, right?
It's up to std::allocator, which is not required to call operator new every
time memory is needed.
> So we need a way to annotate the new/delete calls in the standard library as
> safe for such optimizations (i.e. implement clang's
> __bulitin_operator_new/delete?)
Yes, see PR 110137.
> How clang manages to optimize this out without additional hinting?
It supports __builtin_operator_{new,delete} and libstdc++ uses that when
compiled with clang.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
--- Comment #19 from Jan Hubicka ---
Note that the testcase from PR115037 also shows that we are not able to
optimize out dead stores to the vector, which is another quite noticeable
problem.
void
test()
{
std::vector test;
test.push_back (1);
}
We alocate the block, store 1 and immediately delete it.
void test ()
{
int * test$D25839$_M_impl$D25146$_M_start;
struct vector test;
int * _61;
[local count: 1073741824]:
_61 = operator new (4);
[local count: 1063439392]:
*_61 = 1;
operator delete (_61, 4);
test ={v} {CLOBBER};
test ={v} {CLOBBER(eol)};
return;
[count: 0]:
:
test ={v} {CLOBBER};
resx 2
}
So my understanding is that we decided to not optimize away the dead stores
since the particular operator delete does not pass test:
/* 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 ";
This is because we believe that operator delete may be implemented in an insane
way that inspects the values stored in the block being freed.
I can sort of see that one can write standard conforming code that allocates
some data that is POD and inspects it in destructor.
However for std::vector this argument is not really applicable. Standard does
specify that new/delete is used to allocate/deallocate the memory but does not
say how the memory is organized or what happens before deallocation.
(i.e. it is probably valid for std::vector to memset the block just before
deallocating it).
Similar argument can IMO be used for eliding unused memory allocations. It is
kind of up to std::vector implementation on how many allocations/deallocations
it does, right?
So we need a way to annotate the new/delete calls in the standard library as
safe for such optimizations (i.e. implement clang's
__bulitin_operator_new/delete?)
How clang manages to optimize this out without additional hinting?
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 Xi Ruoyao changed: What|Removed |Added CC||hubicka at gcc dot gnu.org --- Comment #18 from Xi Ruoyao --- *** Bug 115037 has been marked as a duplicate of this bug. ***
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #17 from AK --- With recent changes in libc++ (https://reviews.llvm.org/D147741) clang optimizes away the new-delete pair. https://godbolt.org/z/a6PG54Pvb $ clang++ -O3 -stdlib=libc++ -fno-exceptions vat1(std::__1::vector >): # @vat1(std::__1::vector >) sub rsp, 24 xorps xmm0, xmm0 movaps xmmword ptr [rsp], xmm0 mov qword ptr [rsp + 16], 0 mov rax, qword ptr [rdi + 8] sub rax, qword ptr [rdi] je .LBB0_2 js .LBB0_3 .LBB0_2: mov eax, 10 add rsp, 24 ret .LBB0_3: mov rdi, rsp call std::__1::vector >::__throw_length_error[abi:v17]() const .L.str: .asciz "vector" .L.str.1: .asciz "length_error was thrown in -fno-exceptions mode with message \"%s\"" Previously clang couldn't even convert the copy to a memmove and would generate a raw loop e.g., https://godbolt.org/z/G8ax1o5bc .LBB0_6: # =>This Inner Loop Header: Depth=1 movups xmm0, xmmword ptr [r15 + 4*rdi] movups xmm1, xmmword ptr [r15 + 4*rdi + 16] movups xmmword ptr [rax + 4*rdi], xmm0 movups xmmword ptr [rax + 4*rdi + 16], xmm1 add rdi, 8 cmp rsi, rdi jne .LBB0_6 cmp rbx, rsi jne .LBB0_8 jmp .LBB0_9 .LBB0_3:
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
--- Comment #16 from Richard Smith
---
(In reply to Richard Biener from comment #15)
> I was specifically looking at C++20 7.6.2.7/10 to /14 (but maybe also
> others and of course the relevant parts of the delete expression). In
> particular the extra leeway the standard provides to new/delete _expressions_
> vs. calls of the global replaceable operators directly - do the
> __builtin_operator_{new,delete} in this regard behave like new/delete
> _expressions_ or like direct calls to the operators?
They permit the same optimizations as new/delete expressions. The sections of
the C++ standard that you referred to are what the documentation for the
builtins means when it says:
"[__builtin_operator_new] allows certain optimizations that the C++ standard
does not permit for a direct function call to ::operator new (in particular,
removing new / delete pairs and merging allocations)"
> Do the builtins call one of the replaceable global new/delete operators
> and thus users can reliably override them?
If the implementation doesn't provide storage in some other way, optimize away
the allocation, or merge it with another allocation, then yes, the storage is
obtained by calling the corresponding replaceable global new/delete operators.
Per the documentation:
"A call to __builtin_operator_new(args) is exactly the same as a call to
::operator new(args), except that [text quoted above]"
> How do the builtins behave during constexpr evaluation? new/delete
> expressions have their behavior documented in the standard.
They behave exactly like a direct call to the replaceable global allocation
function.
In Clang's implementation, direct calls to ::operator new and ::operator delete
are permitted within calls to std::allocator::allocate and
std::allocator::deallocate during constant evaluation, and are treated as
allocating or deallocating arrays of T; consequently, calls to
__builtin_operator_new and __builtin_operator_delete are permitted in the same
contexts. In all other contexts, Clang rejects such calls, because the callee
is not declared `constexpr`.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
--- Comment #15 from Richard Biener ---
(In reply to Richard Smith from comment #14)
> If I understand correctly, you're looking for documentation that
>
> __builtin_operator_new(size)
>
> has the exact same semantics and permits the same optimizations as `::new T`
> for a trivially-constructible non-array type `T` whose size is `size` and
> that
>
> __builtin_operator_delete(p)
>
> has the exact same semantics and permits the same optimizations as `::delete
> p` for a trivially-destructible non-array type `T` whose size is `size`,
> with `p` of type `T*` -- and similarly for the other (aligned, nothrow)
> variants?
>
> That is the intent; I can look into getting Clang's documentation updated to
> say that more explicitly if that's useful to you.
I was specifically looking at C++20 7.6.2.7/10 to /14 (but maybe also
others and of course the relevant parts of the delete expression). In
particular the extra leeway the standard provides to new/delete _expressions_
vs. calls of the global replaceable operators directly - do the
__builtin_operator_{new,delete} in this regard behave like new/delete
_expressions_ or like direct calls to the operators?
Do the builtins call one of the replaceable global new/delete operators
and thus users can reliably override them?
How do the builtins behave during constexpr evaluation? new/delete
expressions have their behavior documented in the standard.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #14 from Richard Smith --- If I understand correctly, you're looking for documentation that __builtin_operator_new(size) has the exact same semantics and permits the same optimizations as `::new T` for a trivially-constructible non-array type `T` whose size is `size` and that __builtin_operator_delete(p) has the exact same semantics and permits the same optimizations as `::delete p` for a trivially-destructible non-array type `T` whose size is `size`, with `p` of type `T*` -- and similarly for the other (aligned, nothrow) variants? That is the intent; I can look into getting Clang's documentation updated to say that more explicitly if that's useful to you.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 Jonathan Wakely changed: What|Removed |Added CC||richard-gccbugzilla@metafoo ||.co.uk --- Comment #13 from Jonathan Wakely --- @zygoloid, are you the right person to ask about __builtin_operator_new semantics? See the previous comment (comment 12)
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #12 from rguenther at suse dot de --- On Wed, 12 Apr 2023, redi at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 > > --- Comment #11 from Jonathan Wakely --- > (In reply to Jonathan Wakely from comment #10) > > I would prefer if we don't deviate, and get Clang to clarify things instead > > of reinventing something that looks similar but isn't. > > I suppose as long as the common subset of semantics that the library cares > about works the same way, it's OK. > > It would be bad if the library needs to use extra checks beyond __has_builtin > to discover which "flavour" of __builtin_operator_new is present. So if we can get clarification that __builtin_operator_new/delete allow at least all transforms as if using a new or delete expression then we can use the same representation as that in the middle-end for now (just set the CALL_FROM_NEW_OR_DELETE_P flag). If the builtins provide _additional_ guarantees then we can make use of those only when we extend things on the middle-end side. Now we then only need to make the C++ frontend "mangle" the __builtin_operator_new/delete calls to the "correct" allocation function. I suppose the goal is to resolve to exactly the same symbols as when not using the builtins.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #11 from Jonathan Wakely --- (In reply to Jonathan Wakely from comment #10) > I would prefer if we don't deviate, and get Clang to clarify things instead > of reinventing something that looks similar but isn't. I suppose as long as the common subset of semantics that the library cares about works the same way, it's OK. It would be bad if the library needs to use extra checks beyond __has_builtin to discover which "flavour" of __builtin_operator_new is present.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #10 from Jonathan Wakely --- (In reply to Richard Biener from comment #9) > Yep, looking at that link this is exactly what would be needed. Note > that in the middle-end we already see the calls to > DECL_IS_REPLACEABLE_OPERATOR_NEW_P and replaceable operator delete. We > just restrict all optimizations to calls emitted from new/delete expressions. > I'm not sure the clang builtin is exactly providing the new/delete > expression semantics resolving to replaceable operator new/delete or if > there's other details. That is, I'm curious whether __builtin_operator_new > provides more guarantees than a new/delete expression - the builtin semantics > isn't very well defined unfortunately. It might be safer to go our own > way here. I would prefer if we don't deviate, and get Clang to clarify things instead of reinventing something that looks similar but isn't. > I suppose the standard library could as well use malloc/free? No. std::allocator is required to use operator new and operator delete to obtain memory, but it's unspecified when or how often it calls them. So it's OK to call them once at startup to obtain ALL THE MEMORY that any std::allocator will ever need for the duration of the program, or to merge allocations, or to elide them completely. But it's not OK to use malloc. std::allocator::allocate says: "The storage for the array is obtained by calling ::operator new (17.6.3), but it is unspecified when or how often this function is called."
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #9 from Richard Biener --- (In reply to Jonathan Wakely from comment #8) > (In reply to Jonathan Wakely from comment #7) > > I think it's valid in theory. I don't know if it's possible for GCC to do it > > in practice. There doesn't seem to be anything the library can do to help, > > so WONTFIX. > > I think it's valid in theory because the implementation of std::vector is > opaque. It's not required to actually call operator new and operator delete > to obtain its storage, and so if the library and compiler collaborate to > provide storage some other way (e.g. using a stack buffer) then that's > allowed. > > But in practice it's hard to do that. > > Maybe it could be done with a new __builtin_elidable_operator_new and > __builtin_elidable_operator_delete pair that the library could use, or > attributes like you suggested in comment 4. That would allow the library to > say "I'm calling operator new and operator delete because I need to obtain > memory, but **these particular invocations** are not required to be visible > to the user, and therefore can be elided in the same way as new expressions > and delete expressions". > > In fact Clang's __builtin_operator_new already has exactly those semantics: > > https://clang.llvm.org/docs/LanguageExtensions.html#builtin-operator-new-and- > builtin-operator-delete > > And we already use those builtins if available, see PR 94295. > > So maybe what we need is to implement those. Yep, looking at that link this is exactly what would be needed. Note that in the middle-end we already see the calls to DECL_IS_REPLACEABLE_OPERATOR_NEW_P and replaceable operator delete. We just restrict all optimizations to calls emitted from new/delete expressions. I'm not sure the clang builtin is exactly providing the new/delete expression semantics resolving to replaceable operator new/delete or if there's other details. That is, I'm curious whether __builtin_operator_new provides more guarantees than a new/delete expression - the builtin semantics isn't very well defined unfortunately. It might be safer to go our own way here. I suppose the standard library could as well use malloc/free? Or is the standard library required to perform allocation with "replaceable operator new/delete"? (I suppose it helps not open-coding the exceptional cases)
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442 --- Comment #8 from Jonathan Wakely --- (In reply to Jonathan Wakely from comment #7) > I think it's valid in theory. I don't know if it's possible for GCC to do it > in practice. There doesn't seem to be anything the library can do to help, > so WONTFIX. I think it's valid in theory because the implementation of std::vector is opaque. It's not required to actually call operator new and operator delete to obtain its storage, and so if the library and compiler collaborate to provide storage some other way (e.g. using a stack buffer) then that's allowed. But in practice it's hard to do that. Maybe it could be done with a new __builtin_elidable_operator_new and __builtin_elidable_operator_delete pair that the library could use, or attributes like you suggested in comment 4. That would allow the library to say "I'm calling operator new and operator delete because I need to obtain memory, but **these particular invocations** are not required to be visible to the user, and therefore can be elided in the same way as new expressions and delete expressions". In fact Clang's __builtin_operator_new already has exactly those semantics: https://clang.llvm.org/docs/LanguageExtensions.html#builtin-operator-new-and-builtin-operator-delete And we already use those builtins if available, see PR 94295. So maybe what we need is to implement those.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
--- Comment #7 from Jonathan Wakely ---
(In reply to Richard Biener from comment #6)
> (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?
Indeed.
> 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?
No, I don't think so. The [expr.new] p13 permission to elide calls to operator
new seems to only apply to new expressions and not direct calls to operator
new.
> 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).
I'm talking about a potential change to libstdc++ to add _37->~_Tp() so you
won't see it in the IL now.
What I meant in comment 3 is that libstdc++ uses template metaprogramming to
manually elide destroying the elements, because trivial types like 'int' have
no destructor. See libstdc++-v3/include/bits/stl_construct.h lines 155-197:
template
struct _Destroy_aux
{
template
static _GLIBCXX20_CONSTEXPR void
__destroy(_ForwardIterator __first, _ForwardIterator __last)
{
for (; __first != __last; ++__first)
std::_Destroy(std::__addressof(*__first));
}
};
template<>
struct _Destroy_aux
{
template
static void
__destroy(_ForwardIterator, _ForwardIterator) { }
};
For vector we use the _Destroy_aux specialization, which is a no-op.
So the compiler thinks that the elements of the std::vector are still
alive when we free the storage.
I'm suggesting that if we actually ended the lifetime of the int elements, then
the compiler
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
Richard Biener changed:
What|Removed |Added
Status|RESOLVED|REOPENED
CC||jason at gcc dot gnu.org
Resolution|WONTFIX |---
--- Comment #6 from Richard Biener ---
(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
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
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 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.
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.
> 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.
> (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.
[Bug libstdc++/109442] Dead local copy of std::vector not removed from function
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109442
Richard Biener changed:
What|Removed |Added
Component|tree-optimization |libstdc++
--- Comment #4 from Richard Biener ---
(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. IIRC 'operator delete' (_ZdlPvm in this case), can be
overridden by the user and can inspect the memory state before "releasing"
the storage?
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;
(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;
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.
