Created attachment 32192 [details] File that exhibits the compilation error. When compiled with GCC 4.9, my C++ code gives: pure virtual method called terminate called without an active exception Aborted This goes away if I compile with -fno-devirtualize, or if I compile with GCC 4.8.2. I compiled with: % g++-4.9 -O3 main.C -std=c++11 -g % ./a.out
Confirmed.
Mine. It seems that the cxa_pure_virtual actually happens as a result of fre propagation, possible_polymorphic_call_targets always returns 2 possible targets. I wonder how we trigger this.
OK, it happens by ipa_intraprocedural_devirtualization, but since I was nasty enough to assign the other bug to Martin today, I will look into this one.
OK, I am re-considering my decision to not assign this to Martin. The problem is the following. We have call: struct Box x; ... x = edges_connecting_to_node (1); [return slot optimization] ... _19 = OBJ_TYPE_REF(_18;(const struct Object)&x.D.8084->0) (&x.D.8084); The dynamic type of x at that point is Box. We however get it wrong as Object. This is what come from detect_type_change. The reason is that detect_type_change actually ignores x = edges_connecting_to_node (1); [return slot optimization] which it should not, since it gives it an useful information that x is fully constructed when the return value happens. But it considers other statement: MEM[(struct new_allocator *)&x + 8B] ={v} {CLOBBER}; MEM[(struct allocator *)&x + 8B] ={v} {CLOBBER}; MEM[(struct _Vector_impl *)&x + 8B] ={v} {CLOBBER}; MEM[(struct _Vector_base *)&x + 8B] ={v} {CLOBBER}; MEM[(struct vector *)&x + 8B] ={v} {CLOBBER}; MEM[(struct Object *)&x]._vptr.Object = &MEM[(void *)&_ZTV6Object + 16B]; MEM[(struct Object *)&x] ={v} {CLOBBER}; x ={v} {CLOBBER}; which is end of the loop the whole thing is contained in. The dead store to ._vptr.Object come from inlined destructor and it makes detect_type_change to believe that the dynamic type is Object. That is true if you manage to ignore the initialization. Now I wonder how to fix this; simple fix is to make detect_type_change to notice the call and constructors, that is useful by itself. But I believe there is deeper problem, we need to prove that on _all_ paths to the statement the dynamic type was changed in known way, not that on all paths where we can understand the dynamic change the type changed same way. Can alias oracle walker tell us when it runs into default def?
Created attachment 32231 [details] WIP patch Hi, I actually had code around to detect type known from function call, so it is easy to plug it in and solve the testcase. I am not convinced this is safe - I think we may miss the dynamic type setting statement and then we may give wrong answer. Basically we want to know if on _all_ or none paths we encoutered the type setting statemnt. I do not think the alias oracle walker can tell me if it reached top of function? Honza
(In reply to Jan Hubicka from comment #4) > OK, I am re-considering my decision to not assign this to Martin. > The problem is the following. We have call: > > struct Box x; > ... > x = edges_connecting_to_node (1); [return slot optimization] > ... > _19 = OBJ_TYPE_REF(_18;(const struct Object)&x.D.8084->0) (&x.D.8084); > > The dynamic type of x at that point is Box. We however get it wrong as > Object. This is what come from detect_type_change. > > The reason is that detect_type_change actually ignores > x = edges_connecting_to_node (1); [return slot optimization] > which it should not, since it gives it an useful information that x is fully > constructed when the return value happens. > > But it considers other statement: > MEM[(struct new_allocator *)&x + 8B] ={v} {CLOBBER}; > MEM[(struct allocator *)&x + 8B] ={v} {CLOBBER}; > MEM[(struct _Vector_impl *)&x + 8B] ={v} {CLOBBER}; > MEM[(struct _Vector_base *)&x + 8B] ={v} {CLOBBER}; > MEM[(struct vector *)&x + 8B] ={v} {CLOBBER}; > MEM[(struct Object *)&x]._vptr.Object = &MEM[(void *)&_ZTV6Object + 16B]; > MEM[(struct Object *)&x] ={v} {CLOBBER}; > x ={v} {CLOBBER}; > > which is end of the loop the whole thing is contained in. The dead store to > ._vptr.Object come from inlined destructor and it makes detect_type_change > to believe that the dynamic type is Object. That is true if you manage to > ignore the initialization. > > Now I wonder how to fix this; simple fix is to make detect_type_change to > notice the call and constructors, that is useful by itself. > But I believe there is deeper problem, we need to prove that on _all_ paths > to the statement the dynamic type was changed in known way, not that on all > paths where we can understand the dynamic change the type changed same way. It seems that the current code doesn't properly perform that "merging". The walker will happily walks all incoming edges of PHIs. > Can alias oracle walker tell us when it runs into default def? Well, it simply stops walking, so no, it doesn't return whether the callback returned always false (I didn't need it so I didn't implement it ...).
Of course the bug seems to be static bool stmt_may_be_vtbl_ptr_store (gimple stmt) { if (is_gimple_call (stmt)) return false; ^^^ this. I remember being very curious when seeing this ;) Please at this point only fix that bug, not introduce more fancy stuff (we're long past a stage where that is appropriate).
Created attachment 32235 [details] Better WIP patch Yep, ignoring the calls also surprised me. I spent some time trying to think of and produce more evil testcases to this issue, but rest of Martin's reasoning seems right. The code really just mixes up the case where it knows something on all understood places with the case it knows something on all possible paths through the code. It implements the first but wants the second. I also tried to give up on all calls first. That makes the intraprocedural and part of inter-procedural devirt to give up almost always. Except for trivial cases that are handled by SCCVN you almost always have a call in a way, so testsuite is not really happy about it. Other option is to simply give up on recording anything when type change is seen. This also fixes the bug, perhaps with less fallout. Third option is to record if walk ever reached top of function and if it did give up when type change is seen. This seems to be simple addition to ao walker, we just need some interface for it. Fourth option is to be smarter about the calls defining type that is something I would like to do for next stage1, patch attached. It is ortoghonal to 1/2/3, if we have it we will just understand more paths (and currently I punt if I see some not understood path) I can get some data on difference in between options 1,2,3 for mainline, or do we just want to give up?
OK, this is what I am testing for mainline now: Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 208247) +++ ipa-prop.c (working copy) @@ -572,7 +572,12 @@ static bool stmt_may_be_vtbl_ptr_store (gimple stmt) { if (is_gimple_call (stmt)) - return false; + { + return ((gimple_call_lhs (stmt) + && AGGREGATE_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt)))) + || (gimple_call_fndecl (stmt) + && DECL_CXX_CONSTRUCTOR_P (gimple_call_fndecl (stmt)))); + } else if (gimple_clobber_p (stmt)) return false; else if (is_gimple_assign (stmt)) This will make us to punt on about every ctor sequence except case where everything got early inlined. I hope to get some firefox numbers - I don't expecct it to be that bad. ipa-devirt made us less dependent on type change detection and type change detection was never too strong anyway. other option I did not mention is to simply revert: 2013-12-14 Jan Hubicka <jh@suse.cz> PR middle-end/58477 * ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers. this change made the bug to trigger: by punting on all clobbers one can not mix construction and destruction within the loop for automatic vars. I spent some time playing with this and I think those are only ones that matters. It seems that most of checks for dynamic type change for function parameters are not that useful based on Jason's comment that once you get a non-pod built you can not in-place new it to something else. Because we don't track heap allocated objects the type is given by the type of decl the instance lives in. Reverting that patch seems however rather symptomatic fix. This code is on my TODO to rework and integrate into ipa-devirt for next stage1 anyway.
Author: hubicka Date: Sun Mar 2 20:51:48 2014 New Revision: 208261 URL: http://gcc.gnu.org/viewcvs?rev=208261&root=gcc&view=rev Log: PR ipa/60306 Revert: 2013-12-14 Jan Hubicka <jh@suse.cz> PR middle-end/58477 * ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers. * testsuite/g++.dg/ipa/devirt-29.C: New testcase Added: trunk/gcc/testsuite/g++.dg/ipa/devirt-29.C Modified: trunk/gcc/ChangeLog trunk/gcc/ipa-prop.c trunk/gcc/testsuite/ChangeLog
Fixed, even though for next stage1 we need more work.