| Message ID | CAAkRFZ+BxS0WrUa37cGSN1SBzSfa3w8c-MdxzyVrWZCL0Ct0tw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 7, 2011 at 8:53 AM, Xinliang David Li <davidxl@google.com> wrote: > I have seen compiler build error (segmentation fault) in libstdc++-v3. > It turns out that a vector allocated in gc memory is GCed before the > vector is released. The gc call is from a call to synethesize_method > from cp_finish_decl. > > The following patch fixes the problem. Compiler bootstraps and tested > on linux/x86-64. Ok for trunk (or better fix suggested)? The context looks different on trunk. Do you have a backtrace on where we ggc collect from a frame starting at cp_finish_decl? > thanks, > > David > > 2011-11-05 Xinliang David Li <davidxl@google.com> > > * cp/decl.c (cp_finish_decl): Prevent cleanups from > being garbage collected before being released. > > > Index: cp/decl.c > =================================================================== > --- cp/decl.c (revision 181013) > +++ cp/decl.c (working copy) > @@ -5902,6 +5902,8 @@ value_dependent_init_p (tree init) > FLAGS is LOOKUP_ONLYCONVERTING if the = init syntax was used, else 0 > if the (init) syntax was used. */ > > +static GTY (()) VEC(tree,gc) *cleanups_vec; > + > void > cp_finish_decl (tree decl, tree init, bool init_const_expr_p, > tree asmspec_tree, int flags) > @@ -5914,6 +5916,7 @@ cp_finish_decl (tree decl, tree init, bo > bool var_definition_p = false; > tree auto_node; > > + cleanups_vec = cleanups; > if (decl == error_mark_node) > return; > else if (! decl) > @@ -6319,6 +6322,7 @@ cp_finish_decl (tree decl, tree init, bo > FOR_EACH_VEC_ELT (tree, cleanups, i, t) > push_cleanup (decl, t, false); > release_tree_vector (cleanups); > + cleanups_vec = NULL; > > if (was_readonly) > TREE_READONLY (decl) = 1; >
Here is the stack trace when the watch point is hit (the watch point is on address &cleanups->base.prefix.num David #0 memset () at ../sysdeps/x86_64/memset.S:336 #1 0x0000000000d1528d in poison_pages () at /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:1983 #2 0x0000000000d15424 in ggc_collect () at /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:2076 #3 0x0000000001028d7f in cgraph_finalize_function (decl=0x7ffff577d600, nested=0 '\000') at /usr/local/google/davidxl/dev/gcc/tot/gcc/cgraphunit.c:376 #4 0x0000000000988010 in expand_or_defer_fn (fn=0x7ffff577d600) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3797 #5 0x0000000000a678a7 in maybe_clone_body (fn=0x7ffff5770700) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/optimize.c:426 #6 0x0000000000987aa3 in expand_or_defer_fn_1 (fn=0x7ffff5770700) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3722 #7 0x0000000000987fe0 in expand_or_defer_fn (fn=0x7ffff5770700) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3792 #8 0x000000000091c5f5 in synthesize_method (fndecl=0x7ffff5770700) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/method.c:773 #9 0x0000000000551fa0 in cp_finish_decl (decl=0x7ffff5770700, init=0x7ffff6d8f898, init_const_expr_p=0 '\000', asmspec_tree=0x0, flags=11) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/decl.c:6286 #10 0x00000000007c33e0 in cp_parser_init_declarator (parser=0x7ffff68a5370, decl_specifiers=0x7fffffffced0, checks=0x0, function_definition_allowed_p=1 '\001', member_p=0 '\000', declares_class_or_enum=0, function_definition_p=0x7fffffffcf4e "", maybe_range_for_decl=0x0) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:15462 #11 0x00000000007ba512 in cp_parser_simple_declaration (parser=0x7ffff68a5370, function_definition_allowed_p=1 '\001', maybe_range_for_decl=0x0) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:10301 #12 0x00000000007ba322 in cp_parser_block_declaration (parser=0x7ffff68a5370, statement_p=0 '\000') at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:10187 #13 0x00000000007ba107 in cp_parser_declaration (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:10092 #14 0x00000000007b9cea in cp_parser_declaration_seq_opt (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:9978 #15 0x00000000007c1e36 in cp_parser_namespace_body (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:14633 #16 0x00000000007c1dec in cp_parser_namespace_definition (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:14614 #17 0x00000000007ba04d in cp_parser_declaration (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:10076 #18 0x00000000007b9cea in cp_parser_declaration_seq_opt (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:9978 #19 0x00000000007ac646 in cp_parser_translation_unit (parser=0x7ffff68a5370) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:3739 #20 0x00000000007e0682 in c_parse_file () at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/parser.c:26714 #21 0x0000000000c5dc2b in c_common_parse_file () at /usr/local/google/davidxl/dev/gcc/tot/gcc/c-family/c-opts.c:1122 #22 0x000000000203036f in compile_file () at /usr/local/google/davidxl/dev/gcc/tot/gcc/toplev.c:565 #23 0x0000000002032584 in do_compile () at /usr/local/google/davidxl/dev/gcc/tot/gcc/toplev.c:1930 #24 0x00000000020326fb in toplev_main (argc=71, argv=0x7fffffffd3c8) at /usr/local/google/davidxl/dev/gcc/tot/gcc/toplev.c:2006 #25 0x0000000000cd97e0 in main (argc=71, argv=0x7fffffffd3c8) at /usr/local/google/davidxl/dev/gcc/tot/gcc/main.c:36 On Mon, Nov 7, 2011 at 1:56 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Mon, Nov 7, 2011 at 8:53 AM, Xinliang David Li <davidxl@google.com> wrote: >> I have seen compiler build error (segmentation fault) in libstdc++-v3. >> It turns out that a vector allocated in gc memory is GCed before the >> vector is released. The gc call is from a call to synethesize_method >> from cp_finish_decl. >> >> The following patch fixes the problem. Compiler bootstraps and tested >> on linux/x86-64. Ok for trunk (or better fix suggested)? > > The context looks different on trunk. Do you have a backtrace on where > we ggc collect from a frame starting at cp_finish_decl? > >> thanks, >> >> David >> >> 2011-11-05 Xinliang David Li <davidxl@google.com> >> >> * cp/decl.c (cp_finish_decl): Prevent cleanups from >> being garbage collected before being released. >> >> >> Index: cp/decl.c >> =================================================================== >> --- cp/decl.c (revision 181013) >> +++ cp/decl.c (working copy) >> @@ -5902,6 +5902,8 @@ value_dependent_init_p (tree init) >> FLAGS is LOOKUP_ONLYCONVERTING if the = init syntax was used, else 0 >> if the (init) syntax was used. */ >> >> +static GTY (()) VEC(tree,gc) *cleanups_vec; >> + >> void >> cp_finish_decl (tree decl, tree init, bool init_const_expr_p, >> tree asmspec_tree, int flags) >> @@ -5914,6 +5916,7 @@ cp_finish_decl (tree decl, tree init, bo >> bool var_definition_p = false; >> tree auto_node; >> >> + cleanups_vec = cleanups; >> if (decl == error_mark_node) >> return; >> else if (! decl) >> @@ -6319,6 +6322,7 @@ cp_finish_decl (tree decl, tree init, bo >> FOR_EACH_VEC_ELT (tree, cleanups, i, t) >> push_cleanup (decl, t, false); >> release_tree_vector (cleanups); >> + cleanups_vec = NULL; >> >> if (was_readonly) >> TREE_READONLY (decl) = 1; >> >
On Mon, Nov 7, 2011 at 5:41 PM, Xinliang David Li <davidxl@google.com> wrote: > Here is the stack trace when the watch point is hit (the watch point > is on address &cleanups->base.prefix.num > > David > > #0 memset () at ../sysdeps/x86_64/memset.S:336 > #1 0x0000000000d1528d in poison_pages () at > /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:1983 > #2 0x0000000000d15424 in ggc_collect () at > /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:2076 > #3 0x0000000001028d7f in cgraph_finalize_function > (decl=0x7ffff577d600, nested=0 '\000') at > /usr/local/google/davidxl/dev/gcc/tot/gcc/cgraphunit.c:376 Hm. We already conditionally arrange for cgraph_finalize_function to not call ggc_collect - so it seems that doing so is even less safe than originally thought. Which means I think we should push calling ggc_collect to the callers, which for the C++ frontend means ... > #4 0x0000000000988010 in expand_or_defer_fn (fn=0x7ffff577d600) at > /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3797 > #5 0x0000000000a678a7 in maybe_clone_body (fn=0x7ffff5770700) at > /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/optimize.c:426 > #6 0x0000000000987aa3 in expand_or_defer_fn_1 (fn=0x7ffff5770700) at > /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3722 > #7 0x0000000000987fe0 in expand_or_defer_fn (fn=0x7ffff5770700) at > /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3792 > #8 0x000000000091c5f5 in synthesize_method (fndecl=0x7ffff5770700) at > /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/method.c:773 > #9 0x0000000000551fa0 in cp_finish_decl (decl=0x7ffff5770700, > init=0x7ffff6d8f898, init_const_expr_p=0 '\000', asmspec_tree=0x0, > flags=11) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/decl.c:6286 ... probably here. Though I'd also approve a patch that simply removes the ggc_collect call (and the nested parameter). Honza - you probably added the ggc_collect - what's the reason to do it in this lowlevel place? Thanks, Richard.
Here is the revised patch. Bootstrap and regression tested on linux/x86-64. Honza, can you comment on the implication of this change? thanks, David On Mon, Nov 7, 2011 at 2:09 PM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Mon, Nov 7, 2011 at 5:41 PM, Xinliang David Li <davidxl@google.com> wrote: >> Here is the stack trace when the watch point is hit (the watch point >> is on address &cleanups->base.prefix.num >> >> David >> >> #0 memset () at ../sysdeps/x86_64/memset.S:336 >> #1 0x0000000000d1528d in poison_pages () at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:1983 >> #2 0x0000000000d15424 in ggc_collect () at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:2076 >> #3 0x0000000001028d7f in cgraph_finalize_function >> (decl=0x7ffff577d600, nested=0 '\000') at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/cgraphunit.c:376 > > Hm. We already conditionally arrange for cgraph_finalize_function to not > call ggc_collect - so it seems that doing so is even less safe than originally > thought. > > Which means I think we should push calling ggc_collect to the callers, > which for the C++ frontend means ... > >> #4 0x0000000000988010 in expand_or_defer_fn (fn=0x7ffff577d600) at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3797 >> #5 0x0000000000a678a7 in maybe_clone_body (fn=0x7ffff5770700) at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/optimize.c:426 >> #6 0x0000000000987aa3 in expand_or_defer_fn_1 (fn=0x7ffff5770700) at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3722 >> #7 0x0000000000987fe0 in expand_or_defer_fn (fn=0x7ffff5770700) at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3792 >> #8 0x000000000091c5f5 in synthesize_method (fndecl=0x7ffff5770700) at >> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/method.c:773 >> #9 0x0000000000551fa0 in cp_finish_decl (decl=0x7ffff5770700, >> init=0x7ffff6d8f898, init_const_expr_p=0 '\000', asmspec_tree=0x0, >> flags=11) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/decl.c:6286 > > ... probably here. Though I'd also approve a patch that simply removes > the ggc_collect call (and the nested parameter). Honza - you probably > added the ggc_collect - what's the reason to do it in this lowlevel place? > > Thanks, > Richard. >
On Tue, Nov 8, 2011 at 6:10 PM, Xinliang David Li <davidxl@google.com> wrote: > Here is the revised patch. Bootstrap and regression tested on linux/x86-64. > > Honza, can you comment on the implication of this change? Jason also seems to have touched this again, so maybe it's already fixed? > thanks, > > David > > On Mon, Nov 7, 2011 at 2:09 PM, Richard Guenther > <richard.guenther@gmail.com> wrote: >> On Mon, Nov 7, 2011 at 5:41 PM, Xinliang David Li <davidxl@google.com> wrote: >>> Here is the stack trace when the watch point is hit (the watch point >>> is on address &cleanups->base.prefix.num >>> >>> David >>> >>> #0 memset () at ../sysdeps/x86_64/memset.S:336 >>> #1 0x0000000000d1528d in poison_pages () at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:1983 >>> #2 0x0000000000d15424 in ggc_collect () at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:2076 >>> #3 0x0000000001028d7f in cgraph_finalize_function >>> (decl=0x7ffff577d600, nested=0 '\000') at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cgraphunit.c:376 >> >> Hm. We already conditionally arrange for cgraph_finalize_function to not >> call ggc_collect - so it seems that doing so is even less safe than originally >> thought. >> >> Which means I think we should push calling ggc_collect to the callers, >> which for the C++ frontend means ... >> >>> #4 0x0000000000988010 in expand_or_defer_fn (fn=0x7ffff577d600) at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3797 >>> #5 0x0000000000a678a7 in maybe_clone_body (fn=0x7ffff5770700) at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/optimize.c:426 >>> #6 0x0000000000987aa3 in expand_or_defer_fn_1 (fn=0x7ffff5770700) at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3722 >>> #7 0x0000000000987fe0 in expand_or_defer_fn (fn=0x7ffff5770700) at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3792 >>> #8 0x000000000091c5f5 in synthesize_method (fndecl=0x7ffff5770700) at >>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/method.c:773 >>> #9 0x0000000000551fa0 in cp_finish_decl (decl=0x7ffff5770700, >>> init=0x7ffff6d8f898, init_const_expr_p=0 '\000', asmspec_tree=0x0, >>> flags=11) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/decl.c:6286 >> >> ... probably here. Though I'd also approve a patch that simply removes >> the ggc_collect call (and the nested parameter). Honza - you probably >> added the ggc_collect - what's the reason to do it in this lowlevel place? >> >> Thanks, >> Richard. >> >
Looks like it is fixed already, so there is no need for this patch. David On Wed, Nov 9, 2011 at 12:36 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Tue, Nov 8, 2011 at 6:10 PM, Xinliang David Li <davidxl@google.com> wrote: >> Here is the revised patch. Bootstrap and regression tested on linux/x86-64. >> >> Honza, can you comment on the implication of this change? > > Jason also seems to have touched this again, so maybe it's already > fixed? > >> thanks, >> >> David >> >> On Mon, Nov 7, 2011 at 2:09 PM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Mon, Nov 7, 2011 at 5:41 PM, Xinliang David Li <davidxl@google.com> wrote: >>>> Here is the stack trace when the watch point is hit (the watch point >>>> is on address &cleanups->base.prefix.num >>>> >>>> David >>>> >>>> #0 memset () at ../sysdeps/x86_64/memset.S:336 >>>> #1 0x0000000000d1528d in poison_pages () at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:1983 >>>> #2 0x0000000000d15424 in ggc_collect () at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/ggc-page.c:2076 >>>> #3 0x0000000001028d7f in cgraph_finalize_function >>>> (decl=0x7ffff577d600, nested=0 '\000') at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cgraphunit.c:376 >>> >>> Hm. We already conditionally arrange for cgraph_finalize_function to not >>> call ggc_collect - so it seems that doing so is even less safe than originally >>> thought. >>> >>> Which means I think we should push calling ggc_collect to the callers, >>> which for the C++ frontend means ... >>> >>>> #4 0x0000000000988010 in expand_or_defer_fn (fn=0x7ffff577d600) at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3797 >>>> #5 0x0000000000a678a7 in maybe_clone_body (fn=0x7ffff5770700) at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/optimize.c:426 >>>> #6 0x0000000000987aa3 in expand_or_defer_fn_1 (fn=0x7ffff5770700) at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3722 >>>> #7 0x0000000000987fe0 in expand_or_defer_fn (fn=0x7ffff5770700) at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/semantics.c:3792 >>>> #8 0x000000000091c5f5 in synthesize_method (fndecl=0x7ffff5770700) at >>>> /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/method.c:773 >>>> #9 0x0000000000551fa0 in cp_finish_decl (decl=0x7ffff5770700, >>>> init=0x7ffff6d8f898, init_const_expr_p=0 '\000', asmspec_tree=0x0, >>>> flags=11) at /usr/local/google/davidxl/dev/gcc/tot/gcc/cp/decl.c:6286 >>> >>> ... probably here. Though I'd also approve a patch that simply removes >>> the ggc_collect call (and the nested parameter). Honza - you probably >>> added the ggc_collect - what's the reason to do it in this lowlevel place? >>> >>> Thanks, >>> Richard. >>> >> >
> ... probably here. Though I'd also approve a patch that simply removes > the ggc_collect call (and the nested parameter). Honza - you probably > added the ggc_collect - what's the reason to do it in this lowlevel place? The ggc_collect was always here. Before unit-at-a-time we used to run into backend at this place and when I got stuff into unit-at-a-time, I kept the collect as otherwise the frontends did not garbagecollect at all. It is disabled for nested functions because nested functions did not compile immediately before. Honza > > Thanks, > Richard. >
> Here is the revised patch. Bootstrap and regression tested on linux/x86-64. > > Honza, can you comment on the implication of this change? Well, as I explained in previous email, this was traditinal place where frontends collect garbage while parsing, so you would need to verify that all the existing frontends actually do collect garbage resonably often. The change just adding GTY root seems better to me BTW. C++ FE produce garbage and should thus garbage collect... Honza
Index: cp/decl.c =================================================================== --- cp/decl.c (revision 181013) +++ cp/decl.c (working copy) @@ -5902,6 +5902,8 @@ value_dependent_init_p (tree init) FLAGS is LOOKUP_ONLYCONVERTING if the = init syntax was used, else 0 if the (init) syntax was used. */ +static GTY (()) VEC(tree,gc) *cleanups_vec; + void cp_finish_decl (tree decl, tree init, bool init_const_expr_p, tree asmspec_tree, int flags) @@ -5914,6 +5916,7 @@ cp_finish_decl (tree decl, tree init, bo bool var_definition_p = false; tree auto_node; + cleanups_vec = cleanups; if (decl == error_mark_node) return; else if (! decl) @@ -6319,6 +6322,7 @@ cp_finish_decl (tree decl, tree init, bo FOR_EACH_VEC_ELT (tree, cleanups, i, t) push_cleanup (decl, t, false); release_tree_vector (cleanups); + cleanups_vec = NULL; if (was_readonly) TREE_READONLY (decl) = 1;
I have seen compiler build error (segmentation fault) in libstdc++-v3. It turns out that a vector allocated in gc memory is GCed before the vector is released. The gc call is from a call to synethesize_method from cp_finish_decl. The following patch fixes the problem. Compiler bootstraps and tested on linux/x86-64. Ok for trunk (or better fix suggested)? thanks, David 2011-11-05 Xinliang David Li <davidxl@google.com> * cp/decl.c (cp_finish_decl): Prevent cleanups from being garbage collected before being released.