| Message ID | 20190207230259.GI3884@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/89217 - ICE with list-initialization in range-based for loop | expand |
On 2/7/19 6:02 PM, Marek Polacek wrote: > Since r268321 we can call digest_init even in a template, when the compound > literal isn't instantiation-dependent. Right. And since digest_init modifies the CONSTRUCTOR in place, that means the template trees are digested rather than the original parse trees that we try to use. If we're going to use digest_init, we should probably save another CONSTRUCTOR with the original trees. Jason
On Mon, Feb 11, 2019 at 01:43:36PM -0500, Jason Merrill wrote: > On 2/7/19 6:02 PM, Marek Polacek wrote: > > Since r268321 we can call digest_init even in a template, when the compound > > literal isn't instantiation-dependent. > > Right. And since digest_init modifies the CONSTRUCTOR in place, that means > the template trees are digested rather than the original parse trees that we > try to use. If we're going to use digest_init, we should probably save > another CONSTRUCTOR with the original trees. I tried unsharing the constructor and even its contents but only then did I realize that this cannot work. It's not digest_init that adds the problematic INDIRECT_REF via convert_from_reference, it's instantiate_pending_templates -> tsubst_expr -> ... -> finish_non_static_data_member. So the problem isn't sharing the contents of the CONSTRUCTOR, but rather what finish_non_static_data_member does with the {.r=(struct R &) (struct R *) ((struct S *) this)->r} expression. The same problem would appear even before r268321 changes if we called tsubst_* twice on the CONSTRUCTOR above. Do you still think digest_init and/or finish_compound_literal need tweaking? Marek
On 2/11/19 6:03 PM, Marek Polacek wrote: > On Mon, Feb 11, 2019 at 01:43:36PM -0500, Jason Merrill wrote: >> On 2/7/19 6:02 PM, Marek Polacek wrote: >>> Since r268321 we can call digest_init even in a template, when the compound >>> literal isn't instantiation-dependent. >> >> Right. And since digest_init modifies the CONSTRUCTOR in place, that means >> the template trees are digested rather than the original parse trees that we >> try to use. If we're going to use digest_init, we should probably save >> another CONSTRUCTOR with the original trees. > > I tried unsharing the constructor and even its contents but only then did I > realize that this cannot work. Why wouldn't going back to saving {*((struct S *) this)->r} work? > It's not digest_init that adds the problematic > INDIRECT_REF via convert_from_reference, it's instantiate_pending_templates > -> tsubst_expr -> ... -> finish_non_static_data_member. > > So the problem isn't sharing the contents of the CONSTRUCTOR, but rather what > finish_non_static_data_member does with the > > {.r=(struct R &) (struct R *) ((struct S *) this)->r} > > expression. The same problem would appear even before r268321 changes if we > called tsubst_* twice on the CONSTRUCTOR above. Yes, it sounds like there's a bug in that path as well. Perhaps tsubst_copy_and_build/COMPONENT_REF should strip a REFERENCE_REF_P if t was already a reference. > Do you still think digest_init and/or finish_compound_literal need tweaking? I imagine that saving post-digest trees might cause other problems, but perhaps not. Perhaps we ought to move away more generally from trying to save the original parse trees for non-dependent expressions and messing with NON_DEPENDENT_EXPR. Jason
On Fri, Feb 15, 2019 at 01:59:10PM -1000, Jason Merrill wrote: > On 2/11/19 6:03 PM, Marek Polacek wrote: > > On Mon, Feb 11, 2019 at 01:43:36PM -0500, Jason Merrill wrote: > > > On 2/7/19 6:02 PM, Marek Polacek wrote: > > > > Since r268321 we can call digest_init even in a template, when the compound > > > > literal isn't instantiation-dependent. > > > > > > Right. And since digest_init modifies the CONSTRUCTOR in place, that means > > > the template trees are digested rather than the original parse trees that we > > > try to use. If we're going to use digest_init, we should probably save > > > another CONSTRUCTOR with the original trees. > > > > I tried unsharing the constructor and even its contents but only then did I > > realize that this cannot work. > > Why wouldn't going back to saving {*((struct S *) this)->r} work? Sorry, I misunderstood what you meant by "saving". I think I do now. > > It's not digest_init that adds the problematic > > INDIRECT_REF via convert_from_reference, it's instantiate_pending_templates > > -> tsubst_expr -> ... -> finish_non_static_data_member. > > > > So the problem isn't sharing the contents of the CONSTRUCTOR, but rather what > > finish_non_static_data_member does with the > > > > {.r=(struct R &) (struct R *) ((struct S *) this)->r} > > > > expression. The same problem would appear even before r268321 changes if we > > called tsubst_* twice on the CONSTRUCTOR above. > > Yes, it sounds like there's a bug in that path as well. Perhaps > tsubst_copy_and_build/COMPONENT_REF should strip a REFERENCE_REF_P if t was > already a reference. With this patch, this seems no longer to be needed. > > Do you still think digest_init and/or finish_compound_literal need tweaking? > > I imagine that saving post-digest trees might cause other problems, but > perhaps not. Perhaps we ought to move away more generally from trying to > save the original parse trees for non-dependent expressions and messing with > NON_DEPENDENT_EXPR. Now that I've spent a lot of time looking into 89356 (and the other PRs broken by the same revision), I'm convinced that we must return the original tree in finish_compound_literal. But we still have to call digest_init or we lose detecting narrowing conversions. What happens in that PR is that after digest_init we lose the braced-init-list and that changes mangling. I've come up with this fix for 89356, but it also fixes this PR, and likely all the others. The comments hopefully explain what I'm doing and why, the only suspicious thing is the get_target_expr_sfinae call, that is so that initlist109.C keeps compiling; without the call to get_target_expr_sfinae, we end up issuing an error in digest_init_r: 1224 if (COMPOUND_LITERAL_P (stripped_init) && code == ARRAY_TYPE) 1225 { 1226 if (complain & tf_error) 1227 error_at (loc, "cannot initialize aggregate of type %qT with " 1228 "a compound literal", type); But I hope the rest of the patch is reasonable. The LOOKUP_NO_NARROWING bit isn't necessary but it should be a correct thing to do, so that later in perform_implicit_conversion_flags we properly set the recently added flag IMPLICIT_CONV_EXPR_BRACED_INIT. Bootstrapped/regtested on x86_64-linux and ppc64le-linux, ok for trunk? 2019-02-16 Marek Polacek <polacek@redhat.com> PR c++/89217 - ICE with list-initialization in range-based for loop. * constexpr.c (unshare_constructor): No longer static. * cp-tree.h (unshare_constructor): Declare. * semantics.c (finish_compound_literal): When dealing with a non-dependent expression in a template, return the original expression. Pass LOOKUP_NO_NARROWING to digest_init_flags. * g++.dg/cpp0x/range-for37.C: New test. diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c index 923763faa0a..d946a797999 100644 --- gcc/cp/constexpr.c +++ gcc/cp/constexpr.c @@ -1318,7 +1318,7 @@ find_constructor (tree *tp, int *walk_subtrees, void *) /* If T is a CONSTRUCTOR or an expression that has a CONSTRUCTOR node as a subexpression, return an unshared copy of T. Otherwise return T. */ -static tree +tree unshare_constructor (tree t) { tree ctor = walk_tree (&t, find_constructor, NULL, NULL); diff --git gcc/cp/cp-tree.h gcc/cp/cp-tree.h index 44a3620a539..60ca1366cf6 100644 --- gcc/cp/cp-tree.h +++ gcc/cp/cp-tree.h @@ -7710,6 +7710,7 @@ extern void explain_invalid_constexpr_fn (tree); extern vec<tree> cx_error_context (void); extern tree fold_sizeof_expr (tree); extern void clear_cv_and_fold_caches (void); +extern tree unshare_constructor (tree); /* In cp-ubsan.c */ extern void cp_ubsan_maybe_instrument_member_call (tree); diff --git gcc/cp/semantics.c gcc/cp/semantics.c index aa5a163dd64..3ecd192bced 100644 --- gcc/cp/semantics.c +++ gcc/cp/semantics.c @@ -2796,17 +2796,31 @@ finish_compound_literal (tree type, tree compound_literal, return error_mark_node; } - if (instantiation_dependent_expression_p (compound_literal) - || dependent_type_p (type)) + /* Used to hold a copy of the compound literal in a template. */ + tree orig_cl = NULL_TREE; + + if (processing_template_decl) { - TREE_TYPE (compound_literal) = type; + const bool dependent_p + = (instantiation_dependent_expression_p (compound_literal) + || dependent_type_p (type)); + if (dependent_p) + /* We're about to return, no need to copy. */ + orig_cl = compound_literal; + else + /* We're going to need a copy. */ + orig_cl = unshare_constructor (compound_literal); + TREE_TYPE (orig_cl) = type; /* Mark the expression as a compound literal. */ - TREE_HAS_CONSTRUCTOR (compound_literal) = 1; + TREE_HAS_CONSTRUCTOR (orig_cl) = 1; /* And as instantiation-dependent. */ - CONSTRUCTOR_IS_DEPENDENT (compound_literal) = true; + CONSTRUCTOR_IS_DEPENDENT (orig_cl) = dependent_p; if (fcl_context == fcl_c99) - CONSTRUCTOR_C99_COMPOUND_LITERAL (compound_literal) = 1; - return compound_literal; + CONSTRUCTOR_C99_COMPOUND_LITERAL (orig_cl) = 1; + /* If the compound literal is dependent, we're done for now. */ + if (dependent_p) + return orig_cl; + /* Otherwise, do go on to e.g. check narrowing. */ } type = complete_type (type); @@ -2842,8 +2856,18 @@ finish_compound_literal (tree type, tree compound_literal, if (type == error_mark_node) return error_mark_node; } - compound_literal = digest_init_flags (type, compound_literal, LOOKUP_NORMAL, + compound_literal = digest_init_flags (type, compound_literal, + LOOKUP_NORMAL | LOOKUP_NO_NARROWING, complain); + /* If we're in a template, return the original compound literal. */ + if (orig_cl) + { + if (!VECTOR_TYPE_P (type)) + return get_target_expr_sfinae (orig_cl, complain); + else + return orig_cl; + } + if (TREE_CODE (compound_literal) == CONSTRUCTOR) { TREE_HAS_CONSTRUCTOR (compound_literal) = true; diff --git gcc/testsuite/g++.dg/cpp0x/range-for37.C gcc/testsuite/g++.dg/cpp0x/range-for37.C new file mode 100644 index 00000000000..d5c7c091d96 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/range-for37.C @@ -0,0 +1,24 @@ +// PR c++/89217 +// { dg-do compile { target c++11 } } + +struct R {}; + +struct C +{ + R* begin() const { return &r; } + R* end() const { return &r; } + + R& r; +}; + +struct S +{ + void f1() { f2<true>(); } + R& r; + + template<bool> + void f2() + { + for (auto i : C{r}) {} + } +};
On 2/16/19 8:23 PM, Marek Polacek wrote: > On Fri, Feb 15, 2019 at 01:59:10PM -1000, Jason Merrill wrote: >> On 2/11/19 6:03 PM, Marek Polacek wrote: >>> On Mon, Feb 11, 2019 at 01:43:36PM -0500, Jason Merrill wrote: >>>> On 2/7/19 6:02 PM, Marek Polacek wrote: >>>>> Since r268321 we can call digest_init even in a template, when the compound >>>>> literal isn't instantiation-dependent. >>>> >>>> Right. And since digest_init modifies the CONSTRUCTOR in place, that means >>>> the template trees are digested rather than the original parse trees that we >>>> try to use. If we're going to use digest_init, we should probably save >>>> another CONSTRUCTOR with the original trees. >>> >>> I tried unsharing the constructor and even its contents but only then did I >>> realize that this cannot work. >> >> Why wouldn't going back to saving {*((struct S *) this)->r} work? > > Sorry, I misunderstood what you meant by "saving". I think I do now. > >>> It's not digest_init that adds the problematic >>> INDIRECT_REF via convert_from_reference, it's instantiate_pending_templates >>> -> tsubst_expr -> ... -> finish_non_static_data_member. >>> >>> So the problem isn't sharing the contents of the CONSTRUCTOR, but rather what >>> finish_non_static_data_member does with the >>> >>> {.r=(struct R &) (struct R *) ((struct S *) this)->r} >>> >>> expression. The same problem would appear even before r268321 changes if we >>> called tsubst_* twice on the CONSTRUCTOR above. >> >> Yes, it sounds like there's a bug in that path as well. Perhaps >> tsubst_copy_and_build/COMPONENT_REF should strip a REFERENCE_REF_P if t was >> already a reference. > > With this patch, this seems no longer to be needed. > >>> Do you still think digest_init and/or finish_compound_literal need tweaking? >> >> I imagine that saving post-digest trees might cause other problems, but >> perhaps not. Perhaps we ought to move away more generally from trying to >> save the original parse trees for non-dependent expressions and messing with >> NON_DEPENDENT_EXPR. > > Now that I've spent a lot of time looking into 89356 (and the other PRs broken > by the same revision), I'm convinced that we must return the original tree in > finish_compound_literal. But we still have to call digest_init or we lose > detecting narrowing conversions. What happens in that PR is that after > digest_init we lose the braced-init-list and that changes mangling. I've come > up with this fix for 89356, but it also fixes this PR, and likely all the > others. > > The comments hopefully explain what I'm doing and why, the only suspicious > thing is the get_target_expr_sfinae call, that is so that initlist109.C > keeps compiling; without the call to get_target_expr_sfinae, we end up > issuing an error in digest_init_r: > 1224 if (COMPOUND_LITERAL_P (stripped_init) && code == ARRAY_TYPE) > 1225 { > 1226 if (complain & tf_error) > 1227 error_at (loc, "cannot initialize aggregate of type %qT with " > 1228 "a compound literal", type); > > But I hope the rest of the patch is reasonable. The LOOKUP_NO_NARROWING bit > isn't necessary but it should be a correct thing to do, so that later in > perform_implicit_conversion_flags we properly set the recently added flag > IMPLICIT_CONV_EXPR_BRACED_INIT. > > Bootstrapped/regtested on x86_64-linux and ppc64le-linux, ok for trunk? > > 2019-02-16 Marek Polacek <polacek@redhat.com> > > PR c++/89217 - ICE with list-initialization in range-based for loop. > * constexpr.c (unshare_constructor): No longer static. > * cp-tree.h (unshare_constructor): Declare. > * semantics.c (finish_compound_literal): When dealing with a > non-dependent expression in a template, return the original > expression. Pass LOOKUP_NO_NARROWING to digest_init_flags. OK. Jason
diff --git gcc/cp/cvt.c gcc/cp/cvt.c index 82a44f353c7..92677cff0c5 100644 --- gcc/cp/cvt.c +++ gcc/cp/cvt.c @@ -857,7 +857,15 @@ ocp_convert (tree type, tree expr, int convtype, int flags, return ignore_overflows (converted, e); } if (INDIRECT_TYPE_P (type) || TYPE_PTRMEM_P (type)) - return cp_convert_to_pointer (type, e, dofold, complain); + { + /* Undo what finish_non_static_data_member might have done, i.e. + turning e.g. (R *)((S *)this)->r into (R *)*((S *)this)->r, + rendering the conversion invalid. */ + if (REFERENCE_REF_P (e) + && TREE_CODE (TREE_OPERAND (e, 0)) == COMPONENT_REF) + e = TREE_OPERAND (e, 0); + return cp_convert_to_pointer (type, e, dofold, complain); + } if (code == VECTOR_TYPE) { tree in_vtype = TREE_TYPE (e); diff --git gcc/testsuite/g++.dg/cpp0x/range-for37.C gcc/testsuite/g++.dg/cpp0x/range-for37.C new file mode 100644 index 00000000000..d5c7c091d96 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/range-for37.C @@ -0,0 +1,24 @@ +// PR c++/89217 +// { dg-do compile { target c++11 } } + +struct R {}; + +struct C +{ + R* begin() const { return &r; } + R* end() const { return &r; } + + R& r; +}; + +struct S +{ + void f1() { f2<true>(); } + R& r; + + template<bool> + void f2() + { + for (auto i : C{r}) {} + } +};
Since r268321 we can call digest_init even in a template, when the compound literal isn't instantiation-dependent. Consequently, when we get to case RANGE_FOR_STMT in tsubst_expr, RANGE_FOR_EXPR might already have been digested, as in this case, where before digesting it was {*((struct S *) this)->r} and now it's TARGET_EXPR <D.2334, {.r=(struct R &) (struct R *) ((struct S *) this)->r}> (that's correct). Now, in tsubst_expr, we recurse on the latter: 17095 expr = RECUR (RANGE_FOR_EXPR (t)); and since the expression contains a COMPONENT_REF, we end up calling finish_non_static_data_member which calls build_class_member_access_expr with preserve_reference=false. Thus, after we've tsubst'd the RANGE_FOR_EXPR, we have TARGET_EXPR <D.2344, {.r=(struct R &) (struct R *) (struct R &) (struct R *) *((struct S *) this)->r}> Nevermind those casts, but "(struct R *) *((struct S *) this)->r" causes problems later in cp_fold -> cp_convert_to_pointer because we're trying to convert "*((struct S *) this)->r" of type R to R *. That errors and the error_mark_node causes grief. My first attempt was to handle this in tsubst_copy_and_build's case COMPONENT_REF, after the call to finish_non_static_data_member, but that breaks -- we can't have a LHS of a MODIFY_EXPR of a reference type. So it seems this needs to be handled closer to where it actually fails, for instance in ocp_convert. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2019-02-07 Marek Polacek <polacek@redhat.com> PR c++/89217 - ICE with list-initialization in range-based for loop. * cvt.c (ocp_convert): Unwrap REFERENCE_REF_P in a COMPONENT_REF. * g++.dg/cpp0x/range-for37.C: New test.