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

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.)


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.

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;

On 8/12/19 4:37 AM, Richard Biener wrote:
> 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?

It's already a significant issue for C++ that we only collect between 
function declarations, I'm concerned that this change will cause more 
memory problems.

Perhaps if we start parsing a class-specifier we could push the 
decl_specifiers onto a vec that is a GC root?

Jason

>> 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.

...and is defined in the declaration of something else?  Does it happen 
without declaring the variable 's'?

Jason