#include <iostream> /* Basic design concept is that WorldObject implements remote method call functionality using the "curiously recurring template pattern" to enable forwarding calls from the generic base class that implements the transport layer to the derived class. The specific failure was in forwarding calls to items in a container. This is emulated here by wrapping a single item. In the main program there are two forms of the call. In the last (uncommented) form the member function pointer is for clarity assigned to a variable (itemfunptr) before making the call. With 4.3.0 and 4.3.1 this code compiles incorrectly with -O1 or greater to produce this warning reproduce.cc: In function ‘int main()’: reproduce.cc:26: warning: ‘itemfunptr.void (Container::*)(void (Item::*)(int), int)::__pfn’ is used uninitialized in this function reproduce.cc:47: note: ‘itemfunptr.void (Container::*)(void (Item::*)(int), int)::__pfn’ was declared here and the resulting executable segvs. It works correctly with -O0. With 4.2.3 and earlier it works correctly with optimization. In the first (commented out) form of the call in the main program we directly refer to desired member function. This compiles and executes correctly with all tested versions. */ template <class Derived> struct WorldObject { template <typename memfunT, typename arg1T, typename arg2T> void forward(memfunT memfun, arg1T arg1, arg2T arg2) { Derived* obj = static_cast<Derived*>(this); (obj->*memfun)(arg1, arg2); } }; struct Item { void fred(int a) { std::cout << "a=" << a << std::endl; } }; struct Container : public WorldObject<Container> { Item item; template <typename itemfunT, typename arg1T> void itemfun(itemfunT itemfun, int a) { (item.*itemfun)(a); } }; int main() { typedef void (Item::*itemfun)(int); Container t; // This call compiles and executes correctly with all versions tested //t.forward(&Container::itemfun<itemfun,int>, &Item::fred, 1); // This call compiles with a warning and segvs on execution if using // -O1 or greater with 4.3.*. 4.2.* is correct. void (Container::*itemfunptr)(itemfun, int) = &Container::itemfun<itemfun,int>; t.forward(itemfunptr, &Item::fred, 1); return 0; }
Confirmed. SRA messing up again when there is different FIELD decls. itemfunptrD.21695.__pfnD.21700 ={v} itemfunD.21696; memfunD.24341 ={v} itemfunptrD.21695; memfun$__pfnD.24346_4 = memfunD.24341.__pfnD.21692; You can see how the two different field decls are allocated. I don't know if I want to mark this a front bug or a middle one.
Uhm. Lovely SRA.
Ok, so SRA does - itemfunptrD.2084.__pfnD.2089 ={v} itemfunD.2085; - itemfunptrD.2084.__deltaD.2082 ={v} 0; - memfunD.2158 ={v} itemfunptrD.2084; + itemfunptr$__pfnD.2176_1 = itemfunD.2085; + itemfunptr$__deltaD.2175_21 = 0; + memfun$__pfnD.2180_25 = itemfunptr$__pfnD.2177_24(D); + memfun$__deltaD.2181_26 = itemfunptr$__deltaD.2175_21; using a new scalar element for itemfunptr$__pfn instead of the just-assigned one (which means, somehow the element lookup doesn't work). I'll investigate.
This is yet another case of the C++ frontend using two different record types for the same PFN structure. For example we have <component_ref 0x7ffff7ec18c0 ... arg 0 <var_decl 0x7ffff759c8c0 itemfunptr type <record_type 0x7ffff75a5300 sizes-gimplified type_6 BLK ... arg 1 <field_decl 0x7ffff759c960 __pfn type <pointer_type 0x7ffff75a56c0> ... bit offset <integer_cst 0x7ffff7edc300 constant 0> context <record_type 0x7ffff75a5780> where you can see the type of the variable indexed is not of the same type as the field context type of the field we index it by ... Because TYPE_GET_PTRMEMFUNC_TYPE for itemfun is a different type than the type of itemfunptr, so we build a "wrong" PTRMEM_CST object. The following "fixes" this, but I'd like to have a C++ maintainer have a look here... (there doesn't seem to exist an easy way to fix or workaround this in SRA, but eventually gimplification of the CONSTRUCTOR could insert a conversion) Index: cp/class.c =================================================================== --- cp/class.c (revision 138552) +++ cp/class.c (working copy) @@ -6197,7 +6197,7 @@ resolve_address_of_overloaded_function ( } if (TYPE_PTRFN_P (target_type) || TYPE_PTRMEMFUNC_P (target_type)) - return cp_build_unary_op (ADDR_EXPR, fn, 0, flags); + return make_ptrmem_cst (target_type, fn); else { /* The target must be a REFERENCE_TYPE. Above, cp_build_unary_op
I'm testing that patch, but I'm sure some side-effects of cp_build_unary_op are necessary...
Of course it didn't work. The following seems to Index: gcc/cp/class.c =================================================================== --- gcc/cp/class.c (revision 138611) +++ gcc/cp/class.c (working copy) @@ -6197,7 +6197,12 @@ resolve_address_of_overloaded_function ( } if (TYPE_PTRFN_P (target_type) || TYPE_PTRMEMFUNC_P (target_type)) - return cp_build_unary_op (ADDR_EXPR, fn, 0, flags); + { + tree tem = cp_build_unary_op (ADDR_EXPR, fn, 0, flags); + if (TREE_CODE (tem) == PTRMEM_CST) + return make_ptrmem_cst (target_type, fn); + return tem; + } else { /* The target must be a REFERENCE_TYPE. Above, cp_build_unary_op we might be able to unconditionally do return make_ptrmem_cst (target_type, fn); for TYPE_PTRMEMFUNC_P (target_type) though.
Causes FAIL: g++.dg/template/ptrmem8.C (test for errors, line 18) FAIL: g++.dg/template/ptrmem8.C (test for errors, line 19) Jason - can you help here? Thanks.
In general, it seems like SRA needs to be more conservative with TYPE_STRUCTURAL_EQUALITY_P types such as these.
However, it's not clear to me that pointers to member functions need to have TYPE_STRUCTURAL_EQUALITY_P in general; I'm testing a patch to set TYPE_CANONICAL appropriately.
The middle-end expects that if there is variable of record type T that for a component-reference it can walk its fields and find the one used as operand 1 in the component-reference. If you look at the tree in commend #4 as it reaches the middle-end then this is not the case. SRA tries to deal with this by doing case FIELD_DECL: /* Fields are unique within a record, but not between compatible records. */ if (DECL_FIELD_CONTEXT (ae) == DECL_FIELD_CONTEXT (be)) return false; return fields_compatible_p (ae, be); but fields_compatible_p returns false because if (!types_compatible_p (TREE_TYPE (f1), TREE_TYPE (f2))) return false; returns false.
Created attachment 16017 [details] Patch to check TYPE_STRUCTURAL_EQUALITY_P in useless_type_conversion
Well, that's the problem then: we have two POINTER_TYPE to METHOD_TYPE which are considered equivalent in the front end, but not equivalent in the tree-ssa types_compatible_p. The specific bug seems to be that useless_type_conversion_1 doesn't expect to need to do structural comparison of non-aggregate types. That's an invalid assumption; it should check TYPE_STRUCTURAL_EQUALITY_P. Does the attached patch seem appropriate to you? It fixes the bug for me, though I haven't regression tested it.
Subject: Re: [4.3/4.4 Regression] member function pointer failure with optimization On Mon, 4 Aug 2008, jason at gcc dot gnu dot org wrote: > ------- Comment #12 from jason at gcc dot gnu dot org 2008-08-04 21:03 ------- > Well, that's the problem then: we have two POINTER_TYPE to METHOD_TYPE which > are considered equivalent in the front end, but not equivalent in the tree-ssa > types_compatible_p. > > The specific bug seems to be that useless_type_conversion_1 doesn't expect to > need to do structural comparison of non-aggregate types. That's an invalid > assumption; it should check TYPE_STRUCTURAL_EQUALITY_P. Does the attached > patch seem appropriate to you? It fixes the bug for me, though I haven't > regression tested it. Duh, if TYPE_STRUCTURAL_EQUALITY_P is used on non-aggregates then yes, this is a correct patch (the code currently makes FUNCTION_TYPEs and METHOD_TYPEs distinct if they are not pointer-equivalent or linked via TYPE_CANONICAL / TYPE_MAIN_VARIANT). So I would prefer if you add those to the AGGREGATE_TYPE_P case instead of letting it fall-through to the "default" case - we should probably add some gcc_unreachable () to make sure we handled all cases. Thanks, Richard.
Subject: Re: [4.3/4.4 Regression] member function pointer failure with optimization On Mon, 4 Aug 2008, rguenther at suse dot de wrote: > ------- Comment #13 from rguenther at suse dot de 2008-08-04 21:11 ------- > Subject: Re: [4.3/4.4 Regression] member function pointer > failure with optimization > > So I would prefer if > you add those to the AGGREGATE_TYPE_P case instead of letting > it fall-through to the "default" case - we should probably add > some gcc_unreachable () to make sure we handled all cases. On a second thought this is probably not so good, but a /* Fallthrough. */ comment would be nice for the AGGREGATE_TYPE_P case. Thanks, Richard.
Subject: Bug 37016 Author: jason Date: Tue Aug 5 22:22:00 2008 New Revision: 138740 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138740 Log: PR c++/37016 * tree-ssa.c (useless_type_conversion_p_1): Call langhook if TYPE_STRUCTURAL_EQUALITY_P is true for both types. Added: trunk/gcc/testsuite/g++.dg/opt/pmf1.C Modified: trunk/gcc/ChangeLog trunk/gcc/tree-ssa.c
Subject: Bug 37016 Author: jason Date: Wed Aug 6 01:54:31 2008 New Revision: 138755 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138755 Log: PR c++/37016 * tree-ssa.c (useless_type_conversion_p_1): Call langhook if TYPE_STRUCTURAL_EQUALITY_P is true for both types. Added: branches/gcc-4_3-branch/gcc/testsuite/g++.dg/opt/pmf1.C - copied unchanged from r138740, trunk/gcc/testsuite/g++.dg/opt/pmf1.C Modified: branches/gcc-4_3-branch/gcc/ChangeLog branches/gcc-4_3-branch/gcc/tree-ssa.c
Subject: Bug 37016 Author: jason Date: Wed Aug 6 02:25:20 2008 New Revision: 138756 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=138756 Log: PR c++/37016 * decl.c (build_ptrmemfunc_type): Don't require structural comparison of PMF types. * tree.c (cp_build_qualified_type_real): Don't clear a valid TYPE_PTRMEMFUNC_TYPE. * typeck.c (cp_build_unary_op): Still do build_ptrmemfunc in templates. Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/decl.c trunk/gcc/cp/tree.c trunk/gcc/cp/typeck.c
Fixed in two different ways.