Bug 23383

Confirmed:
  *a = 1;
  *b = 2;
  t = *a;
  operator delete (a);
  operator delete (b);
  return t;

*** Bug 35467 has been marked as a duplicate of this bug. ***

*** Bug 35559 has been marked as a duplicate of this bug. ***

This would not be legal, there is no reason operator new can't return a pointer that already exists in the program. 

(In reply to comment #4)
> This would not be legal, there is no reason operator new can't return a pointer
> that already exists in the program. 
> 

This is certainly a flaw in the C++ standard (it requires p returned from operator new to be different from previous calls to the operator unless the previous call's return is passed to operator delete -- but it does not require the pointer returned to be not pointing to any known location) -- and I do not see a reason this is intentional. 

For compiler optimization, it would better to assume the aggressive default, and provide an option to turn it off in case user provided definition violates the aliasing assumption.  Someone in the language committee might need to bring this up.

David

This has been extensively discussed on the C++ reflector.  They decided (informally, on the reflector) that people should be able to globally override operator new to do logging, etc, which can make malloc have arbitrary side effects.

That said, it would be fine to add support for this under a non-standards-mode option of some sort of course.

(In reply to comment #6)
> This has been extensively discussed on the C++ reflector.  They decided
> (informally, on the reflector) that people should be able to globally override
> operator new to do logging, etc, which can make malloc have arbitrary side
> effects.
> 
yes, things like saving pointers to user visible locations are bad.  In this kind of situation, optimizer people's voice seems not loud enough :(. 

One way to solve this problem is to require user who override the default definition to always provide a declaration (before any use of operator new) -- if such declaration is not seen, the operator new will be treated as builtin (the default) implementation which has the nice property.

David

This isn't possible.  The user can override operator new at the very last minute: e.g. by interposing a shared object with LD_PRELOAD.  There is no way that a compiler or even LTO optimizing linker can know about this.  A special non-standard compiler flag is required.

(In reply to comment #9)
> This isn't possible.  The user can override operator new at the very last
> minute: e.g. by interposing a shared object with LD_PRELOAD.  There is no way
> that a compiler or even LTO optimizing linker can know about this.  A special
> non-standard compiler flag is required.
> 

Then runtime override would be illegal (in theory) (if builtin operator is assumed), though the program may still run fine.

so LLVM's LTO completely gives up when seeing operator new or there is a special flag?

David

Expecting people to modify their source is bad news.

LLVM's LTO does nothing for operator new, it treats it as any other external function with undefined behavior.

Interesting things start to happen once you inline allocator functions as well.
See PR29286 and PR33407 which we still don't handle 100% correct.

(In reply to comment #12)
> Interesting things start to happen once you inline allocator functions as well.
> See PR29286 and PR33407 which we still don't handle 100% correct.
> 

I browsed through the two bugs -- it seems that compiler should get this right regardless -- local pointer analysis should detect the must aliasing and should overrule the type based aliasing decision when the placement new is inlined. If not inlined, compiler should know the exact semantics of placement new (return == arg), or treat it conservatively. 

David

We do the exact opposite - type-based rules override points-to must-alias
information (or really may-alias information).  Also for the proposed scheme
to work you need to guarantee that you always can compute correct points-to
relations (I mean, if points-to information says pt_anything and if you
then assume must-alias and thus a conflict then you simply disable TBAA
completely).

(In reply to comment #14)
> We do the exact opposite - type-based rules override points-to must-alias
> information (or really may-alias information).  Also for the proposed scheme
> to work you need to guarantee that you always can compute correct points-to
> relations (I mean, if points-to information says pt_anything and if you
> then assume must-alias and thus a conflict then you simply disable TBAA
> completely).
> 

Right, in general, type alias rules should override field and flow insensitive pointer aliasing information as they really have very low confidence level (especially for pt_anything case which is just a baseless guess) -- but  precise/trustworthy aliasing info should be checked before assertion based alias information and decide whether to proceed. 

For example:

if (no_alias_according_to_conservative_pointer_info) return no_alias;
if (no_alias_according_to_precise_pointer_info) return no_alias;
if (must_alias or definitely_may_alias) return may/must_alias;   (1)

// now proceed with type based rules, etc.


This is in theory. In practice, it can be tricky to tag the confidence level of aliasing info.

David

A related topic - aliasing property of realloc -- the malloc attribute is not applied in the glibc header and the comment is like

/* __attribute_malloc__ is not used, because if realloc returns
   the same pointer that was passed to it, aliasing needs to be allowed
   between objects pointed by the old and new pointers.  *


It is true that the realloc can return an address is physically aliased with the pointer passed to it -- but assuming no-alias by the compiler should cause no harm -- as all code motions/CSEs across the realloc call will not be possible because realloc may modify/use the memory location.


Any comment on this? 

David


(In reply to comment #15)
> (In reply to comment #14)
> > We do the exact opposite - type-based rules override points-to must-alias
> > information (or really may-alias information).  Also for the proposed scheme
> > to work you need to guarantee that you always can compute correct points-to
> > relations (I mean, if points-to information says pt_anything and if you
> > then assume must-alias and thus a conflict then you simply disable TBAA
> > completely).
> > 
> 
> Right, in general, type alias rules should override field and flow insensitive
> pointer aliasing information as they really have very low confidence level
> (especially for pt_anything case which is just a baseless guess) -- but 
> precise/trustworthy aliasing info should be checked before assertion based
> alias information and decide whether to proceed. 
> 
> For example:
> 
> if (no_alias_according_to_conservative_pointer_info) return no_alias;
> if (no_alias_according_to_precise_pointer_info) return no_alias;
> if (must_alias or definitely_may_alias) return may/must_alias;   (1)
> 
> // now proceed with type based rules, etc.
> 
> 
> This is in theory. In practice, it can be tricky to tag the confidence level of
> aliasing info.
> 
> David

On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> 
> --- Comment #16 from davidxl <xinliangli at gmail dot com> 2012-01-04 00:28:55 UTC ---
> A related topic - aliasing property of realloc -- the malloc attribute is not
> applied in the glibc header and the comment is like
> 
> /* __attribute_malloc__ is not used, because if realloc returns
>    the same pointer that was passed to it, aliasing needs to be allowed
>    between objects pointed by the old and new pointers.  *
> 
> 
> It is true that the realloc can return an address is physically aliased with
> the pointer passed to it -- but assuming no-alias by the compiler should cause
> no harm -- as all code motions/CSEs across the realloc call will not be
> possible because realloc may modify/use the memory location.
> 
> 
> Any comment on this? 

The malloc attribute assumes that the contents of the memory pointed
to by the return value is undefined, so the comment is inaccurate
but the malloc attribute can indeed be not used.

We can explicitly handle REALLOC in the points-to code to honor the
fact that it is not (we do not at the moment).

(In reply to comment #17)
> On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> > 
> > --- Comment #16 from davidxl <xinliangli at gmail dot com> 2012-01-04 00:28:55 UTC ---
> > A related topic - aliasing property of realloc -- the malloc attribute is not
> > applied in the glibc header and the comment is like
> > 
> > /* __attribute_malloc__ is not used, because if realloc returns
> >    the same pointer that was passed to it, aliasing needs to be allowed
> >    between objects pointed by the old and new pointers.  *
> > 
> > 
> > It is true that the realloc can return an address is physically aliased with
> > the pointer passed to it -- but assuming no-alias by the compiler should cause
> > no harm -- as all code motions/CSEs across the realloc call will not be
> > possible because realloc may modify/use the memory location.
> > 
> > 
> > Any comment on this? 
> 
> The malloc attribute assumes that the contents of the memory pointed
> to by the return value is undefined, so the comment is inaccurate
> but the malloc attribute can indeed be not used.

Which part of the optimizer takes advantage of the 'undefinedness' of returned memory?


> 
> We can explicitly handle REALLOC in the points-to code to honor the
> fact that it is not (we do not at the moment).

ok.

On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> 
> --- Comment #18 from davidxl <xinliangli at gmail dot com> 2012-01-04 17:11:26 UTC ---
> (In reply to comment #17)
> > On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:
> > 
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> > > 
> > > --- Comment #16 from davidxl <xinliangli at gmail dot com> 2012-01-04 00:28:55 UTC ---
> > > A related topic - aliasing property of realloc -- the malloc attribute is not
> > > applied in the glibc header and the comment is like
> > > 
> > > /* __attribute_malloc__ is not used, because if realloc returns
> > >    the same pointer that was passed to it, aliasing needs to be allowed
> > >    between objects pointed by the old and new pointers.  *
> > > 
> > > 
> > > It is true that the realloc can return an address is physically aliased with
> > > the pointer passed to it -- but assuming no-alias by the compiler should cause
> > > no harm -- as all code motions/CSEs across the realloc call will not be
> > > possible because realloc may modify/use the memory location.
> > > 
> > > 
> > > Any comment on this? 
> > 
> > The malloc attribute assumes that the contents of the memory pointed
> > to by the return value is undefined, so the comment is inaccurate
> > but the malloc attribute can indeed be not used.
> 
> Which part of the optimizer takes advantage of the 'undefinedness' of returned
> memory?

points-to analysis.  It assumes that the returned blob of memory
points to nothing (yet).  So for

 int i;
 int **p = malloc (8);
 *p = &i;
 int **q = realloc (p, 8);

you'd get that *q points to nothing insteda of i.

> > We can explicitly handle REALLOC in the points-to code to honor the
> > fact that it is not (we do not at the moment).
> 
> ok.

which needs to be fixed here.

Richard.

(In reply to comment #19)
> On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> > 
> > --- Comment #18 from davidxl <xinliangli at gmail dot com> 2012-01-04 17:11:26 UTC ---
> > (In reply to comment #17)
> > > On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:
> > > 
> > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> > > > 
> > > > --- Comment #16 from davidxl <xinliangli at gmail dot com> 2012-01-04 00:28:55 UTC ---
> > > > A related topic - aliasing property of realloc -- the malloc attribute is not
> > > > applied in the glibc header and the comment is like
> > > > 
> > > > /* __attribute_malloc__ is not used, because if realloc returns
> > > >    the same pointer that was passed to it, aliasing needs to be allowed
> > > >    between objects pointed by the old and new pointers.  *
> > > > 
> > > > 
> > > > It is true that the realloc can return an address is physically aliased with
> > > > the pointer passed to it -- but assuming no-alias by the compiler should cause
> > > > no harm -- as all code motions/CSEs across the realloc call will not be
> > > > possible because realloc may modify/use the memory location.
> > > > 
> > > > 
> > > > Any comment on this? 
> > > 
> > > The malloc attribute assumes that the contents of the memory pointed
> > > to by the return value is undefined, so the comment is inaccurate
> > > but the malloc attribute can indeed be not used.
> > 
> > Which part of the optimizer takes advantage of the 'undefinedness' of returned
> > memory?
> 
> points-to analysis.  It assumes that the returned blob of memory
> points to nothing (yet).  So for
> 
>  int i;
>  int **p = malloc (8);
>  *p = &i;
>  int **q = realloc (p, 8);
> 
> you'd get that *q points to nothing insteda of i.

The malloc attribute documentation is confusing:

malloc
The malloc attribute is used to tell the compiler that a function may be treated as if any non-NULL pointer it returns cannot alias any other pointer valid when the function returns. This will often improve optimization. Standard functions with this property include malloc and calloc. realloc-like functions have this property as long as the old pointer is never referred to (including comparing it to the new pointer) after the function returns a non-NULL value. 


It does not mention the undefineness explicitly.

It might be better to fix the semantics of this attribute to only specify the aliasing property so that it can also be applied to realloc. Points-to needs to be fixed to special case realloc for the initialization.

David


> 
> > > We can explicitly handle REALLOC in the points-to code to honor the
> > > fact that it is not (we do not at the moment).
> > 
> > ok.
> 
> which needs to be fixed here.
> 
> Richard.

But can't a valid code also compare the result from realloc with the old pointer, and if they are equal, do something, otherwise do something else?
I think it is pretty common e.g. if the malloced block contains pointers to parts of the malloced area and upon realloc that didn't return the passed address wants to adjust all those pointers.
Having a malloc attribute on realloc would still break this.
I'd say we want realloc attribute and handle it where we currently handle BUILT_IN_REALLOC.

(In reply to comment #21)
> But can't a valid code also compare the result from realloc with the old
> pointer, and if they are equal, do something, otherwise do something else?
> I think it is pretty common e.g. if the malloced block contains pointers to
> parts of the malloced area and upon realloc that didn't return the passed
> address wants to adjust all those pointers.
> Having a malloc attribute on realloc would still break this.
> I'd say we want realloc attribute and handle it where we currently handle
> BUILT_IN_REALLOC.

Right, the case you describe may be common. On the other hand, the compiler probably should not optimize away pointer comparisons without knowing if the pointer is used after 'free' or 'free' like function 'realloc'.

David

On Thu, 5 Jan 2012, jakub at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #21 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-01-05 18:26:17 UTC ---
> But can't a valid code also compare the result from realloc with the old
> pointer, and if they are equal, do something, otherwise do something else?
> I think it is pretty common e.g. if the malloced block contains pointers to
> parts of the malloced area and upon realloc that didn't return the passed
> address wants to adjust all those pointers.

Sure.

> Having a malloc attribute on realloc would still break this.

We cannot use the malloc attribute on realloc, ever.

> I'd say we want realloc attribute and handle it where we currently handle
> BUILT_IN_REALLOC.

Not sure it's worth it besides handling BUILT_IN_REALLOC explicitly.

On Thu, 5 Jan 2012, xinliangli at gmail dot com wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> 
> --- Comment #20 from davidxl <xinliangli at gmail dot com> 2012-01-05 18:11:18 UTC ---
> (In reply to comment #19)
> > On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:
> > 
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> > > 
> > > --- Comment #18 from davidxl <xinliangli at gmail dot com> 2012-01-04 17:11:26 UTC ---
> > > (In reply to comment #17)
> > > > On Wed, 4 Jan 2012, xinliangli at gmail dot com wrote:
> > > > 
> > > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=23383
> > > > > 
> > > > > --- Comment #16 from davidxl <xinliangli at gmail dot com> 2012-01-04 00:28:55 UTC ---
> > > > > A related topic - aliasing property of realloc -- the malloc attribute is not
> > > > > applied in the glibc header and the comment is like
> > > > > 
> > > > > /* __attribute_malloc__ is not used, because if realloc returns
> > > > >    the same pointer that was passed to it, aliasing needs to be allowed
> > > > >    between objects pointed by the old and new pointers.  *
> > > > > 
> > > > > 
> > > > > It is true that the realloc can return an address is physically aliased with
> > > > > the pointer passed to it -- but assuming no-alias by the compiler should cause
> > > > > no harm -- as all code motions/CSEs across the realloc call will not be
> > > > > possible because realloc may modify/use the memory location.
> > > > > 
> > > > > 
> > > > > Any comment on this? 
> > > > 
> > > > The malloc attribute assumes that the contents of the memory pointed
> > > > to by the return value is undefined, so the comment is inaccurate
> > > > but the malloc attribute can indeed be not used.
> > > 
> > > Which part of the optimizer takes advantage of the 'undefinedness' of returned
> > > memory?
> > 
> > points-to analysis.  It assumes that the returned blob of memory
> > points to nothing (yet).  So for
> > 
> >  int i;
> >  int **p = malloc (8);
> >  *p = &i;
> >  int **q = realloc (p, 8);
> > 
> > you'd get that *q points to nothing insteda of i.
> 
> The malloc attribute documentation is confusing:
> 
> malloc
> The malloc attribute is used to tell the compiler that a function may be
> treated as if any non-NULL pointer it returns cannot alias any other pointer
> valid when the function returns. This will often improve optimization. Standard
> functions with this property include malloc and calloc. realloc-like functions
> have this property as long as the old pointer is never referred to (including
> comparing it to the new pointer) after the function returns a non-NULL value. 

Ugh, it should not mention realloc here.

> It does not mention the undefineness explicitly.
> 
> It might be better to fix the semantics of this attribute to only specify the
> aliasing property so that it can also be applied to realloc. Points-to needs to
> be fixed to special case realloc for the initialization.

It can't be "fixed" to not assume the undefinedness and still be useful.
For any non-builtin with the malloc attribute it would need to assume
the pointed to memory points to anything.  That will be pretty useless
once you put pointers in the allocated memory.

I'll fixup the documentation.

*** Bug 57823 has been marked as a duplicate of this bug. ***

> That said, it would be fine to add support for this
> under a non-standards-mode option of some sort of course.

C++14 allows to merge and remove global allocations [expr.new]:

"An implementation is allowed to omit a call to a replaceable global allocation function (21.6.2.1, 21.6.2.2).
When it does so, the storage is instead provided by the implementation or provided by extending the allocation
of another new-expression."


Moreover, clang already does the elisions: https://godbolt.org/g/jPGuk6

(In reply to Andrew Pinski from comment #0)
> the return is not turned into 1

It is now. I didn't check since when.

Author: marxin
Date: Thu Jul 25 09:36:38 2019
New Revision: 273791

URL: https://gcc.gnu.org/viewcvs?rev=273791&root=gcc&view=rev
Log:
Extend DCE to remove unnecessary new/delete-pairs (PR c++/23383).

2019-07-25  Martin Liska  <mliska@suse.cz>
	    Dominik Infuhr  <dominik.infuehr@theobroma-systems.com>

	PR c++/23383
	* common.opt: Add -fallocation-dce
	* gimple.c (gimple_call_operator_delete_p): New.
	* gimple.h (gimple_call_operator_delete_p): Likewise.
	* tree-core.h (enum function_decl_type): Add OPERATOR_DELETE.
	* tree-ssa-dce.c (mark_stmt_if_obviously_necessary): Handle
	DECL_IS_OPERATOR_DELETE_P.
	(mark_all_reaching_defs_necessary_1): Likewise.
	(propagate_necessity): Likewise.
	(eliminate_unnecessary_stmts): Handle
	gimple_call_operator_delete_p.
	* tree-streamer-in.c (unpack_ts_function_decl_value_fields):
	Add packing of OPERATOR_DELETE.
	* tree-streamer-out.c (pack_ts_function_decl_value_fields):
	Similarly here.
	* tree.h (DECL_IS_OPERATOR_DELETE_P): New.
	(DECL_SET_IS_OPERATOR_DELETE): New.
	(DECL_IS_REPLACEABLE_OPERATOR_NEW_P): Likewise.
2019-07-25  Martin Liska  <mliska@suse.cz>
	    Dominik Infuhr  <dominik.infuehr@theobroma-systems.com>

	PR c++/23383
	* c-decl.c (merge_decls): Merge OPERATOR_DELETE flag.
2019-07-25  Martin Liska  <mliska@suse.cz>
	    Dominik Infuhr  <dominik.infuehr@theobroma-systems.com>

	PR c++/23383
	* decl.c (cxx_init_decl_processing): Mark delete operators
	with DECL_SET_IS_OPERATOR_DELETE.
2019-07-25  Martin Liska  <mliska@suse.cz
	    Dominik Infuhr  <dominik.infuehr@theobroma-systems.com>

	PR c++/23383
	* g++.dg/cpp1y/new1.C: New test.
2019-07-25  Martin Liska  <mliska@suse.cz>
	    Dominik Infuhr  <dominik.infuehr@theobroma-systems.com>

	PR c++/23383
	* testsuite/ext/bitmap_allocator/check_delete.cc: Add
	-fno-allocation-dce.
	* testsuite/ext/bitmap_allocator/check_new.cc: Likewise.
	* testsuite/ext/new_allocator/check_delete.cc: Likewise.
	* testsuite/ext/new_allocator/check_new.cc: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/cpp1y/new1.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c/ChangeLog
    trunk/gcc/c/c-decl.c
    trunk/gcc/common.opt
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/gimple.c
    trunk/gcc/gimple.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-core.h
    trunk/gcc/tree-ssa-dce.c
    trunk/gcc/tree-streamer-in.c
    trunk/gcc/tree-streamer-out.c
    trunk/gcc/tree.h
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/testsuite/ext/bitmap_allocator/check_delete.cc
    trunk/libstdc++-v3/testsuite/ext/bitmap_allocator/check_new.cc
    trunk/libstdc++-v3/testsuite/ext/new_allocator/check_delete.cc
    trunk/libstdc++-v3/testsuite/ext/new_allocator/check_new.cc

Fixed for GCC 10.

(In reply to Jason Merrill from comment #29)
> Fixed for GCC 10.

I'm closing as such then (since I don't see any mentions of backports needed)