Message ID | 20210401181520.4147238-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: GC collects live data when synthesizing operator== [PR99831] | expand |
On 4/1/21 2:15 PM, Marek Polacek wrote: > Here we crash in reshape_init because we're accessing ggc_freed > & poisoned data: since r277865 in defaulted_late_check we call > synthesize_method here: > > if (kind == sfk_comparison) > { > /* If the function was declared constexpr, check that the definition > qualifies. Otherwise we can define the function lazily. */ > if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) > synthesize_method (fn); > return; > } > > which in this test triggers when we're processing the string<"a">{} in > the static_assert. First, we create a CONSTRUCTOR for the "{}" in > cp_parser_functional_cast, then we call finish_compound_literal which > calls complete_type and that results in garbage collection, which then > frees the CONSTRUCTOR {} we created when parsing the braced-list in > string<"a">{} -- at this point, it's not referenced by anything. > (That's not the case for 'type' in finish_compound_literal: the symbol > table contains a node for operator==, so ggc_mark_roots goes and marks > the fn decl, its type, its arguments etc., as used, so we don't collect > it.) > > We could just bump function_depth around the new call to > synthesize_method. Alternatively, as in this patch, we can create a new > GC root to hold the expression list. Since I'm adding a new global, I > felt compelled to get rid of one, and I actually did find an unused one! > So, 86 it. But I'm also happy to just go with bumping function_depth. I think bumping function_depth is the right solution; this patch only deals with one source of unreachable expressions that could be collected prematurely if complete_type results in GC running. > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? > > gcc/cp/ChangeLog: > > PR c++/99831 > * parser.c (cp_parser_functional_cast): Make the expression list > a GC root. > * pt.c: Remove the saved_trees global. > > gcc/testsuite/ChangeLog: > > PR c++/99831 > * g++.dg/other/gc6.C: New test. > --- > gcc/cp/parser.c | 20 ++++++++++++-------- > gcc/cp/pt.c | 1 - > gcc/testsuite/g++.dg/other/gc6.C | 16 ++++++++++++++++ > 3 files changed, 28 insertions(+), 9 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/other/gc6.C > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index d0477c42afe..28d7a8be258 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -30569,13 +30569,17 @@ cp_parser_simple_cast_expression (cp_parser *parser) > } > > /* Parse a functional cast to TYPE. Returns an expression > - representing the cast. */ > + representing the cast. Preserve the expression list across > + a garbage collect (by making it a GC root), which can occur > + in finish_compound_literal -> complete_type, when we synthesize > + a constexpr comparison function. */ > + > +static GTY(()) tree fncast_expression_list; > > static cp_expr > cp_parser_functional_cast (cp_parser* parser, tree type) > { > vec<tree, va_gc> *vec; > - tree expression_list; > cp_expr cast; > bool nonconst_p; > > @@ -30588,12 +30592,12 @@ cp_parser_functional_cast (cp_parser* parser, tree type) > { > cp_lexer_set_source_position (parser->lexer); > maybe_warn_cpp0x (CPP0X_INITIALIZER_LISTS); > - expression_list = cp_parser_braced_list (parser, &nonconst_p); > - CONSTRUCTOR_IS_DIRECT_INIT (expression_list) = 1; > + fncast_expression_list = cp_parser_braced_list (parser, &nonconst_p); > + CONSTRUCTOR_IS_DIRECT_INIT (fncast_expression_list) = 1; > if (TREE_CODE (type) == TYPE_DECL) > type = TREE_TYPE (type); > > - cast = finish_compound_literal (type, expression_list, > + cast = finish_compound_literal (type, fncast_expression_list, > tf_warning_or_error, fcl_functional); > /* Create a location of the form: > type_name{i, f} > @@ -30612,10 +30616,10 @@ cp_parser_functional_cast (cp_parser* parser, tree type) > /*allow_expansion_p=*/true, > /*non_constant_p=*/NULL); > if (vec == NULL) > - expression_list = error_mark_node; > + fncast_expression_list = error_mark_node; > else > { > - expression_list = build_tree_list_vec (vec); > + fncast_expression_list = build_tree_list_vec (vec); > release_tree_vector (vec); > } > > @@ -30626,7 +30630,7 @@ cp_parser_functional_cast (cp_parser* parser, tree type) > finishing at the closing paren. */ > location_t combined_loc = make_location (start_loc, start_loc, > parser->lexer); > - cast = build_functional_cast (combined_loc, type, expression_list, > + cast = build_functional_cast (combined_loc, type, fncast_expression_list, > tf_warning_or_error); > > /* [expr.const]/1: In an integral constant expression "only type > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index dc6f2f37f9b..7956e83c1de 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -65,7 +65,6 @@ static GTY(()) struct pending_template *last_pending_template; > int processing_template_parmlist; > static int template_header_count; > > -static GTY(()) tree saved_trees; > static vec<int> inline_parm_levels; > > static GTY(()) struct tinst_level *current_tinst_level; > diff --git a/gcc/testsuite/g++.dg/other/gc6.C b/gcc/testsuite/g++.dg/other/gc6.C > new file mode 100644 > index 00000000000..ff45dd313d6 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/other/gc6.C > @@ -0,0 +1,16 @@ > +// PR c++/99831 > +// { dg-do compile { target c++20 } } > +// { dg-options "--param ggc-min-heapsize=0 --param ggc-min-expand=0" } > + > +template <int N> struct S { > + constexpr S(const char (&str)[N]) : value{} { } > + char value[N]; > +}; > +template <S> struct string { > + constexpr bool operator==(const string &) const = default; > +}; > +template <S L2> void operator+(string<L2>) { > + char value[1]; > + S{value}; > +} > +static_assert(string<"a">{} == string<"a">{}); > > base-commit: 5f00df5925082c7b66da91270f2ed29bf4818c93 >
On Thu, Apr 01, 2021 at 03:09:44PM -0400, Jason Merrill wrote: > On 4/1/21 2:15 PM, Marek Polacek wrote: > > Here we crash in reshape_init because we're accessing ggc_freed > > & poisoned data: since r277865 in defaulted_late_check we call > > synthesize_method here: > > > > if (kind == sfk_comparison) > > { > > /* If the function was declared constexpr, check that the definition > > qualifies. Otherwise we can define the function lazily. */ > > if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) > > synthesize_method (fn); > > return; > > } > > > > which in this test triggers when we're processing the string<"a">{} in > > the static_assert. First, we create a CONSTRUCTOR for the "{}" in > > cp_parser_functional_cast, then we call finish_compound_literal which > > calls complete_type and that results in garbage collection, which then > > frees the CONSTRUCTOR {} we created when parsing the braced-list in > > string<"a">{} -- at this point, it's not referenced by anything. > > (That's not the case for 'type' in finish_compound_literal: the symbol > > table contains a node for operator==, so ggc_mark_roots goes and marks > > the fn decl, its type, its arguments etc., as used, so we don't collect > > it.) > > > > We could just bump function_depth around the new call to > > synthesize_method. Alternatively, as in this patch, we can create a new > > GC root to hold the expression list. Since I'm adding a new global, I > > felt compelled to get rid of one, and I actually did find an unused one! > > So, 86 it. But I'm also happy to just go with bumping function_depth. > > I think bumping function_depth is the right solution; this patch only deals > with one source of unreachable expressions that could be collected > prematurely if complete_type results in GC running. Fair enough, let's go back to my original patch. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? And now I should get back to <https://gcc.gnu.org/pipermail/gcc-patches/2019-August/527699.html> where we actually want a new GC root... -- >8 -- Here we crash in reshape_init because we're accessing ggc_freed & poisoned data: since r277865 in defaulted_late_check we call synthesize_method here: if (kind == sfk_comparison) { /* If the function was declared constexpr, check that the definition qualifies. Otherwise we can define the function lazily. */ if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) synthesize_method (fn); return; } which in this test triggers when we're processing the string<"a">{} in the static_assert. First, we create a CONSTRUCTOR for the "{}" in cp_parser_functional_cast, then we call finish_compound_literal which calls complete_type and that results in garbage collection, which then frees the CONSTRUCTOR {} we created when parsing the braced-list in string<"a">{} -- at this point, it's not referenced by anything. (That's not the case for 'type' in finish_compound_literal: the symbol table contains a node for operator==, so ggc_mark_roots goes and marks the fn decl, its type, its arguments etc., as used, so we don't collect it.) We could just bump function_depth around the new call to synthesize_method to prevent GC. gcc/cp/ChangeLog: PR c++/99831 * method.c (defaulted_late_check): ++ and -- function_depth around the call to synthesize_method. * pt.c: Remove the saved_trees global. gcc/testsuite/ChangeLog: PR c++/99831 * g++.dg/other/gc6.C: New test. --- gcc/cp/method.c | 7 ++++++- gcc/cp/pt.c | 1 - gcc/testsuite/g++.dg/other/gc6.C | 16 ++++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/other/gc6.C diff --git a/gcc/cp/method.c b/gcc/cp/method.c index 8ae7496f023..0f416bec35b 100644 --- a/gcc/cp/method.c +++ b/gcc/cp/method.c @@ -3131,7 +3131,12 @@ defaulted_late_check (tree fn) /* If the function was declared constexpr, check that the definition qualifies. Otherwise we can define the function lazily. */ if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) - synthesize_method (fn); + { + /* Prevent GC. */ + function_depth++; + synthesize_method (fn); + function_depth--; + } return; } diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index dc6f2f37f9b..7956e83c1de 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -65,7 +65,6 @@ static GTY(()) struct pending_template *last_pending_template; int processing_template_parmlist; static int template_header_count; -static GTY(()) tree saved_trees; static vec<int> inline_parm_levels; static GTY(()) struct tinst_level *current_tinst_level; diff --git a/gcc/testsuite/g++.dg/other/gc6.C b/gcc/testsuite/g++.dg/other/gc6.C new file mode 100644 index 00000000000..ff45dd313d6 --- /dev/null +++ b/gcc/testsuite/g++.dg/other/gc6.C @@ -0,0 +1,16 @@ +// PR c++/99831 +// { dg-do compile { target c++20 } } +// { dg-options "--param ggc-min-heapsize=0 --param ggc-min-expand=0" } + +template <int N> struct S { + constexpr S(const char (&str)[N]) : value{} { } + char value[N]; +}; +template <S> struct string { + constexpr bool operator==(const string &) const = default; +}; +template <S L2> void operator+(string<L2>) { + char value[1]; + S{value}; +} +static_assert(string<"a">{} == string<"a">{}); base-commit: 0cf4813202f19768e31666f6aa82bde4dce4065a
On 4/1/21 5:12 PM, Marek Polacek wrote: > On Thu, Apr 01, 2021 at 03:09:44PM -0400, Jason Merrill wrote: >> On 4/1/21 2:15 PM, Marek Polacek wrote: >>> Here we crash in reshape_init because we're accessing ggc_freed >>> & poisoned data: since r277865 in defaulted_late_check we call >>> synthesize_method here: >>> >>> if (kind == sfk_comparison) >>> { >>> /* If the function was declared constexpr, check that the definition >>> qualifies. Otherwise we can define the function lazily. */ >>> if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) >>> synthesize_method (fn); >>> return; >>> } >>> >>> which in this test triggers when we're processing the string<"a">{} in >>> the static_assert. First, we create a CONSTRUCTOR for the "{}" in >>> cp_parser_functional_cast, then we call finish_compound_literal which >>> calls complete_type and that results in garbage collection, which then >>> frees the CONSTRUCTOR {} we created when parsing the braced-list in >>> string<"a">{} -- at this point, it's not referenced by anything. >>> (That's not the case for 'type' in finish_compound_literal: the symbol >>> table contains a node for operator==, so ggc_mark_roots goes and marks >>> the fn decl, its type, its arguments etc., as used, so we don't collect >>> it.) >>> >>> We could just bump function_depth around the new call to >>> synthesize_method. Alternatively, as in this patch, we can create a new >>> GC root to hold the expression list. Since I'm adding a new global, I >>> felt compelled to get rid of one, and I actually did find an unused one! >>> So, 86 it. But I'm also happy to just go with bumping function_depth. >> >> I think bumping function_depth is the right solution; this patch only deals >> with one source of unreachable expressions that could be collected >> prematurely if complete_type results in GC running. > > Fair enough, let's go back to my original patch. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? OK. > And now I should get back to > <https://gcc.gnu.org/pipermail/gcc-patches/2019-August/527699.html> > where we actually want a new GC root... > > -- >8 -- > Here we crash in reshape_init because we're accessing ggc_freed > & poisoned data: since r277865 in defaulted_late_check we call > synthesize_method here: > > if (kind == sfk_comparison) > { > /* If the function was declared constexpr, check that the definition > qualifies. Otherwise we can define the function lazily. */ > if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) > synthesize_method (fn); > return; > } > > which in this test triggers when we're processing the string<"a">{} in > the static_assert. First, we create a CONSTRUCTOR for the "{}" in > cp_parser_functional_cast, then we call finish_compound_literal which > calls complete_type and that results in garbage collection, which then > frees the CONSTRUCTOR {} we created when parsing the braced-list in > string<"a">{} -- at this point, it's not referenced by anything. > (That's not the case for 'type' in finish_compound_literal: the symbol > table contains a node for operator==, so ggc_mark_roots goes and marks > the fn decl, its type, its arguments etc., as used, so we don't collect > it.) > > We could just bump function_depth around the new call to synthesize_method > to prevent GC. > > gcc/cp/ChangeLog: > > PR c++/99831 > * method.c (defaulted_late_check): ++ and -- function_depth around > the call to synthesize_method. > * pt.c: Remove the saved_trees global. > > gcc/testsuite/ChangeLog: > > PR c++/99831 > * g++.dg/other/gc6.C: New test. > --- > gcc/cp/method.c | 7 ++++++- > gcc/cp/pt.c | 1 - > gcc/testsuite/g++.dg/other/gc6.C | 16 ++++++++++++++++ > 3 files changed, 22 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/other/gc6.C > > diff --git a/gcc/cp/method.c b/gcc/cp/method.c > index 8ae7496f023..0f416bec35b 100644 > --- a/gcc/cp/method.c > +++ b/gcc/cp/method.c > @@ -3131,7 +3131,12 @@ defaulted_late_check (tree fn) > /* If the function was declared constexpr, check that the definition > qualifies. Otherwise we can define the function lazily. */ > if (DECL_DECLARED_CONSTEXPR_P (fn) && !DECL_INITIAL (fn)) > - synthesize_method (fn); > + { > + /* Prevent GC. */ > + function_depth++; > + synthesize_method (fn); > + function_depth--; > + } > return; > } > > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index dc6f2f37f9b..7956e83c1de 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -65,7 +65,6 @@ static GTY(()) struct pending_template *last_pending_template; > int processing_template_parmlist; > static int template_header_count; > > -static GTY(()) tree saved_trees; > static vec<int> inline_parm_levels; > > static GTY(()) struct tinst_level *current_tinst_level; > diff --git a/gcc/testsuite/g++.dg/other/gc6.C b/gcc/testsuite/g++.dg/other/gc6.C > new file mode 100644 > index 00000000000..ff45dd313d6 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/other/gc6.C > @@ -0,0 +1,16 @@ > +// PR c++/99831 > +// { dg-do compile { target c++20 } } > +// { dg-options "--param ggc-min-heapsize=0 --param ggc-min-expand=0" } > + > +template <int N> struct S { > + constexpr S(const char (&str)[N]) : value{} { } > + char value[N]; > +}; > +template <S> struct string { > + constexpr bool operator==(const string &) const = default; > +}; > +template <S L2> void operator+(string<L2>) { > + char value[1]; > + S{value}; > +} > +static_assert(string<"a">{} == string<"a">{}); > > base-commit: 0cf4813202f19768e31666f6aa82bde4dce4065a >
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index d0477c42afe..28d7a8be258 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -30569,13 +30569,17 @@ cp_parser_simple_cast_expression (cp_parser *parser) } /* Parse a functional cast to TYPE. Returns an expression - representing the cast. */ + representing the cast. Preserve the expression list across + a garbage collect (by making it a GC root), which can occur + in finish_compound_literal -> complete_type, when we synthesize + a constexpr comparison function. */ + +static GTY(()) tree fncast_expression_list; static cp_expr cp_parser_functional_cast (cp_parser* parser, tree type) { vec<tree, va_gc> *vec; - tree expression_list; cp_expr cast; bool nonconst_p; @@ -30588,12 +30592,12 @@ cp_parser_functional_cast (cp_parser* parser, tree type) { cp_lexer_set_source_position (parser->lexer); maybe_warn_cpp0x (CPP0X_INITIALIZER_LISTS); - expression_list = cp_parser_braced_list (parser, &nonconst_p); - CONSTRUCTOR_IS_DIRECT_INIT (expression_list) = 1; + fncast_expression_list = cp_parser_braced_list (parser, &nonconst_p); + CONSTRUCTOR_IS_DIRECT_INIT (fncast_expression_list) = 1; if (TREE_CODE (type) == TYPE_DECL) type = TREE_TYPE (type); - cast = finish_compound_literal (type, expression_list, + cast = finish_compound_literal (type, fncast_expression_list, tf_warning_or_error, fcl_functional); /* Create a location of the form: type_name{i, f} @@ -30612,10 +30616,10 @@ cp_parser_functional_cast (cp_parser* parser, tree type) /*allow_expansion_p=*/true, /*non_constant_p=*/NULL); if (vec == NULL) - expression_list = error_mark_node; + fncast_expression_list = error_mark_node; else { - expression_list = build_tree_list_vec (vec); + fncast_expression_list = build_tree_list_vec (vec); release_tree_vector (vec); } @@ -30626,7 +30630,7 @@ cp_parser_functional_cast (cp_parser* parser, tree type) finishing at the closing paren. */ location_t combined_loc = make_location (start_loc, start_loc, parser->lexer); - cast = build_functional_cast (combined_loc, type, expression_list, + cast = build_functional_cast (combined_loc, type, fncast_expression_list, tf_warning_or_error); /* [expr.const]/1: In an integral constant expression "only type diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index dc6f2f37f9b..7956e83c1de 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -65,7 +65,6 @@ static GTY(()) struct pending_template *last_pending_template; int processing_template_parmlist; static int template_header_count; -static GTY(()) tree saved_trees; static vec<int> inline_parm_levels; static GTY(()) struct tinst_level *current_tinst_level; diff --git a/gcc/testsuite/g++.dg/other/gc6.C b/gcc/testsuite/g++.dg/other/gc6.C new file mode 100644 index 00000000000..ff45dd313d6 --- /dev/null +++ b/gcc/testsuite/g++.dg/other/gc6.C @@ -0,0 +1,16 @@ +// PR c++/99831 +// { dg-do compile { target c++20 } } +// { dg-options "--param ggc-min-heapsize=0 --param ggc-min-expand=0" } + +template <int N> struct S { + constexpr S(const char (&str)[N]) : value{} { } + char value[N]; +}; +template <S> struct string { + constexpr bool operator==(const string &) const = default; +}; +template <S L2> void operator+(string<L2>) { + char value[1]; + S{value}; +} +static_assert(string<"a">{} == string<"a">{});