From patchwork Mon Feb 5 19:34:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 25824 Received: (qmail 2785 invoked by alias); 5 Feb 2018 19:34:59 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 2768 invoked by uid 89); 5 Feb 2018 19:34:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f49.google.com Received: from mail-wm0-f49.google.com (HELO mail-wm0-f49.google.com) (74.125.82.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 05 Feb 2018 19:34:55 +0000 Received: by mail-wm0-f49.google.com with SMTP id v123so28096478wmd.5 for ; Mon, 05 Feb 2018 11:34:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+sc/0Vw8E1gZXVo1zA4yeHnElR6dSHjrsGOyJSdIcIg=; b=kMfFJvR5b/KfZ71dIKUA7bxR2dIn3Vml85RgrnvMdj2yTeqIo62KhjYRRpete0h8+2 6FJlnnzytshYytUkIlwsv3kaKuPtQ6HkFnmqYQPugWJt+YbxtCkl5OIydwYTP1FrfKQN rGxHGHxPMx5fmZou1qcUehEWDVTnUkJBzeUrVSzriAB6ip1JYrC3s74ENDVxB9BUCN11 a63A4vMioB8U7hoxDTCGIoUcvRhMnDtx3eRz6NGdNhEyFuikuxuABPcas9u6TpZ45IGl DGXejKMxVtajiA8cxgzKwIBH6Z7Gl45KHI4wtxgxflglCHP1S33rdv51C9tb0WNQRUQA +7eQ== X-Gm-Message-State: APf1xPCpRt0ZJE6Hk0Z9GkgcamVMH940Jo5Yms0s0oA1P9wjyoufpAPh vpBqnw4ZVB9MOvQILc5GPVAr/XQa X-Google-Smtp-Source: AH8x226BEr6xIp6MOcbA9K7vLzF5VB+QHweieaqtjaZFPub3XH2kohX5ScgkMF1Im7iShcXqXc9q8A== X-Received: by 10.28.65.133 with SMTP id o127mr318919wma.130.1517859292374; Mon, 05 Feb 2018 11:34:52 -0800 (PST) Received: from localhost (host86-152-213-123.range86-152.btcentralplus.com. [86.152.213.123]) by smtp.gmail.com with ESMTPSA id v9sm4956554wre.8.2018.02.05.11.34.51 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 05 Feb 2018 11:34:51 -0800 (PST) Date: Mon, 5 Feb 2018 19:34:50 +0000 From: Andrew Burgess To: GDB Patches Cc: Yao Qi Subject: Re: [PATCH] gdb: Remove cleanup from dw2_do_instantiate_symtab Message-ID: <20180205193450.GC2778@embecosm.com> References: <20180202235354.6468-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.1 (2017-09-22) X-IsSubscribed: yes * Yao Qi [2018-02-05 14:45:13 +0000]: > On Fri, Feb 2, 2018 at 11:53 PM, Andrew Burgess > wrote: > > When running the test gdb.dwarf2/dw2-bad-parameter-type.exp under > > valgrind, I see the following issue reported (on x86-64 Fedora): > > > > Hi Andrew, > Thanks for fixing this issue. I've seen this for some days, but didn't have > a chance to fix it. > > > @@ -3132,14 +3167,15 @@ dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu) > > { > > struct cleanup *back_to; > > back_to is no longer used. We can remove it. > > > struct dwarf2_per_objfile *dwarf2_per_objfile = per_cu->dwarf2_per_objfile; > > + /* The destructor of this dwarf2_queue_guard frees any entries left on > > + the queue. */ > > + dwarf2_queue_guard q_guard; > > > > /* Skip type_unit_groups, reading the type units they contain > > is handled elsewhere. */ > > if (IS_TYPE_UNIT_GROUP (per_cu)) > > return; > > > > - back_to = make_cleanup (dwarf2_release_queue, NULL); > > - > > I don't know much about dwarf2_queue. The existing code only releases > dwarf2_queue after this point (either the function returns or exception > is thrown), but your patch changes this to "releases dwarf2_queue" every > time dw2_do_instantiate_symtab is called. Is it expected? That's a good point that I hadn't considered. I ran a few experiments using GCC 7.2, with -gdwarf-4 and -fdebug-type-sections added to the compiler flags (I believe IS_TYPE_UNIT_GROUP is related to -fdebug-type-sections) but I was unable to cause the IS_TYPE_UNIT_GROUP check to trigger, which would explain why my change didn't break anything. So, assuming that the IS_TYPE_UNIT_GROUP can trigger in some cases, I've simply moved the creation of the dwarf2_queue_guard down the function until after the early return. I don't know if this violates the coding-standard. I know it used to be the case that variables had to be declared at the start of the block, but there are a few examples in this same file where this rule is not followed. Alternatively, I can create a dummy block, or move the core of the function into a new function if this is preferred. Let me know what you'd prefer. New patch below. Thanks, Andrew --- [PATCH] gdb: Remove cleanup from dw2_do_instantiate_symtab When running the test gdb.dwarf2/dw2-bad-parameter-type.exp under valgrind, I see the following issue reported (on x86-64 Fedora): (gdb) ptype f ==5203== Invalid read of size 1 ==5203== at 0x6931FE: process_die_scope::~process_die_scope() (dwarf2read.c:10642) ==5203== by 0x66818F: process_die(die_info*, dwarf2_cu*) (dwarf2read.c:10664) ==5203== by 0x66A01F: read_file_scope(die_info*, dwarf2_cu*) (dwarf2read.c:11650) ==5203== by 0x667F2D: process_die(die_info*, dwarf2_cu*) (dwarf2read.c:10672) ==5203== by 0x6677B6: process_full_comp_unit(dwarf2_per_cu_data*, language) (dwarf2read.c:10445) ==5203== by 0x66657A: process_queue(dwarf2_per_objfile*) (dwarf2read.c:9945) ==5203== by 0x6559B4: dw2_do_instantiate_symtab(dwarf2_per_cu_data*) (dwarf2read.c:3163) ==5203== by 0x66683D: psymtab_to_symtab_1(partial_symtab*) (dwarf2read.c:10034) ==5203== by 0x66622A: dwarf2_read_symtab(partial_symtab*, objfile*) (dwarf2read.c:9811) ==5203== by 0x787984: psymtab_to_symtab(objfile*, partial_symtab*) (psymtab.c:792) ==5203== by 0x786E3E: psym_lookup_symbol(objfile*, int, char const*, domain_enum_tag) (psymtab.c:522) ==5203== by 0x804BD0: lookup_symbol_via_quick_fns(objfile*, int, char const*, domain_enum_tag) (symtab.c:2383) ==5203== Address 0x147ed063 is 291 bytes inside a block of size 4,064 free'd ==5203== at 0x4C2CD5A: free (vg_replace_malloc.c:530) ==5203== by 0x444415: void xfree(void*) (common-utils.h:60) ==5203== by 0x9DA8C2: call_freefun (obstack.c:103) ==5203== by 0x9DAD35: _obstack_free (obstack.c:280) ==5203== by 0x44464C: auto_obstack::~auto_obstack() (gdb_obstack.h:73) ==5203== by 0x68AFB0: dwarf2_cu::~dwarf2_cu() (dwarf2read.c:25080) ==5203== by 0x68B204: free_one_cached_comp_unit(dwarf2_per_cu_data*) (dwarf2read.c:25174) ==5203== by 0x66668C: dwarf2_release_queue(void*) (dwarf2read.c:9982) ==5203== by 0x563A4C: do_my_cleanups(cleanup**, cleanup*) (cleanups.c:154) ==5203== by 0x563AA7: do_cleanups(cleanup*) (cleanups.c:176) ==5203== by 0x5646CE: throw_exception_cxx(gdb_exception) (common-exceptions.c:289) ==5203== by 0x5647B7: throw_exception(gdb_exception) (common-exceptions.c:317) ==5203== Block was alloc'd at ==5203== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) ==5203== by 0x564BE8: xmalloc (common-utils.c:44) ==5203== by 0x9DA872: call_chunkfun (obstack.c:94) ==5203== by 0x9DA935: _obstack_begin_worker (obstack.c:141) ==5203== by 0x9DAA3C: _obstack_begin (obstack.c:164) ==5203== by 0x4445E0: auto_obstack::auto_obstack() (gdb_obstack.h:70) ==5203== by 0x68AE07: dwarf2_cu::dwarf2_cu(dwarf2_per_cu_data*) (dwarf2read.c:25073) ==5203== by 0x661A8A: init_cutu_and_read_dies(dwarf2_per_cu_data*, abbrev_table*, int, int, void (*)(die_reader_specs const*, unsigned char const*, die_info*, int, void*), void*) (dwarf2read.c:7869) ==5203== by 0x666A29: load_full_comp_unit(dwarf2_per_cu_data*, language) (dwarf2read.c:10108) ==5203== by 0x655847: load_cu(dwarf2_per_cu_data*) (dwarf2read.c:3120) ==5203== by 0x655928: dw2_do_instantiate_symtab(dwarf2_per_cu_data*) (dwarf2read.c:3148) ==5203== by 0x66683D: psymtab_to_symtab_1(partial_symtab*) (dwarf2read.c:10034) There's actually a series of three issues reported, but it turns out they're all related, so we can consider on the first one. The invalid read is triggered from a destructor which is being invoked as part of a stack unwind after throwing an error. At the time the error is thrown, the stack looks like this: #0 0x00000000009f4ecd in __cxa_throw () #1 0x0000000000564761 in throw_exception_cxx (exception=...) at ../../src/gdb/common/common-exceptions.c:303 #2 0x00000000005647b8 in throw_exception (exception=...) at ../../src/gdb/common/common-exceptions.c:317 #3 0x00000000005648ff in throw_it(return_reason, errors, const char *, typedef __va_list_tag __va_list_tag *) (reason=RETURN_ERROR, error=GENERIC_ERROR, fmt=0xb33020 "Dwarf Error: Cannot find DIE at 0x%x referenced from DIE at 0x%x [in module %s]", ap=0x7fff387f2d68) at ../../src/gdb/common/common-exceptions.c:373 #4 0x0000000000564929 in throw_verror (error=GENERIC_ERROR, fmt=0xb33020 "Dwarf Error: Cannot find DIE at 0x%x referenced from DIE at 0x%x [in module %s]", ap=0x7fff387f2d68) at ../../src/gdb/common/common-exceptions.c:379 #5 0x0000000000867be4 in verror (string=0xb33020 "Dwarf Error: Cannot find DIE at 0x%x referenced from DIE at 0x%x [in module %s]", args=0x7fff387f2d68) at ../../src/gdb/utils.c:251 #6 0x000000000056879d in error (fmt=0xb33020 "Dwarf Error: Cannot find DIE at 0x%x referenced from DIE at 0x%x [in module %s]") at ../../src/gdb/common/errors.c:43 #7 0x0000000000686875 in follow_die_ref (src_die=0x30bc8a0, attr=0x30bc8c8, ref_cu=0x7fff387f2ed0) at ../../src/gdb/dwarf2read.c:22969 #8 0x00000000006844cd in lookup_die_type (die=0x30bc8a0, attr=0x30bc8c8, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:21976 #9 0x0000000000683f27 in die_type (die=0x30bc8a0, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:21832 #10 0x0000000000679b39 in read_subroutine_type (die=0x30bc830, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:17343 #11 0x00000000006845fb in read_type_die_1 (die=0x30bc830, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:22035 #12 0x0000000000684576 in read_type_die (die=0x30bc830, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:22010 #13 0x000000000067003f in read_func_scope (die=0x30bc830, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:13822 #14 0x0000000000667f5e in process_die (die=0x30bc830, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:10679 #15 0x000000000066a020 in read_file_scope (die=0x30bc720, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:11650 #16 0x0000000000667f2e in process_die (die=0x30bc720, cu=0x30bc5d0) at ../../src/gdb/dwarf2read.c:10672 #17 0x00000000006677b7 in process_full_comp_unit (per_cu=0x3089b80, pretend_language=language_minimal) at ../../src/gdb/dwarf2read.c:10445 #18 0x000000000066657b in process_queue (dwarf2_per_objfile=0x30897d0) at ../../src/gdb/dwarf2read.c:9945 #19 0x00000000006559b5 in dw2_do_instantiate_symtab (per_cu=0x3089b80) at ../../src/gdb/dwarf2read.c:3163 #20 0x000000000066683e in psymtab_to_symtab_1 (pst=0x3089bd0) at ../../src/gdb/dwarf2read.c:10034 #21 0x000000000066622b in dwarf2_read_symtab (self=0x3089bd0, objfile=0x3073f40) at ../../src/gdb/dwarf2read.c:9811 #22 0x0000000000787985 in psymtab_to_symtab (objfile=0x3073f40, pst=0x3089bd0) at ../../src/gdb/psymtab.c:792 #23 0x0000000000786e3f in psym_lookup_symbol (objfile=0x3073f40, block_index=1, name=0x30b2e30 "f", domain=VAR_DOMAIN) at ../../src/gdb/psymtab.c:522 #24 0x0000000000804bd1 in lookup_symbol_via_quick_fns (objfile=0x3073f40, block_index=1, name=0x30b2e30 "f", domain=VAR_DOMAIN) at ../../src/gdb/symtab.c:2383 #25 0x0000000000804fe4 in lookup_symbol_in_objfile (objfile=0x3073f40, block_index=1, name=0x30b2e30 "f", domain=VAR_DOMAIN) at ../../src/gdb/symtab.c:2558 #26 0x0000000000805125 in lookup_static_symbol (name=0x30b2e30 "f", domain=VAR_DOMAIN) at ../../src/gdb/symtab.c:2595 #27 0x0000000000804357 in lookup_symbol_aux (name=0x30b2e30 "f", match_type=symbol_name_match_type::FULL, block=0x0, domain=VAR_DOMAIN, language=language_c, is_a_field_of_this=0x0) at ../../src/gdb/symtab.c:2105 #28 0x0000000000803ad9 in lookup_symbol_in_language (name=0x30b2e30 "f", block=0x0, domain=VAR_DOMAIN, lang=language_c, is_a_field_of_this=0x0) at ../../src/gdb/symtab.c:1887 #29 0x0000000000803b53 in lookup_symbol (name=0x30b2e30 "f", block=0x0, domain=VAR_DOMAIN, is_a_field_of_this=0x0) at ../../src/gdb/symtab.c:1899 #30 0x000000000053b246 in classify_name (par_state=0x7fff387f6090, block=0x0, is_quoted_name=false, is_after_structop=false) at ../../src/gdb/c-exp.y:2879 #31 0x000000000053b7e9 in c_yylex () at ../../src/gdb/c-exp.y:3083 #32 0x000000000053414a in c_yyparse () at c-exp.c:1903 #33 0x000000000053c2e7 in c_parse (par_state=0x7fff387f6090) at ../../src/gdb/c-exp.y:3255 #34 0x0000000000774a02 in parse_exp_in_context_1 (stringptr=0x7fff387f61c0, pc=0, block=0x0, comma=0, void_context_p=0, out_subexp=0x0) at ../../src/gdb/parse.c:1213 #35 0x000000000077476a in parse_exp_in_context (stringptr=0x7fff387f61c0, pc=0, block=0x0, comma=0, void_context_p=0, out_subexp=0x0) at ../../src/gdb/parse.c:1115 #36 0x0000000000774714 in parse_exp_1 (stringptr=0x7fff387f61c0, pc=0, block=0x0, comma=0) at ../../src/gdb/parse.c:1106 #37 0x0000000000774c53 in parse_expression (string=0x27ff996 "f") at ../../src/gdb/parse.c:1253 #38 0x0000000000861dc4 in whatis_exp (exp=0x27ff996 "f", show=1) at ../../src/gdb/typeprint.c:472 #39 0x00000000008620d8 in ptype_command (type_name=0x27ff996 "f", from_tty=1) at ../../src/gdb/typeprint.c:561 #40 0x000000000047430b in do_const_cfunc (c=0x3012010, args=0x27ff996 "f", from_tty=1) at ../../src/gdb/cli/cli-decode.c:106 #41 0x000000000047715e in cmd_func (cmd=0x3012010, args=0x27ff996 "f", from_tty=1) at ../../src/gdb/cli/cli-decode.c:1886 #42 0x00000000008431bb in execute_command (p=0x27ff996 "f", from_tty=1) at ../../src/gdb/top.c:630 #43 0x00000000006bf946 in command_handler (command=0x27ff990 "ptype f") at ../../src/gdb/event-top.c:583 #44 0x00000000006bfd12 in command_line_handler (rl=0x30bb3a0 "\240\305\v\003") at ../../src/gdb/event-top.c:774 The problem is that in `process_die` (frames 14 and 16) we create a `process_die_scope` object, that takes a copy of the `struct dwarf2_cu *` passed into the frame. The destructor of the `process_die_scope` dereferences the stored pointer. This wouldn't be an issue, except... ... in dw2_do_instantiate_symtab (frame 19) a clean up was registered that clears the dwarf2_queue in case of an error. Part of this clean up involves deleting the `struct dwarf2_cu`s referenced from the queue.. The problem then, is that cleanups are processed at the site of the throw, while, class destructors are invoked as we unwind their frame. The result is that we process the frame 19 cleanup (and delete the struct dwarf2_cu) before we process the destructors in frames 14 and 16. When we do get back to frames 14 and 16 the objects being references have already been deleted. The solution is to remove the cleanup from dw2_do_instantiate_symtab, and instead use a destructor to release the dwarf2_queue instead. With this patch in place, the valgrind errors are now resolved. gdb/ChangeLog: * dwarf2read.c (dwarf2_release_queue): Delete function, move body into... (class dwarf2_queue_guard): ...the destructor of this new class. (dw2_do_instantiate_symtab): Create instance of the new class dwarf2_queue_guard, remove cleanup. --- gdb/dwarf2read.c | 76 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index d651725bbe8..cbfd34154a5 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -2185,13 +2185,48 @@ static struct type *get_die_type_at_offset (sect_offset, static struct type *get_die_type (struct die_info *die, struct dwarf2_cu *cu); -static void dwarf2_release_queue (void *dummy); - static void queue_comp_unit (struct dwarf2_per_cu_data *per_cu, enum language pretend_language); static void process_queue (struct dwarf2_per_objfile *dwarf2_per_objfile); +/* Class, the destructor of which frees all allocated queue entries. This + will only have work to do if an error was thrown while processing the + dwarf. If no error was thrown then the queue entries should have all + been processed, and freed, as we went along. */ + +class dwarf2_queue_guard +{ +public: + dwarf2_queue_guard () = default; + + /* Free any entries remaining on the queue. There should only be + entries left if we hit an error while processing the dwarf. */ + ~dwarf2_queue_guard () + { + struct dwarf2_queue_item *item, *last; + + item = dwarf2_queue; + while (item) + { + /* Anything still marked queued is likely to be in an + inconsistent state, so discard it. */ + if (item->per_cu->queued) + { + if (item->per_cu->cu != NULL) + free_one_cached_comp_unit (item->per_cu); + item->per_cu->queued = 0; + } + + last = item; + item = item->next; + xfree (last); + } + + dwarf2_queue = dwarf2_queue_tail = NULL; + } +}; + /* The return type of find_file_and_directory. Note, the enclosed string pointers are only valid while this object is valid. */ @@ -3130,7 +3165,6 @@ load_cu (struct dwarf2_per_cu_data *per_cu) static void dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu) { - struct cleanup *back_to; struct dwarf2_per_objfile *dwarf2_per_objfile = per_cu->dwarf2_per_objfile; /* Skip type_unit_groups, reading the type units they contain @@ -3138,7 +3172,10 @@ dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu) if (IS_TYPE_UNIT_GROUP (per_cu)) return; - back_to = make_cleanup (dwarf2_release_queue, NULL); + /* The destructor of dwarf2_queue_guard frees any entries left on + the queue. After this point we're guaranteed to leave this function + with the dwarf queue empty. */ + dwarf2_queue_guard q_guard; if (dwarf2_per_objfile->using_index ? per_cu->v.quick->compunit_symtab == NULL @@ -3165,8 +3202,6 @@ dw2_do_instantiate_symtab (struct dwarf2_per_cu_data *per_cu) /* Age the cache, releasing compilation units that have not been used recently. */ age_cached_comp_units (dwarf2_per_objfile); - - do_cleanups (back_to); } /* Ensure that the symbols for PER_CU have been read in. OBJFILE is @@ -9962,35 +9997,6 @@ process_queue (struct dwarf2_per_objfile *dwarf2_per_objfile) } } -/* Free all allocated queue entries. This function only releases anything if - an error was thrown; if the queue was processed then it would have been - freed as we went along. */ - -static void -dwarf2_release_queue (void *dummy) -{ - struct dwarf2_queue_item *item, *last; - - item = dwarf2_queue; - while (item) - { - /* Anything still marked queued is likely to be in an - inconsistent state, so discard it. */ - if (item->per_cu->queued) - { - if (item->per_cu->cu != NULL) - free_one_cached_comp_unit (item->per_cu); - item->per_cu->queued = 0; - } - - last = item; - item = item->next; - xfree (last); - } - - dwarf2_queue = dwarf2_queue_tail = NULL; -} - /* Read in full symbols for PST, and anything it depends on. */ static void