From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13105 invoked by alias); 19 May 2014 21:52:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 13091 invoked by uid 89); 19 May 2014 21:52:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ve0-f169.google.com Received: from mail-ve0-f169.google.com (HELO mail-ve0-f169.google.com) (209.85.128.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 19 May 2014 21:52:18 +0000 Received: by mail-ve0-f169.google.com with SMTP id jx11so7232886veb.14 for ; Mon, 19 May 2014 14:52:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:content-type; bh=p5+Vbo96SNa/MO/7nLcpb7S0eVXlfFscaQ/8uAFS7pM=; b=nBP7aPvHY4j9CAQEqFvdutys0XltYRqru2+npfUgmub0AE8insWBfZ2mF4SzXuRNwa 4FtQxlxMEjbH8F/ke7IsfBSP+LLju8ZmAW6eWgIWd1ml93Y7LFREvVaVG71+cxBwEaRW TIV0pYXt07znuGvYNOXx5fCJonXm+cRMdzVbPdLtQsX0cBDyB7dszzIBQBO1CbsABSPX zgTIli2tGxE651YNb5M1g74yQiu4bViB2/wESNt1o2ee/7kAk1NUgdtX//K8jkmRmd2T l5w4Wy4bYdwumdiUKA8hn9P4qRz62xw6eomH4xfGVaDzJ52uAt/Em5YxTrObuAXaHF19 dS0A== X-Gm-Message-State: ALoCoQmsX7Y8yLqmgdSCUEh/+Cn3Cgq3dUHPSvSZbNJszjLFT1aQTy4LO2Y8/T1DD3s/shSJ1aOF MIME-Version: 1.0 X-Received: by 10.58.1.231 with SMTP id 7mr3989185vep.31.1400536335762; Mon, 19 May 2014 14:52:15 -0700 (PDT) Received: by 10.52.28.230 with HTTP; Mon, 19 May 2014 14:52:15 -0700 (PDT) In-Reply-To: References: Date: Mon, 19 May 2014 21:52:00 -0000 Message-ID: Subject: Re: [PATCH] Fix gdb.multi/base.exp, gdb memory corruption From: Doug Evans To: gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00385.txt.bz2 On Mon, May 19, 2014 at 1:39 PM, Doug Evans wrote: > Hi. > > My prune_inferiors patch > > 2014-05-17 Doug Evans > > * inferior.c (prune_inferiors): Fix comment. > (remove_inferior_command): Call prune_program_spaces. > > is causing gdb.multi/base.exp to fail: > > UNRESOLVED: gdb.multi/base.exp: remove-inferiors 2-3 > UNRESOLVED: gdb.multi/base.exp: check remove-inferiors > > gdb is crashing because it's accessing/freeing already freed memory. > I was running the testsuite all weekend, sorry I only saw this now. > > E.g. > ==16368== Invalid read of size 4 > ==16368== at 0x660A9D: find_pc_section (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/objfiles.c:1349) > ==16368== by 0x663ECB: lookup_minimal_symbol_by_pc_section (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/minsyms.c:734) > ==16368== by 0x5D987A: find_pc_sect_symtab (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/symtab.c:2153) > ==16368== by 0x5D4D77: blockvector_for_pc_sect (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/block.c:168) > ==16368== by 0x5D4F59: block_for_pc_sect (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/block.c:246) > ==16368== by 0x5D4F9B: block_for_pc (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/block.c:258) > ==16368== by 0x734C5D: inline_frame_sniffer (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/inline-frame.c:218) > ==16368== by 0x732104: frame_unwind_try_unwinder (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame-unwind.c:108) > ==16368== by 0x73223F: frame_unwind_find_by_frame (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame-unwind.c:159) > ==16368== by 0x72D5AA: compute_frame_id (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame.c:453) > ==16368== by 0x7300EC: get_prev_frame_if_no_cycle (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame.c:1758) > ==16368== by 0x73079A: get_prev_frame_always (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/frame.c:1931) > ==16368== Address 0x5b13500 is 16 bytes inside a block of size 24 free'd > ==16368== at 0x403072E: free (valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c:445) > ==16368== by 0x762134: xfree (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/common/common-utils.c:108) > ==16368== by 0x65DACF: objfiles_pspace_data_cleanup (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/objfiles.c:91) > ==16368== by 0x75E546: program_spaceregistry_callback_adaptor (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:45) > ==16368== by 0x7644F6: registry_clear_data (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/registry.c:82) > ==16368== by 0x7645AB: registry_container_free_data (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/registry.c:95) > ==16368== by 0x75E5B4: program_space_free_data (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:45) > ==16368== by 0x75E9BA: release_program_space (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:167) > ==16368== by 0x75EB9B: prune_program_spaces (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/progspace.c:269) > ==16368== by 0x75303D: remove_inferior_command (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/inferior.c:792) > ==16368== by 0x50B5FD: do_cfunc (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/cli/cli-decode.c:107) > ==16368== by 0x50E6F2: cmd_func (/usr/local/lib/debug/debug-src/obj64/gdb/../../binutils-gdb/gdb/cli/cli-decode.c:1886) > > The problem originates from the get_current_arch call in > py-progspace.c:py_free_pspace. > I think the comment in this patch explains things pretty well. > Basically, we're in the pspace destructor, and thus there's not much we can > rely on. The Python machinery needs an arch, so we give it one, > albeit a fiction. I think(!) it doesn't matter. > I'm going with this fix because it's not clear to me what The Right fix > is, short of removing global state in gdb which is non-trivial. > Since "there is always an inferior" calling target_gdbarch seems > pretty safe here. > > 2014-05-19 Doug Evans > > * python/py-progspace.c (py_free_pspace): Use target_gdbarch instead > of get_current_arch. > > diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c > index cda5a86..c787c69 100644 > --- a/gdb/python/py-progspace.c > +++ b/gdb/python/py-progspace.c > @@ -241,7 +241,16 @@ py_free_pspace (struct program_space *pspace, void *datum) > { > struct cleanup *cleanup; > pspace_object *object = datum; > - struct gdbarch *arch = get_current_arch (); > + /* This is a fiction, but we're in a nasty spot: The pspace is in the > + process of being deleted, we can't rely on anything in it. Plus > + this is one time when the current program space and current inferior > + are not in sync. The architecture comes from the inferior, which cannot > + be the current one because we wouldn't be deleting its pspace. > + We don't need to do much here so this fiction suffices. > + Note: We cannot call get_current_arch because it may try to access > + the target, which may involve accessing data in the pspace currently > + being deleted. */ > + struct gdbarch *arch = target_gdbarch (); > > cleanup = ensure_python_env (arch, current_language); > object->pspace = NULL; Hmmm... I looked at py_free_inferior to see what it does: ---snip--- static void py_free_inferior (struct inferior *inf, void *datum) { struct cleanup *cleanup; inferior_object *inf_obj = datum; struct threadlist_entry *th_entry, *th_tmp; if (!gdb_python_initialized) return; cleanup = ensure_python_env (python_gdbarch, python_language); ---snip--- Just passing python_gdbarch back into ensure_python_env doesn't feel right. One might try to get the arch from INF but it is being destructed and we it would be preferable to not have to rely on it being available. One solution would be to have a special "dummy" or some such arch for use during these kinds of situations. Plus I see python_on_normal_stop calling get_current_arch whereas python_on_resume calling target_gdbarch. There are no comments in the code explaining why things are the way they are, thus it's hard to reason whether these choices have been deliberately made or are essentially random. It could be the case that either choice is fine because it will never be used, but IWBN to have that documented (or better yet just be consistent and document it some place). It feels like there's a good cleanup project here. Am I missing something?