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.
Related to PR29286.
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; }
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).
-O fails with -fstrict-aliasing as well.
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.
Yes, this was an idea I had as well.
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.
Confirmed.
Ian, is there any chance you can look at this? Your fix for PR29286 could be extended to handle this as well, right?
Created attachment 14953 [details] Possible patch This untested patch fixes the problem with the test case.
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.
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
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.
This is now fixed.
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