GCC Bugzilla has been upgraded from version 4.4.9 to 5.0rc3. If you see any problem, please report it to bug 64968.
Bug 33407 - [4.1/4.3 Regression] C++ operator new and new expression do not change dynamic type
Summary: [4.1/4.3 Regression] C++ operator new and new expression do not change dynami...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.3.0
: P1 normal
Target Milestone: 4.3.0
Assignee: Not yet assigned to anyone
URL:
Keywords: alias, wrong-code
Depends on: 29286
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-12 09:59 UTC by Richard Biener
Modified: 2008-01-18 16:17 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 2.95.4 4.2.1
Known to fail: 3.3.6 3.4.6 4.0.4 4.1.2 4.3.0
Last reconfirmed: 2008-01-02 12:57:34


Attachments
Possible patch (1.43 KB, patch)
2008-01-16 22:49 UTC, Ian Lance Taylor
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2007-09-12 09:59:44 UTC
Nothing in the IL of the following testcase prevents the stores
to *q and *r in function doit from being reordered:

extern "C" void * malloc(__SIZE_TYPE__);
extern "C" void abort(void);

void *p;
void __attribute__((noinline)) init(void)
{
  p = malloc(4);
}

inline void *operator new(__SIZE_TYPE__)
{
  return p;
}

inline void operator delete (void*) {}

int * __attribute__((noinline)) doit(void)
{
  float *q = new float;
  *q = 1.0;
  delete q;
  int *r = new int;
  *r = 1;
  return r;
}

int main()
{
  if (*doit() != 1)
    abort();
  return 0;
}

from the first alias run results:

int* doit() ()
{
  void * D.1643;
  void * D.1643;
  void * D.1639;
  void * D.1639;
  int * r;
  float * q;

<bb 2>:
  # VUSE <p_14(D)>
  D.1639_7 = p;
  q_2 = (float *) D.1639_7;
  # SMT.6_16 = VDEF <SMT.6_15(D)>
  *q_2 = 1.0e+0;
  # VUSE <p_14(D)>
  D.1643_8 = p;
  r_4 = (int *) D.1643_8;
  # SMT.7_18 = VDEF <SMT.7_17(D)>
  *r_4 = 1;
  return r_4;

}

One way to fix this is to make sure that if operator new is inlined
we insert a CHANGE_DYNAMIC_TYPE_EXPR.
Comment 1 Richard Biener 2007-09-12 10:00:49 UTC
Related to PR29286.
Comment 2 Richard Biener 2007-09-12 10:13:56 UTC
main should call init(), but it doesn't make a difference for the IL.  The
bug is wrong-IL for me only at the moment, but nothing prevents the two stores
from being reordered.

Here's one that abort()s at runtime on i686 with -O2 for me:  (same trick
as in PR29286, trick LIM into reordering the stores :))

extern "C" void * malloc(__SIZE_TYPE__);
extern "C" void abort(void);

void *p;
void __attribute__((noinline)) init(void)
{
  p = malloc(4);
}

inline void *operator new(__SIZE_TYPE__)
{
  return p;
}

inline void operator delete (void*) {}

int * __attribute__((noinline)) doit(int n)
{
  float *q;
  int *r;

  for (int i=0; i<n; ++i)
  {
    q = new float;
    *q = 1.0;
    delete q;
    r = new int;
    *r = 1;
  }

  return r;
}

int main()
{
  init();
  if (*doit(1) != 1)
    abort();
  return 0;
}
Comment 3 Richard Biener 2007-09-12 10:18:48 UTC
4.2 works by luck as we weakened aliasing by the NONLOCAL stuff.  2.95 works
for whatever reason ;)  Even pre-tree-ssa we fail with -O2 (but it works with
-O).
Comment 4 Richard Biener 2007-09-12 10:20:54 UTC
-O fails with -fstrict-aliasing as well.
Comment 5 Jakub Jelinek 2007-09-12 16:13:23 UTC
Could we limit adding of the CHANGE_DYNAMIC_TYPE_EXPRs just to the case
where operator new or __attribute__((malloc)) marked FUNCTION_DECL is not
external?  That would be solid even for LTO, if you LTO and have say malloc
implemented among the stuff you read in, then we need to handle that carefully,
but if it is for the compiler just a black box in libc, there is no need
to pollute the IL.
Comment 6 Richard Biener 2007-09-13 09:36:54 UTC
Yes, this was an idea I had as well.
Comment 7 Mark Mitchell 2007-10-10 17:58:50 UTC
We really need to fix this class of problems.  Every release of GCC over the past couple of years has had serious aliasing issues that caused real-world programs to fall over.  We can fix this by making the compiler smarter (teaching it more about what can alias) or dumber (by making it do less with aliasing information), but we need to do one or the other.
Comment 8 Richard Biener 2008-01-02 12:57:34 UTC
Confirmed.
Comment 9 Richard Biener 2008-01-16 10:52:05 UTC
Ian, is there any chance you can look at this?  Your fix for PR29286 could be
extended to handle this as well, right?
Comment 10 Ian Lance Taylor 2008-01-16 22:49:30 UTC
Created attachment 14953 [details]
Possible patch

This untested patch fixes the problem with the test case.
Comment 11 Richard Biener 2008-01-18 12:43:33 UTC
The patch should indeed work and I suggest we go forward with it for 4.3.

For 4.4, can we use this sort of flag (name it no_tbaa_for_result) to handle
both the operator new and the placement new case where for the latter we
at the moment do the CHANGE_DYNAMIC_TYPE_EXPR thing?  After all, the
placement new also gets inlined from its libstdc++ implementation.
Comment 12 ian@gcc.gnu.org 2008-01-18 15:25:47 UTC
Subject: Bug 33407

Author: ian
Date: Fri Jan 18 15:25:02 2008
New Revision: 131629

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131629
Log:
	PR c++/33407
./:
	* tree.h (DECL_IS_OPERATOR_NEW): Define.
	(struct tree_function_decl): Add new field operator_new_flag.
	* tree-inline.c (expand_call_inline): When inlining a call to
	operator new, force the return value to go into a variable, and
	set DECL_NO_TBAA_P on that variable.
	* c-decl.c (merge_decls): Merge DECL_IS_OPERATOR_NEW flag.
cp/:
	* decl.c (duplicate_decls): Copy DECL_IS_OPERATOR_NEW flag.
	(grok_op_properties): For NEW_EXPR and VEC_NEW_EXPR set
	DECL_IS_OPERATOR_NEW flag.
testsuite/:
	* g++.dg/init/new26.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/init/new26.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/c-decl.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/decl.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-inline.c
    trunk/gcc/tree.h

Comment 13 Ian Lance Taylor 2008-01-18 16:01:14 UTC
I think you're right.  If the call to placement new is not inlined, and if we don't know anything special about it (which we currently don't), then it seems to me that everything is bound to work OK.  It is only the inlining that makes a difference.

Pity we didn't realize that before.  Still, the heart of CHANGE_DYNAMIC_TYPE_EXPR is compute_tbaa_pruning, and that will remain.  What can be removed is the code in cp/init.c which creates CHANGE_DYNAMIC_TYPE_EXPR and the code in find_func_aliases which sets the no_tbaa_pruning flag.

I have a vague memory that there was some weird test case in PR 29286 which we would need to reconsider.  But I couldn't find it in a quick look, and I'm not sure my memory is correct.
Comment 14 Ian Lance Taylor 2008-01-18 16:17:21 UTC
This is now fixed.
Comment 15 ian@gcc.gnu.org 2008-01-28 19:44:40 UTC
Subject: Bug 33407

Author: ian
Date: Mon Jan 28 19:43:51 2008
New Revision: 131916

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131916
Log:
	PR c++/34862
	PR c++/33407
	* tree-ssa-copyrename.c (copy_rename_partition_coalesce): Don't
	coalesce pointers if they have different DECL_NO_TBAA_P values.
	* tree-ssa-copy.c (may_propagate_copy): Don't propagate copies
	between variables with different DECL_NO_TBAA_P values.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa-copy.c
    trunk/gcc/tree-ssa-copyrename.c