On Sat, Apr 03, 2021 at 10:47:13AM -0400, Jason Merrill wrote: > On 4/2/21 9:53 PM, Marek Polacek wrote: > > Coming back to > > <https://gcc.gnu.org/pipermail/gcc-patches/2019-August/527699.html>: > > > > This is a crash that points to a GC problem. Consider this test: > > > > __attribute__ ((unused)) struct S { > > S() { } > > } s; > > > > We're parsing a simple-declaration. While parsing the decl specs, we parse > > the attribute, which means creating a TREE_LIST using ggc_alloc_*. > > > > A function body is a complete-class context so when parsing the > > member-specification of this class-specifier, we parse the bodies of the > > functions we'd queued in cp_parser_late_parsing_for_member. This then > > leads to this call chain: > > cp_parser_function_definition_after_declarator -> expand_or_defer_fn -> > > expand_or_defer_fn_1 -> maybe_clone_body -> expand_or_defer_fn -> > > cgraph_node::finalize_function -> ggc_collect. > > > > In this test, the ggc_collect call collects the TREE_LIST we had > > allocated, and a crash duly ensues. > > > > I couldn't do what Richard suggested, that is, attach the attribute list > > to struct S, because we don't pass decl_specs from cp_parser_type_specifier > > down to cp_parser_class_specifier. Therefore I've attempted to do "push the > > decl_specifiers onto a vec that is a GC root", except I couldn't really push > > the decl_specifiers, because first I'd have to mark cp_decl_specifier_seq > > with > > GTY(()) and even that wouldn't be enough for me to be able to create > > > > static GTY(()) vec<cp_decl_specifier_seq *, va_gc> > > > > But here we only care about cp_decl_specifier_seq::attributes, so the > > patch is just this. I've also extended the test so now we test a nested > > class too. > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10.4? > > > > gcc/cp/ChangeLog: > > > > PR c++/91416 > > * parser.c: Create a GC root for attributes in a decl specifier. > > (cp_parser_declaration): Free it. > > (cp_parser_type_specifier): Push ->attributes onto it. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/91416 > > * g++.dg/other/gc7.C: New test. > > --- > > gcc/cp/parser.c | 7 +++++++ > > gcc/testsuite/g++.dg/other/gc7.C | 16 ++++++++++++++++ > > 2 files changed, 23 insertions(+) > > create mode 100644 gcc/testsuite/g++.dg/other/gc7.C > > > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > > index 808e5b61b1e..31da9f56b1b 100644 > > --- a/gcc/cp/parser.c > > +++ b/gcc/cp/parser.c > > @@ -13957,6 +13957,11 @@ cp_parser_declaration_seq_opt (cp_parser* parser) > > } > > } > > +/* Preserve the attributes across a garbage collect (by making it a GC > > + root), which can occur when parsing a member function. */ > > + > > +static GTY(()) vec<tree, va_gc> *cp_parser_decl_specs_attrs; > > + > > /* Parse a declaration. > > declaration: > > @@ -14140,6 +14145,7 @@ cp_parser_declaration (cp_parser* parser, tree > > prefix_attrs) > > /* Free any declarators allocated. */ > > obstack_free (&declarator_obstack, p); > > + vec_free (cp_parser_decl_specs_attrs); > > } > > /* Parse a namespace-scope declaration. */ > > @@ -18438,6 +18444,7 @@ cp_parser_type_specifier (cp_parser* parser, > > /* Parse tentatively so that we can back up if we don't find a > > class-specifier. */ > > cp_parser_parse_tentatively (parser); > > + vec_safe_push (cp_parser_decl_specs_attrs, decl_specs->attributes); > > This looks like any time we see 'class' in a function we'll add another > entry to the vec, that accumulates until we finish the enclosing top-level > declaration. Let's pop the entry again after the call to > cp_parser_class_specifier, and only push/pop if there are actually any > attributes.
Ah, yeah, push/pop works here too, thanks. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10? -- >8 -- Coming back to <https://gcc.gnu.org/pipermail/gcc-patches/2019-August/527699.html>: This is a crash that points to a GC problem. Consider this test: __attribute__ ((unused)) struct S { S() { } } s; We're parsing a simple-declaration. While parsing the decl specs, we parse the attribute, which means creating a TREE_LIST using ggc_alloc_*. A function body is a complete-class context so when parsing the member-specification of this class-specifier, we parse the bodies of the functions we'd queued in cp_parser_late_parsing_for_member. This then leads to this call chain: cp_parser_function_definition_after_declarator -> expand_or_defer_fn -> expand_or_defer_fn_1 -> maybe_clone_body -> expand_or_defer_fn -> cgraph_node::finalize_function -> ggc_collect. In this test, the ggc_collect call collects the TREE_LIST we had allocated, and a crash duly ensues. I couldn't do what Richard suggested, that is, attach the attribute list to struct S, because we don't pass decl_specs from cp_parser_type_specifier down to cp_parser_class_specifier. Therefore I've attempted to do "push the decl_specifiers onto a vec that is a GC root", except I couldn't really push the decl_specifiers, because first I'd have to mark cp_decl_specifier_seq with GTY(()) and even that wouldn't be enough for me to be able to create static GTY(()) vec<cp_decl_specifier_seq *, va_gc> But here we only care about cp_decl_specifier_seq::attributes, so the patch is just this. I've also extended the test so now we test a nested class too. gcc/cp/ChangeLog: PR c++/91416 * parser.c: Create a GC root for attributes in a decl specifier. (cp_parser_type_specifier): Push/pop ->attributes onto/from it. gcc/testsuite/ChangeLog: PR c++/91416 * g++.dg/other/gc7.C: New test. --- gcc/cp/parser.c | 9 +++++++++ gcc/testsuite/g++.dg/other/gc7.C | 16 ++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 gcc/testsuite/g++.dg/other/gc7.C diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 808e5b61b1e..c915d6415de 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -18346,6 +18346,11 @@ cp_parser_explicit_specialization (cp_parser* parser) --parser->num_template_parameter_lists; } +/* Preserve the attributes across a garbage collect (by making it a GC + root), which can occur when parsing a member function. */ + +static GTY(()) vec<tree, va_gc> *cp_parser_decl_specs_attrs; + /* Parse a type-specifier. type-specifier: @@ -18438,8 +18443,12 @@ cp_parser_type_specifier (cp_parser* parser, /* Parse tentatively so that we can back up if we don't find a class-specifier. */ cp_parser_parse_tentatively (parser); + if (decl_specs->attributes) + vec_safe_push (cp_parser_decl_specs_attrs, decl_specs->attributes); /* Look for the class-specifier. */ type_spec = cp_parser_class_specifier (parser); + if (decl_specs->attributes) + cp_parser_decl_specs_attrs->pop (); invoke_plugin_callbacks (PLUGIN_FINISH_TYPE, type_spec); /* If that worked, we're done. */ if (cp_parser_parse_definitely (parser)) diff --git a/gcc/testsuite/g++.dg/other/gc7.C b/gcc/testsuite/g++.dg/other/gc7.C new file mode 100644 index 00000000000..ab436bac72f --- /dev/null +++ b/gcc/testsuite/g++.dg/other/gc7.C @@ -0,0 +1,16 @@ +// PR c++/91416 - GC during late parsing collects live data. +// { dg-do compile } +// { dg-options "--param ggc-min-heapsize=0 --param ggc-min-expand=0" } + +__attribute__ ((unused)) struct S { + S() { } +} s; + +__attribute__ ((unused)) struct X { + void fn () + { + __attribute__ ((unused)) struct N { + N() { } + } n; + } +} x; base-commit: fc27115d6107f219e6f3dc610c99210005fe9dc5 -- 2.30.2
