Re: C++ PATCH for c++/91416 - GC during late parsing collects live data

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Aug 11, 2019 at 7:12 PM Marek Polacek <polacek@redhat.com> wrote:
>
> 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.  We can't avoid late parsing of members in this context,
> so my fix is to bump function_depth, exactly following cp_parser_lambda_body.
> Since we are performing late parsing, we know we have to be nested in a class.
> (We still ggc_collect later, in c_parse_final_cleanups.)

So the struct S itself is properly referenced by a GC root?  If so why not
attach the attribute list to the tentative struct instead?  Or do we
fear we have
other non-rooted data live at the point we collect?  If so shouldn't we instead
bump function_depth when parsing a declaration in general?

>
> But here's the thing.  This goes back to ancient r104500, at least.  How has
> this not broken before?  All you need to trigger it is to enable GC checking
> and have a class with a ctor/member function, that has an attribute.  You'd
> think that since we've got hundreds of those in the testsuite, at least one of
> them would trigger this crash.  Or that there'd be several reports about this in
> the BZ already.  Yet I didn't find any.  Truly, I'm perplexed.
>
> Anyway, bootstrapped/regtested on x86_64-linux, ok for trunk?  How about 9.3?
> I vote yes.
>
> 2019-08-11  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/91416 - GC during late parsing collects live data.
>         * parser.c (cp_parser_late_parsing_for_member): Increment function_depth
>         around call to cp_parser_function_definition_after_declarator.
>
>         * g++.dg/other/gc6.C: New test.
>
> diff --git gcc/cp/parser.c gcc/cp/parser.c
> index b56cc6924f4..0d4d32e9670 100644
> --- gcc/cp/parser.c
> +++ gcc/cp/parser.c
> @@ -28934,6 +28934,8 @@ cp_parser_late_parsing_for_member (cp_parser* parser, tree member_function)
>        function_scope = current_function_decl;
>        if (function_scope)
>         push_function_context ();
> +      else
> +       ++function_depth;
>
>        /* Push the body of the function onto the lexer stack.  */
>        cp_parser_push_lexer_for_tokens (parser, tokens);
> @@ -28966,6 +28968,9 @@ cp_parser_late_parsing_for_member (cp_parser* parser, tree member_function)
>        /* Leave the scope of the containing function.  */
>        if (function_scope)
>         pop_function_context ();
> +      else
> +       --function_depth;
> +
>        cp_parser_pop_lexer (parser);
>      }
>
> diff --git gcc/testsuite/g++.dg/other/gc6.C gcc/testsuite/g++.dg/other/gc6.C
> new file mode 100644
> index 00000000000..385be5c945e
> --- /dev/null
> +++ gcc/testsuite/g++.dg/other/gc6.C
> @@ -0,0 +1,7 @@
> +// 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;