> On Wed, Jan 9, 2019 at 12:48 PM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Wed, Jan 9, 2019 at 10:46 AM Joern Wolfgang Rennecke
> > <joern.renne...@riscy-ip.com> wrote:
> > >
> > > We've been running builds/regression tests for GCC 8.2 configured with
> > > --enable-checking=all, and have observed some failures related to
> > > garbage collection.
> > >
> > > First problem:
> > >
> > > The g++.dg/pr85039-2.C tests (I've looked in detail at -std=c++98, but
> > > -std=c++11 and -std=c++14 appear to follow the same pattern) see gcc
> > > garbage-collecting a live vector. A subsequent access to the vector
> > > with vec_quick_push causes a segmentation fault, as m_vecpfx.m_num is
> > > 0xa5a5a5a5 . The vec data is also being freed / poisoned. The vector in
> > > question is an auto-variable of cp_parser_parenthesized_expression_list,
> > > which is declared as: vec<tree, va_gc> *expression_list;
> > >
> > > According to doc/gty/texi: "you should reference all your data from
> > > static or external @code{GTY}-ed variables, and it is advised to call
> > > @code{ggc_collect} with a shallow call stack."
> > >
> > > In this case, cgraph_node::finalize_function calls the garage collector,
> > > as we are finishing a member function of a struct. gdb shows a backtrace
> > > of 34 frames, which is not really much as far as C++ parsing goes. The
> > > caller of finalize_function is expand_or_defer_fn, which uses the
> > > expression "function_depth > 1" to compute the no_collect paramter to
> > > finalize_function.
> > > cp_parser_parenthesized_expression_list is in frame 21 of the backtrace
> > > at this point.
> > > So, if we consider this shallow, cp_parser_parenthesized_expression_list
> > > either has to refrain from using a vector with garbage-collected
> > > allocation, or it has to make the pointer reachable from a GC root - at
> > > least if function_depth <= 1.
> > > Is the attached patch the right approach?
> > >
> > > When looking at regression test results for gcc version 9.0.0 20181028
> > > (experimental), the excess errors test for g++.dg/pr85039-2.C seems to
> > > pass, yet I can see no definite reason in the source code why that is
> > > so. I tried running the test by hand in order to check if maybe the
> > > patch for PR c++/84455 plays a role,
> > > but running the test by hand, it crashes again, and gdb shows the
> > > telltale a5 pattern in a pointer register.
> > > #0 vec<tree_node*, va_gc, vl_embed>::quick_push (obj=<optimized out>,
> > > this=0x7ffff05ece60)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/vec.h:974
> > > #1 vec_safe_push<tree_node*, va_gc> (obj=<optimized out>,
> > > v=@0x7fffffffd038: 0x7ffff05ece60)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/vec.h:766
> > > #2 cp_parser_parenthesized_expression_list (
> > > parser=parser@entry=0x7ffff7ff83f0,
> > > is_attribute_list=is_attribute_list@entry=0,
> > > cast_p=cast_p@entry=false,
> > > allow_expansion_p=allow_expansion_p@entry=true,
> > > non_constant_p=non_constant_p@entry=0x7fffffffd103,
> > > close_paren_loc=close_paren_loc@entry=0x0, wrap_locations_p=false)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:7803
> > > #3 0x00000000006e910d in cp_parser_initializer (
> > > parser=parser@entry=0x7ffff7ff83f0,
> > > is_direct_init=is_direct_init@entry=0x7fffffffd102,
> > > non_constant_p=non_constant_p@entry=0x7fffffffd103,
> > > subexpression_p=subexpression_p@entry=false)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:22009
> > > #4 0x000000000070954e in cp_parser_init_declarator (
> > > parser=parser@entry=0x7ffff7ff83f0,
> > > decl_specifiers=decl_specifiers@entry=0x7fffffffd1c0,
> > > checks=checks@entry=0x0,
> > > function_definition_allowed_p=function_definition_allowed_p@entry=true,
> > > member_p=member_p@entry=false, declares_class_or_enum=<optimized
> > > out>,
> > > function_definition_p=0x7fffffffd250, maybe_range_for_decl=0x0,
> > > init_loc=0x7fffffffd1ac, auto_result=0x7fffffffd2e0)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:19827
> > > #5 0x0000000000711c5d in cp_parser_simple_declaration
> > > (parser=0x7ffff7ff83f0,
> > > function_definition_allowed_p=<optimized out>,
> > > maybe_range_for_decl=0x0)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:13179
> > > #6 0x0000000000717bb5 in cp_parser_declaration (parser=0x7ffff7ff83f0)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:12876
> > > #7 0x000000000071837d in cp_parser_translation_unit
> > > (parser=0x7ffff7ff83f0)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:4631
> > > #8 c_parse_file ()
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/cp/parser.c:39108
> > > #9 0x0000000000868db1 in c_common_parse_file ()
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/c-family/c-opts.c:1150
> > > #10 0x0000000000e0aaaf in compile_file ()
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/toplev.c:455
> > > #11 0x000000000059248a in do_compile ()
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/toplev.c:2172
> > > #12 toplev::main (this=this@entry=0x7fffffffd54e, argc=<optimized out>,
> > >
> > > argc@entry=100, argv=<optimized out>, argv@entry=0x7fffffffd648)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/toplev.c:2307
> > > #13 0x0000000000594b5b in main (argc=100, argv=0x7fffffffd648)
> > > at
> > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/main.c:39
> > > (gdb) info reg rdx
> > > rdx 0xa5a5a5a5 2779096485
> > >
> > > 0x00000000006e84eb
> > > <cp_parser_parenthesized_expression_list(cp_parser*, int, bool, bool,
> > > bool*, location_t*, bool)+283>: je 0x6e8608
> > > <cp_parser_parenthesized_expression_list(cp_parser*, int, bool, bool,
> > > bool*, location_t*, bool)+568>
> > > 0x00000000006e84f1
> > > <cp_parser_parenthesized_expression_list(cp_parser*, int, bool, bool,
> > > bool*, location_t*, bool)+289>: lea 0x1(%rdx),%esi
> > > 0x00000000006e84f4
> > > <cp_parser_parenthesized_expression_list(cp_parser*, int, bool, bool,
> > > bool*, location_t*, bool)+292>: mov %esi,0x4(%rax)
> > > => 0x00000000006e84f7
> > > <cp_parser_parenthesized_expression_list(cp_parser*, int, bool, bool,
> > > bool*, location_t*, bool)+295>: mov %rcx,0x8(%rax,%rdx,8)
> > > 0x00000000006e84fc
> > > <cp_parser_parenthesized_expression_list(cp_parser*, int, bool, bool,
> > > bool*, location_t*, bool)+300>: cmp %rcx,0x16cd67d(%rip) #
> > > 0x1db5b80 <global_trees>
> > >
> > >
> > > Second problem:
> > >
> > > We see: FAIL: gcc.dg/plugin/start_unit-test-1.c
> > > -fplugin=./start_unit_plugin.so (test for excess errors)
> > > Excess errors:
> > > fake_var not a VAR_DECL
> > >
> > > The message comes from plugin; the source code is in
> > > testsuite/gcc.dg/plugin/start_unit_plugin.c . The plugin holds a tree
> > > value in a static variable, yet fails to make it referenced by any
> > > garbage collection root tables.
> > > When configuring with --enable-checking=all, a garbage colection comes
> > > along and
> > > poisons the fake_var allocated by the plugin.
> > > gty.texi advises:
> > > Plugins can add additional root tables. Run the @code{gengtype}
> > > utility in plugin mode as @code{gengtype -P pluginout.h
> > > @var{source-dir}
> > > @var{file-list} @var{plugin*.c}} with your plugin files
> > > @var{plugin*.c} using @code{GTY} to generate the @var{pluginout.h}
> > > file.
> > > The GCC build tree is needed to be present in that mode.
> > > However, can we use the build directory in the test suite, considering
> > > that
> > > it's supposed to be usable also for installed compilers?
> > > I suppose, for gcc core types like tree we should be able to prepare a
> > > header file beforehand.
> > >
> > >
> > > Third problem:
> > >
> > > Both in 8.2 and 9.0.0 configureed with --enable-checking-all, we see:
> > > FAIL: gcc.dg/ipa/ipa-pta-1.c scan-ipa-dump pta2 "bar.arg0 = { a }"
> > > FAIL: gcc.dg/ipa/ipa-pta-1.c scan-ipa-dump pta2 "bar.arg1 = { a }"
> > >
> > > Looking closer at the problem in 8.2, see that large parts of the dump
> > > file show signs of garbage-collector poisoned memory.
> > > Looking at "Points-to sets", the information is reached from a static
> > > variable in
> > > tree-ssa-structalias.c that lacks a GTY marker:
> > > static vec<varinfo_t> varmap;
> >
> > Hmm, it shouldn't be live across pass invocations. Where does it collect
> > when ipa_pta_execute is in the call stack?
>
> Ah, ipa_pta_execute calls ->get_body () which does
>
> if (ipa_transforms_to_apply.exists ())
> {
> ...
> push_cfun (DECL_STRUCT_FUNCTION (decl));
> execute_all_ipa_transforms ();
>
> which ends up in execute_one_ipa_transform_pass which does
>
> /* Signal this is a suitable GC collection point. */
> if (!(todo_after & TODO_do_not_ggc_collect))
> ggc_collect ();
>
> that's of course bogus. Honza - this get_body () behavior is broken, one
> "fix" would be to never collect in execute_one_ipa_transform_pass or
> somehow pass down a flag not to.
>
> Can you take care of this?
Yes, i will take a look. I think we want to be able to get bodies lazily
at random points in compilation, so we do not want to collect after
transforms. I will prepare patch.
Honza
>
> Richard.
>
> > > The "name" field in struct variable_info sometimes contains a string
> > > literal, and sometimes a ggc-allocated string.
> > > I'm not sure if ggc can handle this, or if we need to change the code so
> > > that we make these either all ggc-allocated strings, or add a
> > > discriminator so we can tell which kind of string there is so that we
> > > can tell the garbage collector to ignore string literals.
> > > At any rate, We can't make the "vec<varinfo_t>" as-is into a GC root.
> > > Should that be changed into a va_gc vector pointer, and all the code that
> > > uses it adjusted accordingly, or would it be better to use a new variable
> > > as GC root and fake the references with a GTY((user)) for the new variable
> > > that employs a marking function that marks the elements inside varmap?
> > >
