Bug 91859 - Using destroying delete should not clobber stores to the object
Summary: Using destroying delete should not clobber stores to the object
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 9.2.0
: P3 normal
Target Milestone: 11.2
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2019-09-23 10:39 UTC by Domingo Alvarez
Modified: 2021-06-01 16:41 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-09-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Domingo Alvarez 2019-09-23 10:39:43 UTC
When compiling the sample bellow with g++9 (9.1 and 9.2) with optmization -O2 the generated code eliminates a valid and necessary code:
=====
#include <stdio.h>
#include <stdlib.h>

struct Expression {
	Expression *next;
	Expression *right;
	void *operator new(size_t);
	void operator delete(void *);
};

void * Expression::operator new(size_t sz)
{
	return malloc(sz);
}

void Expression::operator delete(void *p)
{
	Expression *e = (Expression*)p;
	printf("%p : %p\n", e, e->next);
	while(e->next)
	{
		Expression *f = e->next;
		//pretend doing something
		printf("delete is doing something with %p\n", e->next);
		e = f;
	}
	free(p);
}

int main()
{
	Expression *etmp, *ebase;
	etmp = ebase = new Expression();
	for(int i = 0; i < 10; ++i)
	{
		etmp->next = new Expression();
		etmp->right = new Expression();
		etmp->right->next = NULL;
		etmp->next->right = new Expression();
		etmp->next->right->next = NULL;
		etmp->next->right->right = NULL;
		etmp = etmp->next;
	}
	etmp->next = NULL;

	Expression *e, *e0, *e1, *efirst, **ep;

	if ((efirst = ebase)) {
		ep = &e0;
		while((e = efirst)) {
			*ep = e1 = e->right;
			efirst = e->next;
			e->next = 0; //!! with -O2 this line is eliminated
			delete e;
			while((e = e1->next))
				e1 = e;
			ep = &e1->next;
		}
	}

	return 0;
}
=====

To build:
=====
g++-8 -g -o gcc8-bug-no-optmization gcc9-bug.cpp
g++-8 -O2 -g -o gcc8-bug-O2 gcc9-bug.cpp

g++-9 -g -o gcc9-bug-no-optmization gcc9-bug.cpp
g++-9 -O2 -g -o gcc9-bug-O2 gcc9-bug.cpp
=====

Output:
=====
./gcc8-bug-no-optmization
0x560315ecc260 : (nil)
0x560315ecc280 : (nil)
0x560315ecc2e0 : (nil)

./gcc8-bug-O2
0x5562bab29260 : (nil)
0x5562bab29280 : (nil)
0x5562bab292e0 : (nil)

./gcc9-bug-no-optmization
0x557550cd0260 : (nil)
0x557550cd0280 : (nil)
0x557550cd02e0 : (nil)

./gcc9-bug-O2
0x5617642e3260 : 0x5617642e3280
delete is doing something with 0x5617642e3280
delete is doing something with 0x5617642e32e0
0x5617642e3280 : 0x5617642e32e0
delete is doing something with 0x5617642e32e0
0x5617642e32e0 : (nil)

=====

The sample code is similar to a production code that do some work inside the custom delete if the "next" pointer is not NULL.

g++-8 and older works as we expect so far only gcc9 (9.1 and 9.2) changed the generated code breaking our build.
Comment 1 Jonathan Wakely 2019-09-23 11:05:43 UTC
(In reply to Domingo Alvarez from comment #0)
> The sample code is similar to a production code that do some work inside the
> custom delete if the "next" pointer is not NULL.

That's not valid. THe operator delete gets called after the object's destructor has run, which means you can't access its members.

You can either fix the code, or use -fno-lifetime-dse to tell GCC to disable the valid optimizations that break your invalid code.
Comment 2 Jonathan Wakely 2019-09-23 11:11:25 UTC
The original report is invalid but the DSE optimization is **not** valid when using C++20 destroying delete:

#include <stdio.h>
#include <stdlib.h>
#include <new>

struct Expression {
  int i = 0;
  void *operator new(size_t);
  void operator delete(Expression *, std::destroying_delete_t);
};

void * Expression::operator new(size_t sz)
{
  return malloc(sz);
}

void Expression::operator delete(Expression *p, std::destroying_delete_t)
{
  Expression * e = p;
  printf("%p : %d\n", e, e->i);
  p->~Expression();
  free(p);
}

int main()
{
  auto p = new Expression();
  p->i = 1;
  delete p;
}

With this feature the compiler doesn't automatically invoke the destructor, so the store to p->i is not dead, and must not be eliminated.

Not a regression though, because destroying delete wasn't supported before GCC 9.
Comment 3 Domingo Alvarez 2019-09-23 11:21:37 UTC
Thank you !
I was suspecting it after report this problem and added a destructor to the sample and then the code behaves as you describe.

Sorry by the noise and thank you again !
Comment 4 Jonathan Wakely 2019-09-25 16:03:29 UTC
Jason, see comment 2 - the original report is invalid.
Comment 5 Joseph C. Sible 2021-05-20 02:07:10 UTC
The real problem mentioned in comment 2 still happens as of GCC 11.1. I've narrowed it down somewhat to the optimization flags "-Og -ftree-dse -ftree-pta". Removing any one of those will make it behave again.
Comment 6 Jonathan Wakely 2021-05-20 06:29:16 UTC
Yes, but those optimizations are just doing what they are designed to do. I suspect that the bug is due to the front end inserting a CLOBBER that says it's ok for those optimizations to do that.
Comment 7 CVS Commits 2021-06-01 15:39:29 UTC
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:cf2b7020ee8e9745ede527b0a3b2e0ffbafd492b

commit r12-1145-gcf2b7020ee8e9745ede527b0a3b2e0ffbafd492b
Author: Jason Merrill <jason@redhat.com>
Date:   Fri May 28 17:05:23 2021 -0400

    c++: no clobber for C++20 destroying delete [PR91859]
    
    Before C++20 added destroying operator delete, by the time we called
    operator delete for a pointer, the object would already be gone.  But that
    isn't true for destroying delete.  Since the optimizers' assumptions about
    operator delete are based on either DECL_IS_REPLACEABLE_OPERATOR (which
    already is not set) or CALL_FROM_NEW_OR_DELETE_P, let's avoid setting the
    latter flag in this case.
    
            PR c++/91859
    
    gcc/ChangeLog:
    
            * tree.h (CALL_FROM_NEW_OR_DELETE_P): Adjust comment.
    
    gcc/cp/ChangeLog:
    
            * call.c (build_op_delete_call): Don't set CALL_FROM_NEW_OR_DELETE_P
            for destroying delete.
            * init.c (build_delete): Don't clobber before destroying delete.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp2a/destroying-delete5.C: New test.
Comment 8 CVS Commits 2021-06-01 15:53:28 UTC
The releases/gcc-11 branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:ee3edeb01eca1cc8d7ebe777b4adb333f0c1118a

commit r11-8495-gee3edeb01eca1cc8d7ebe777b4adb333f0c1118a
Author: Jason Merrill <jason@redhat.com>
Date:   Fri May 28 17:05:23 2021 -0400

    c++: no clobber for C++20 destroying delete [PR91859]
    
    Before C++20 added destroying operator delete, by the time we called
    operator delete for a pointer, the object would already be gone.  But that
    isn't true for destroying delete.  Since the optimizers' assumptions about
    operator delete are based on either DECL_IS_REPLACEABLE_OPERATOR (which
    already is not set) or CALL_FROM_NEW_OR_DELETE_P, let's avoid setting the
    latter flag in this case.
    
            PR c++/91859
    
    gcc/ChangeLog:
    
            * tree.h (CALL_FROM_NEW_OR_DELETE_P): Adjust comment.
    
    gcc/cp/ChangeLog:
    
            * call.c (build_op_delete_call): Don't set CALL_FROM_NEW_OR_DELETE_P
            for destroying delete.
            * init.c (build_delete): Don't clobber before destroying delete.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/cpp2a/destroying-delete5.C: New test.
Comment 9 Jason Merrill 2021-06-01 16:41:17 UTC
Fixed for 11.2/12.