* [PATCH] c++: non-dependent .* folding [PR112427] @ 2023-11-08 21:59 Patrick Palka 2023-11-10 0:28 ` Jason Merrill 0 siblings, 1 reply; 7+ messages in thread From: Patrick Palka @ 2023-11-08 21:59 UTC (permalink / raw) To: gcc-patches; +Cc: jason, Patrick Palka Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for trunk? -- >8 -- Here when building up the non-dependent .* expression, we crash from fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by replacing the problematic piecemeal folding with a single call to cp_fully_fold. PR c++/112427 gcc/cp/ChangeLog: * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and cp_fully_fold instead of fold_build_pointer_plus and fold_convert. gcc/testsuite/ChangeLog: * g++.dg/template/non-dependent29.C: New test. --- gcc/cp/typeck2.cc | 5 ++++- gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 309903afed8..208004221da 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree component, tsubst_flags_t complain) /* Build an expression for "object + offset" where offset is the value stored in the pointer-to-data-member. */ ptype = build_pointer_type (type); - datum = fold_build_pointer_plus (fold_convert (ptype, datum), component); + datum = cp_convert (ptype, datum, complain); + datum = build2 (POINTER_PLUS_EXPR, ptype, + datum, convert_to_ptrofftype (component)); + datum = cp_fully_fold (datum); datum = cp_build_fold_indirect_ref (datum); if (datum == error_mark_node) return error_mark_node; diff --git a/gcc/testsuite/g++.dg/template/non-dependent29.C b/gcc/testsuite/g++.dg/template/non-dependent29.C new file mode 100644 index 00000000000..41bd11ae6b4 --- /dev/null +++ b/gcc/testsuite/g++.dg/template/non-dependent29.C @@ -0,0 +1,13 @@ +// PR c++/112427 + +struct A { int m; void f(); }; +struct B { A a; }; + +template<class T> +void f(B b) { + int A::*pd = &A::m; + b.a.*pd; + + void (A::*pf)() = &A::f; + (b.a.*pf)(); +} -- 2.43.0.rc1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: non-dependent .* folding [PR112427] 2023-11-08 21:59 [PATCH] c++: non-dependent .* folding [PR112427] Patrick Palka @ 2023-11-10 0:28 ` Jason Merrill 2023-11-10 15:12 ` Patrick Palka 0 siblings, 1 reply; 7+ messages in thread From: Jason Merrill @ 2023-11-10 0:28 UTC (permalink / raw) To: Patrick Palka, gcc-patches On 11/8/23 16:59, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk? > > -- >8 -- > > Here when building up the non-dependent .* expression, we crash from > fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an > IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines > expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by > replacing the problematic piecemeal folding with a single call to > cp_fully_fold. > > PR c++/112427 > > gcc/cp/ChangeLog: > > * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and > cp_fully_fold instead of fold_build_pointer_plus and fold_convert. > gcc/testsuite/ChangeLog: > > * g++.dg/template/non-dependent29.C: New test. > --- > gcc/cp/typeck2.cc | 5 ++++- > gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 309903afed8..208004221da 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree component, tsubst_flags_t complain) > /* Build an expression for "object + offset" where offset is the > value stored in the pointer-to-data-member. */ > ptype = build_pointer_type (type); > - datum = fold_build_pointer_plus (fold_convert (ptype, datum), component); > + datum = cp_convert (ptype, datum, complain); > + datum = build2 (POINTER_PLUS_EXPR, ptype, > + datum, convert_to_ptrofftype (component)); We shouldn't need to build the POINTER_PLUS_EXPR at all in template context. OK with that change. Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: non-dependent .* folding [PR112427] 2023-11-10 0:28 ` Jason Merrill @ 2023-11-10 15:12 ` Patrick Palka 2023-11-10 15:28 ` Patrick Palka 0 siblings, 1 reply; 7+ messages in thread From: Patrick Palka @ 2023-11-10 15:12 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Thu, 9 Nov 2023, Jason Merrill wrote: > On 11/8/23 16:59, Patrick Palka wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > trunk? > > > > -- >8 -- > > > > Here when building up the non-dependent .* expression, we crash from > > fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an > > IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines > > expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by > > replacing the problematic piecemeal folding with a single call to > > cp_fully_fold. > > > > PR c++/112427 > > > > gcc/cp/ChangeLog: > > > > * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and > > cp_fully_fold instead of fold_build_pointer_plus and fold_convert. > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/template/non-dependent29.C: New test. > > --- > > gcc/cp/typeck2.cc | 5 ++++- > > gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ > > 2 files changed, 17 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C > > > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > > index 309903afed8..208004221da 100644 > > --- a/gcc/cp/typeck2.cc > > +++ b/gcc/cp/typeck2.cc > > @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree component, > > tsubst_flags_t complain) > > /* Build an expression for "object + offset" where offset is the > > value stored in the pointer-to-data-member. */ > > ptype = build_pointer_type (type); > > - datum = fold_build_pointer_plus (fold_convert (ptype, datum), > > component); > > + datum = cp_convert (ptype, datum, complain); > > + datum = build2 (POINTER_PLUS_EXPR, ptype, > > + datum, convert_to_ptrofftype (component)); > > We shouldn't need to build the POINTER_PLUS_EXPR at all in template context. > OK with that change. Hmm, that seems harmless at first glance, but I noticed build_min_non_dep (called from build_x_binary_op in this case) is careful to propagate TREE_SIDE_EFFECTS of the given tree, and so eliding POINTER_PLUS_EXPR here could potentially mean that the tree we ultimately return from build_x_binary_op when in a template context has TREE_SIDE_EFFECTS not set when it used to. Shall we still elide the POINTER_PLUS_EXPR in a template context despite this? (The TREE_SIDE_EFFECTS propagation in build_min_non_dep was added in r71108 to avoid bogus ahead of time -Wunused-value warnings. But then r105273 later made us stop issuing -Wunused-value warnings ahead of time altogether. So perhaps we don't need to maintain the TREE_SIDE_EFFECTS flag on templated trees at all anymore?) > > Jason > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: non-dependent .* folding [PR112427] 2023-11-10 15:12 ` Patrick Palka @ 2023-11-10 15:28 ` Patrick Palka 2023-11-10 21:36 ` Jason Merrill 0 siblings, 1 reply; 7+ messages in thread From: Patrick Palka @ 2023-11-10 15:28 UTC (permalink / raw) To: Patrick Palka; +Cc: Jason Merrill, gcc-patches On Fri, 10 Nov 2023, Patrick Palka wrote: > On Thu, 9 Nov 2023, Jason Merrill wrote: > > > On 11/8/23 16:59, Patrick Palka wrote: > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > > trunk? > > > > > > -- >8 -- > > > > > > Here when building up the non-dependent .* expression, we crash from > > > fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an > > > IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines > > > expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by > > > replacing the problematic piecemeal folding with a single call to > > > cp_fully_fold. > > > > > > PR c++/112427 > > > > > > gcc/cp/ChangeLog: > > > > > > * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and > > > cp_fully_fold instead of fold_build_pointer_plus and fold_convert. > > > > > gcc/testsuite/ChangeLog: > > > > > > * g++.dg/template/non-dependent29.C: New test. > > > --- > > > gcc/cp/typeck2.cc | 5 ++++- > > > gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ > > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C > > > > > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > > > index 309903afed8..208004221da 100644 > > > --- a/gcc/cp/typeck2.cc > > > +++ b/gcc/cp/typeck2.cc > > > @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree component, > > > tsubst_flags_t complain) > > > /* Build an expression for "object + offset" where offset is the > > > value stored in the pointer-to-data-member. */ > > > ptype = build_pointer_type (type); > > > - datum = fold_build_pointer_plus (fold_convert (ptype, datum), > > > component); > > > + datum = cp_convert (ptype, datum, complain); > > > + datum = build2 (POINTER_PLUS_EXPR, ptype, > > > + datum, convert_to_ptrofftype (component)); > > > > We shouldn't need to build the POINTER_PLUS_EXPR at all in template context. > > OK with that change. > > Hmm, that seems harmless at first glance, but I noticed > build_min_non_dep (called from build_x_binary_op in this case) is > careful to propagate TREE_SIDE_EFFECTS of the given tree, and so eliding > POINTER_PLUS_EXPR here could potentially mean that the tree we > ultimately return from build_x_binary_op when in a template context has > TREE_SIDE_EFFECTS not set when it used to. Shall we still elide the > POINTER_PLUS_EXPR in a template context despite this? > > (The TREE_SIDE_EFFECTS propagation in build_min_non_dep was added in > r71108 to avoid bogus ahead of time -Wunused-value warnings. But then > r105273 later made us stop issuing -Wunused-value warnings ahead of time > altogether. So perhaps we don't need to maintain the TREE_SIDE_EFFECTS > flag on templated trees at all anymore?) IMO it'd be nice to restore ahead of time -Wunused-value warnings; it seems the original motivation for r105273 / PR8057 was to avoid redundantly issuing a warning twice, once ahead of time and once at instantiation time, which we now could do in a better way with warning_suppressed_p etc. If so, then IIUC eliding the POINTER_PLUS_EXPR could mean we'd incorrectly issue a -Wunused-value warning for e.g. 'a.*f()' in a template context? > > > > > Jason > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: non-dependent .* folding [PR112427] 2023-11-10 15:28 ` Patrick Palka @ 2023-11-10 21:36 ` Jason Merrill 2023-11-14 15:43 ` Patrick Palka 0 siblings, 1 reply; 7+ messages in thread From: Jason Merrill @ 2023-11-10 21:36 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 11/10/23 10:28, Patrick Palka wrote: > On Fri, 10 Nov 2023, Patrick Palka wrote: > >> On Thu, 9 Nov 2023, Jason Merrill wrote: >> >>> On 11/8/23 16:59, Patrick Palka wrote: >>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for >>>> trunk? >>>> >>>> -- >8 -- >>>> >>>> Here when building up the non-dependent .* expression, we crash from >>>> fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an >>>> IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines >>>> expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by >>>> replacing the problematic piecemeal folding with a single call to >>>> cp_fully_fold. >>>> >>>> PR c++/112427 >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and >>>> cp_fully_fold instead of fold_build_pointer_plus and fold_convert. >>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> * g++.dg/template/non-dependent29.C: New test. >>>> --- >>>> gcc/cp/typeck2.cc | 5 ++++- >>>> gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ >>>> 2 files changed, 17 insertions(+), 1 deletion(-) >>>> create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C >>>> >>>> diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc >>>> index 309903afed8..208004221da 100644 >>>> --- a/gcc/cp/typeck2.cc >>>> +++ b/gcc/cp/typeck2.cc >>>> @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree component, >>>> tsubst_flags_t complain) >>>> /* Build an expression for "object + offset" where offset is the >>>> value stored in the pointer-to-data-member. */ >>>> ptype = build_pointer_type (type); >>>> - datum = fold_build_pointer_plus (fold_convert (ptype, datum), >>>> component); >>>> + datum = cp_convert (ptype, datum, complain); >>>> + datum = build2 (POINTER_PLUS_EXPR, ptype, >>>> + datum, convert_to_ptrofftype (component)); >>> >>> We shouldn't need to build the POINTER_PLUS_EXPR at all in template context. >>> OK with that change. >> >> Hmm, that seems harmless at first glance, but I noticed >> build_min_non_dep (called from build_x_binary_op in this case) is >> careful to propagate TREE_SIDE_EFFECTS of the given tree, and so eliding >> POINTER_PLUS_EXPR here could potentially mean that the tree we >> ultimately return from build_x_binary_op when in a template context has >> TREE_SIDE_EFFECTS not set when it used to. Shall we still elide the >> POINTER_PLUS_EXPR in a template context despite this? True, we would need build_min_non_dep to also get TREE_SIDE_EFFECTS from the operands. That might be useful in general for similar situations? I also note that convert_to_ptrofftype uses fold_convert, so the new code could have the same problem if the pointer to member operand is also a COMPONENT_REF. Jason ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: non-dependent .* folding [PR112427] 2023-11-10 21:36 ` Jason Merrill @ 2023-11-14 15:43 ` Patrick Palka 2023-11-14 22:29 ` Jason Merrill 0 siblings, 1 reply; 7+ messages in thread From: Patrick Palka @ 2023-11-14 15:43 UTC (permalink / raw) To: Jason Merrill; +Cc: Patrick Palka, gcc-patches On Fri, 10 Nov 2023, Jason Merrill wrote: > On 11/10/23 10:28, Patrick Palka wrote: > > On Fri, 10 Nov 2023, Patrick Palka wrote: > > > > > On Thu, 9 Nov 2023, Jason Merrill wrote: > > > > > > > On 11/8/23 16:59, Patrick Palka wrote: > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK > > > > > for > > > > > trunk? > > > > > > > > > > -- >8 -- > > > > > > > > > > Here when building up the non-dependent .* expression, we crash from > > > > > fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an > > > > > IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines > > > > > expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by > > > > > replacing the problematic piecemeal folding with a single call to > > > > > cp_fully_fold. > > > > > > > > > > PR c++/112427 > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > * typeck2.cc (build_m_component_ref): Use cp_convert, build2 > > > > > and > > > > > cp_fully_fold instead of fold_build_pointer_plus and > > > > > fold_convert. > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > * g++.dg/template/non-dependent29.C: New test. > > > > > --- > > > > > gcc/cp/typeck2.cc | 5 ++++- > > > > > gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ > > > > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C > > > > > > > > > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > > > > > index 309903afed8..208004221da 100644 > > > > > --- a/gcc/cp/typeck2.cc > > > > > +++ b/gcc/cp/typeck2.cc > > > > > @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree > > > > > component, > > > > > tsubst_flags_t complain) > > > > > /* Build an expression for "object + offset" where offset is > > > > > the > > > > > value stored in the pointer-to-data-member. */ > > > > > ptype = build_pointer_type (type); > > > > > - datum = fold_build_pointer_plus (fold_convert (ptype, datum), > > > > > component); > > > > > + datum = cp_convert (ptype, datum, complain); > > > > > + datum = build2 (POINTER_PLUS_EXPR, ptype, > > > > > + datum, convert_to_ptrofftype (component)); > > > > > > > > We shouldn't need to build the POINTER_PLUS_EXPR at all in template > > > > context. > > > > OK with that change. > > > > > > Hmm, that seems harmless at first glance, but I noticed > > > build_min_non_dep (called from build_x_binary_op in this case) is > > > careful to propagate TREE_SIDE_EFFECTS of the given tree, and so eliding > > > POINTER_PLUS_EXPR here could potentially mean that the tree we > > > ultimately return from build_x_binary_op when in a template context has > > > TREE_SIDE_EFFECTS not set when it used to. Shall we still elide the > > > POINTER_PLUS_EXPR in a template context despite this? > > True, we would need build_min_non_dep to also get TREE_SIDE_EFFECTS from the > operands. That might be useful in general for similar situations? > > I also note that convert_to_ptrofftype uses fold_convert, so the new code > could have the same problem if the pointer to member operand is also a > COMPONENT_REF. Ah, makes sense... How does the following look then? I'm not sure if we still want to replace fold_build_pointer_plus with build2 if we're not going to use that code path in a template context? Bootstrapped and regtested on x86_64-pc-linux-gnu. -- >8 -- - Subject: [PATCH] c++: non-dependent .* operand folding [PR112427] Here when building up the non-dependent .* expression, we crash from fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by replacing the problematic piecemeal folding with a single call to cp_fully_fold. Additionally, don't bother building POINTER_PLUS_EXPR in a template context. This means the returned non-dependent tree might not have TREE_SIDE_EFFECTS set when it used to, so we need to make build_min_non_dep compensate for this by propagating TREE_SIDE_EFFECTS from the original arguments like buildN and build_min do, which seems like a generally desirable thing to do anyway. PR c++/112427 gcc/cp/ChangeLog: * tree.cc (build_min_non_dep): Propagate TREE_SIDE_EFFECTS from the original arguments. (build_min_non_dep_call_vec): Likewise. * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and cp_fully_fold instead of fold_build_pointer_plus and fold_convert. gcc/testsuite/ChangeLog: * g++.dg/template/non-dependent29.C: New test. --- gcc/cp/tree.cc | 11 ++++++++++- gcc/cp/typeck2.cc | 6 +++++- gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc index 417c92ba76f..dc4126f935c 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -3601,7 +3601,12 @@ build_min_non_dep (enum tree_code code, tree non_dep, ...) TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (non_dep); for (i = 0; i < length; i++) - TREE_OPERAND (t, i) = va_arg (p, tree); + { + tree x = va_arg (p, tree); + TREE_OPERAND (t, i) = x; + if (x && !TYPE_P (x)) + TREE_SIDE_EFFECTS (t) |= TREE_SIDE_EFFECTS (x); + } va_end (p); return convert_from_reference (t); @@ -3636,6 +3641,10 @@ build_min_non_dep_call_vec (tree non_dep, tree fn, vec<tree, va_gc> *argvec) non_dep = TREE_OPERAND (non_dep, 0); TREE_TYPE (t) = TREE_TYPE (non_dep); TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (non_dep); + if (argvec) + for (tree x : *argvec) + if (x && !TYPE_P (x)) + TREE_SIDE_EFFECTS (t) |= TREE_SIDE_EFFECTS (x); return convert_from_reference (t); } diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc index 309903afed8..a75f4f8c2d2 100644 --- a/gcc/cp/typeck2.cc +++ b/gcc/cp/typeck2.cc @@ -2378,7 +2378,11 @@ build_m_component_ref (tree datum, tree component, tsubst_flags_t complain) /* Build an expression for "object + offset" where offset is the value stored in the pointer-to-data-member. */ ptype = build_pointer_type (type); - datum = fold_build_pointer_plus (fold_convert (ptype, datum), component); + datum = cp_convert (ptype, datum, complain); + if (!processing_template_decl) + datum = build2 (POINTER_PLUS_EXPR, ptype, + datum, convert_to_ptrofftype (component)); + datum = cp_fully_fold (datum); datum = cp_build_fold_indirect_ref (datum); if (datum == error_mark_node) return error_mark_node; diff --git a/gcc/testsuite/g++.dg/template/non-dependent29.C b/gcc/testsuite/g++.dg/template/non-dependent29.C new file mode 100644 index 00000000000..41bd11ae6b4 --- /dev/null +++ b/gcc/testsuite/g++.dg/template/non-dependent29.C @@ -0,0 +1,13 @@ +// PR c++/112427 + +struct A { int m; void f(); }; +struct B { A a; }; + +template<class T> +void f(B b) { + int A::*pd = &A::m; + b.a.*pd; + + void (A::*pf)() = &A::f; + (b.a.*pf)(); +} -- 2.43.0.rc1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] c++: non-dependent .* folding [PR112427] 2023-11-14 15:43 ` Patrick Palka @ 2023-11-14 22:29 ` Jason Merrill 0 siblings, 0 replies; 7+ messages in thread From: Jason Merrill @ 2023-11-14 22:29 UTC (permalink / raw) To: Patrick Palka; +Cc: gcc-patches On 11/14/23 10:43, Patrick Palka wrote: > On Fri, 10 Nov 2023, Jason Merrill wrote: > >> On 11/10/23 10:28, Patrick Palka wrote: >>> On Fri, 10 Nov 2023, Patrick Palka wrote: >>> >>>> On Thu, 9 Nov 2023, Jason Merrill wrote: >>>> >>>>> On 11/8/23 16:59, Patrick Palka wrote: >>>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK >>>>>> for >>>>>> trunk? >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> Here when building up the non-dependent .* expression, we crash from >>>>>> fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an >>>>>> IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines >>>>>> expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by >>>>>> replacing the problematic piecemeal folding with a single call to >>>>>> cp_fully_fold. >>>>>> >>>>>> PR c++/112427 >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> * typeck2.cc (build_m_component_ref): Use cp_convert, build2 >>>>>> and >>>>>> cp_fully_fold instead of fold_build_pointer_plus and >>>>>> fold_convert. >>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> * g++.dg/template/non-dependent29.C: New test. >>>>>> --- >>>>>> gcc/cp/typeck2.cc | 5 ++++- >>>>>> gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ >>>>>> 2 files changed, 17 insertions(+), 1 deletion(-) >>>>>> create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C >>>>>> >>>>>> diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc >>>>>> index 309903afed8..208004221da 100644 >>>>>> --- a/gcc/cp/typeck2.cc >>>>>> +++ b/gcc/cp/typeck2.cc >>>>>> @@ -2378,7 +2378,10 @@ build_m_component_ref (tree datum, tree >>>>>> component, >>>>>> tsubst_flags_t complain) >>>>>> /* Build an expression for "object + offset" where offset is >>>>>> the >>>>>> value stored in the pointer-to-data-member. */ >>>>>> ptype = build_pointer_type (type); >>>>>> - datum = fold_build_pointer_plus (fold_convert (ptype, datum), >>>>>> component); >>>>>> + datum = cp_convert (ptype, datum, complain); >>>>>> + datum = build2 (POINTER_PLUS_EXPR, ptype, >>>>>> + datum, convert_to_ptrofftype (component)); >>>>> >>>>> We shouldn't need to build the POINTER_PLUS_EXPR at all in template >>>>> context. >>>>> OK with that change. >>>> >>>> Hmm, that seems harmless at first glance, but I noticed >>>> build_min_non_dep (called from build_x_binary_op in this case) is >>>> careful to propagate TREE_SIDE_EFFECTS of the given tree, and so eliding >>>> POINTER_PLUS_EXPR here could potentially mean that the tree we >>>> ultimately return from build_x_binary_op when in a template context has >>>> TREE_SIDE_EFFECTS not set when it used to. Shall we still elide the >>>> POINTER_PLUS_EXPR in a template context despite this? >> >> True, we would need build_min_non_dep to also get TREE_SIDE_EFFECTS from the >> operands. That might be useful in general for similar situations? >> >> I also note that convert_to_ptrofftype uses fold_convert, so the new code >> could have the same problem if the pointer to member operand is also a >> COMPONENT_REF. > > Ah, makes sense... How does the following look then? I'm not sure if > we still want to replace fold_build_pointer_plus with build2 if we're > not going to use that code path in a template context? Bootstrapped and > regtested on x86_64-pc-linux-gnu. We probably do, since we now aren't folding the thing before it. OK. > -- >8 -- > - > Subject: [PATCH] c++: non-dependent .* operand folding [PR112427] > > Here when building up the non-dependent .* expression, we crash from > fold_convert on 'b.a' due to this (templated) COMPONENT_REF having an > IDENTIFIER_NODE instead of FIELD_DECL operand that middle-end routines > expect. Like in r14-4899-gd80a26cca02587, this patch fixes this by > replacing the problematic piecemeal folding with a single call to > cp_fully_fold. Additionally, don't bother building POINTER_PLUS_EXPR > in a template context. This means the returned non-dependent tree might > not have TREE_SIDE_EFFECTS set when it used to, so we need to make > build_min_non_dep compensate for this by propagating TREE_SIDE_EFFECTS > from the original arguments like buildN and build_min do, which seems > like a generally desirable thing to do anyway. > > PR c++/112427 > > gcc/cp/ChangeLog: > > * tree.cc (build_min_non_dep): Propagate TREE_SIDE_EFFECTS from > the original arguments. > (build_min_non_dep_call_vec): Likewise. > * typeck2.cc (build_m_component_ref): Use cp_convert, build2 and > cp_fully_fold instead of fold_build_pointer_plus and fold_convert. > > gcc/testsuite/ChangeLog: > > * g++.dg/template/non-dependent29.C: New test. > --- > gcc/cp/tree.cc | 11 ++++++++++- > gcc/cp/typeck2.cc | 6 +++++- > gcc/testsuite/g++.dg/template/non-dependent29.C | 13 +++++++++++++ > 3 files changed, 28 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/template/non-dependent29.C > > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc > index 417c92ba76f..dc4126f935c 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -3601,7 +3601,12 @@ build_min_non_dep (enum tree_code code, tree non_dep, ...) > TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (non_dep); > > for (i = 0; i < length; i++) > - TREE_OPERAND (t, i) = va_arg (p, tree); > + { > + tree x = va_arg (p, tree); > + TREE_OPERAND (t, i) = x; > + if (x && !TYPE_P (x)) > + TREE_SIDE_EFFECTS (t) |= TREE_SIDE_EFFECTS (x); > + } > > va_end (p); > return convert_from_reference (t); > @@ -3636,6 +3641,10 @@ build_min_non_dep_call_vec (tree non_dep, tree fn, vec<tree, va_gc> *argvec) > non_dep = TREE_OPERAND (non_dep, 0); > TREE_TYPE (t) = TREE_TYPE (non_dep); > TREE_SIDE_EFFECTS (t) = TREE_SIDE_EFFECTS (non_dep); > + if (argvec) > + for (tree x : *argvec) > + if (x && !TYPE_P (x)) > + TREE_SIDE_EFFECTS (t) |= TREE_SIDE_EFFECTS (x); > return convert_from_reference (t); > } > > diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc > index 309903afed8..a75f4f8c2d2 100644 > --- a/gcc/cp/typeck2.cc > +++ b/gcc/cp/typeck2.cc > @@ -2378,7 +2378,11 @@ build_m_component_ref (tree datum, tree component, tsubst_flags_t complain) > /* Build an expression for "object + offset" where offset is the > value stored in the pointer-to-data-member. */ > ptype = build_pointer_type (type); > - datum = fold_build_pointer_plus (fold_convert (ptype, datum), component); > + datum = cp_convert (ptype, datum, complain); > + if (!processing_template_decl) > + datum = build2 (POINTER_PLUS_EXPR, ptype, > + datum, convert_to_ptrofftype (component)); > + datum = cp_fully_fold (datum); > datum = cp_build_fold_indirect_ref (datum); > if (datum == error_mark_node) > return error_mark_node; > diff --git a/gcc/testsuite/g++.dg/template/non-dependent29.C b/gcc/testsuite/g++.dg/template/non-dependent29.C > new file mode 100644 > index 00000000000..41bd11ae6b4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/template/non-dependent29.C > @@ -0,0 +1,13 @@ > +// PR c++/112427 > + > +struct A { int m; void f(); }; > +struct B { A a; }; > + > +template<class T> > +void f(B b) { > + int A::*pd = &A::m; > + b.a.*pd; > + > + void (A::*pf)() = &A::f; > + (b.a.*pf)(); > +} ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-14 22:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-08 21:59 [PATCH] c++: non-dependent .* folding [PR112427] Patrick Palka 2023-11-10 0:28 ` Jason Merrill 2023-11-10 15:12 ` Patrick Palka 2023-11-10 15:28 ` Patrick Palka 2023-11-10 21:36 ` Jason Merrill 2023-11-14 15:43 ` Patrick Palka 2023-11-14 22:29 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).