From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12928 invoked by alias); 9 Jan 2019 11:54:19 -0000 Mailing-List: contact gcc-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-owner@gcc.gnu.org Received: (qmail 12018 invoked by uid 89); 9 Jan 2019 11:54:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-lf1-f53.google.com Received: from mail-lf1-f53.google.com (HELO mail-lf1-f53.google.com) (209.85.167.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 09 Jan 2019 11:54:13 +0000 Received: by mail-lf1-f53.google.com with SMTP id a16so5436383lfg.3 for ; Wed, 09 Jan 2019 03:54:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uJbncyxafYSi2b7DbjxI1wo1zVmeNDtcOEamlXdTJAg=; b=IKepPX7M2Ns95olJss9M1vJE92ACLE4vH4d8j+p0lj+DzFi7rDTTZaLw6qvUTRzhfw 6ZOS+FlZdFWlwd/KvJv9FXzCSwiDIzvML5HsnZ9Aaa/M7k/YZe6aZwZgXdme2ue/sMl5 lpWYIdmID5vZDS3/XDj0kJesZcT6BZ1ETpX9aM5xWJ4I+djmOMN3/NGMLNTro/PE5sZw 3Vx9LBx92/BQJ3rmvqT8/dpDWwqw2Lix2C86/QPkS9+moqhuNnXV8IloQO00SkfqDe46 Znoyy/Xx2zKcxboyJ5H6L6MtJuo0OkANaTTrAHUhBj4ZlfmoQkpJH9y/ZsXy+LK9/Bof yzKA== MIME-Version: 1.0 References: <5C35C2AE.7080001@riscy-ip.com> In-Reply-To: From: Richard Biener Date: Wed, 09 Jan 2019 11:54:00 -0000 Message-ID: Subject: Re: Garbage collection bugs To: Joern Wolfgang Rennecke , Jan Hubicka Cc: "gcc@gcc.gnu.org >> GCC Development" Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00055.txt.bz2 On Wed, Jan 9, 2019 at 12:48 PM Richard Biener wrote: > > On Wed, Jan 9, 2019 at 10:46 AM Joern Wolfgang Rennecke > 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 *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::quick_push (obj=, > > this=0x7ffff05ece60) > > at > > /data/hudson/jobs/gcc-9.0.0-linux/workspace/gcc/build/../gcc/gcc/vec.h:974 > > #1 vec_safe_push (obj=, > > 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=, > > 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=, > > 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=, > > > > argc@entry=100, argv=, 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 > > > bool*, location_t*, bool)+283>: je 0x6e8608 > > > bool*, location_t*, bool)+568> > > 0x00000000006e84f1 > > > bool*, location_t*, bool)+289>: lea 0x1(%rdx),%esi > > 0x00000000006e84f4 > > > bool*, location_t*, bool)+292>: mov %esi,0x4(%rax) > > => 0x00000000006e84f7 > > > bool*, location_t*, bool)+295>: mov %rcx,0x8(%rax,%rdx,8) > > 0x00000000006e84fc > > > bool*, location_t*, bool)+300>: cmp %rcx,0x16cd67d(%rip) # > > 0x1db5b80 > > > > > > 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 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? 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" 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? > >