From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2143 invoked by alias); 19 Jul 2013 00:20:15 -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 2134 invoked by uid 89); 19 Jul 2013 00:20:14 -0000 X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_40,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RDNS_NONE,SPF_PASS,TW_BJ,TW_BM autolearn=no version=3.3.1 Received: from Unknown (HELO mail-vc0-f201.google.com) (209.85.220.201) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 19 Jul 2013 00:20:10 +0000 Received: by mail-vc0-f201.google.com with SMTP id hz11so204204vcb.2 for ; Thu, 18 Jul 2013 17:20:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:mime-version:content-type:content-transfer-encoding:message-id :date:to:cc:subject:in-reply-to:references:x-mailer :x-gm-message-state; bh=JaSEojiLP4lRvxQan+i6xvaiCfE8UqjzZj/ZPZR/mz8=; b=BAjWAwqKYT822ZC1ARAiH4gpPVVvst9PyIv5w7DZPpOH21awHME6KPS/58HS6+NXZP UgZa5dulUAMeR9cY0Na0CDF1mDAt7bjctUTtLt7T9mkZpBEh/BG9WINGKet0SZnVh0sC lsehrSwOwperIy6AzBnM0MDQuXvHJzdTcgShDFwowItjukWKh362FSd13BWnjvSIMgYv zKg2CUgSgcnBmpvB3YNkJKQvXkHjyqIfWdwdjRLGy6GNvZu5dDzo4+a9B6dGq6k/tBTe HLwiJdYCCw+h2PGUBnMwCrZP8oeTxbiI4OSkFePCA04pMb2+Dd8PB8SqoE8+qdKWfAU6 H49g== X-Received: by 10.236.194.33 with SMTP id l21mr7567403yhn.42.1374193202092; Thu, 18 Jul 2013 17:20:02 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id n70si5534755yhj.7.2013.07.18.17.20.02 for (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Thu, 18 Jul 2013 17:20:02 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id 6444B31C111; Thu, 18 Jul 2013 17:20:01 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <20968.34352.845515.406613@ruffy.mtv.corp.google.com> Date: Fri, 19 Jul 2013 00:20:00 -0000 To: jan.kratochvil@redhat.com, tromey@redhat.com Cc: gdb-patches Subject: Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd] In-Reply-To: <20130718163310.GA14833@host2.jankratochvil.net> References: <20130718105435.GA26949@host2.jankratochvil.net> <20130718163310.GA14833@host2.jankratochvil.net> X-Gm-Message-State: ALoCoQn+FsVpr97pP/glkXV1jS7P0X6RS+n7yXkHr555QJexwCAD5jxT4BV0VdSzRZkxVVeJgJhVJA/ArRW079FkwzwcwkLpJN5voy6Eumqtre9pDUu6PswYpV/DzEdJXs1FWB2OtrMavGPL+rt6A3T7ZwU/slBTDPI0cgqPjr4xkHARMU0kvbaKu5rcrp7/Mp+F2eWRZkxfZZ0gMo+MxSTgwqVqZFjm5w== X-SW-Source: 2013-07/txt/msg00463.txt.bz2 Jan Kratochvil writes: > On Thu, 18 Jul 2013 18:24:34 +0200, Doug Evans wrote: > > Blech, I can't recreate this on an ubuntu or fc17 system. > > Tried it on Fedora 17 x86_64 and it is really not reproducible (PASSes) with: > CFLAGS=-g ./configure > > but the regression is reproducible with: > CFLAGS=-g ./configure --enable-libmcheck Ah, thanks. The problem is in this function: void remove_target_sections (void *key, bfd *abfd) { struct target_section *src, *dest; struct target_section_table *table = current_target_sections; dest = table->sections; for (src = table->sections; src < table->sections_end; src++) if (src->key != key || src->the_bfd_section->owner != abfd) { /* Keep this section. */ if (dest < src) *dest = *src; dest++; } When exec_close calls this function the bfd has already been closed. Therefore src->the_bfd_section->owner now contains garbage and so "src->the_bfd_section->owner != abfd" succeeds and we end up keeping the now deleted section. I think yet more cleanup could be applied here, but I'd like to reach a stopping point we can agree on (and I'm not yet willing to revert my patch - though I will if it comes to that of course). There's just so much cleanup that is needed I'd like to make some forward progress ... struct target_section has member "key" that is used to help distinguish sections from different origins (generally, executable and shlibs). The change was added here: http://sourceware.org/ml/gdb-patches/2012-07/msg00742.html Tom: Can you remember the reproducer for the issue fixed by this patch? The "constructor", if you will, is exec.c:add_to_section_table but it sets the key to NULL. It is only when exec.c:add_target_sections is called that key is set to non-NULL, and add_target_sections is *only* called for executables and shlibs. [Seems like another cleanup is to initialize "key" in the constructor, but I'd like to leave that for another day. I can look into that if you want, however.] The only place the key is used is in remove_target_sections, which is also only called for executables and shlibs. So remove_target_sections will never see a non-NULL key. Furthermore, it seems reasonable to me to require that when we're removing sections in remove_target_sections for an object (executable or shlib) it's reasonable from an API point of view to just remove all of them. [And if that doesn't work my first thought is that yet more cleanup is required ...] Note that all calls to remove_target_sections pass the bfd from "key": exec.c: bfd *abfd = exec_bfd; ... remove_target_sections (&exec_bfd, abfd); solib.c: remove_target_sections (gdb, gdb->abfd); remove_target_sections (so, so->abfd); remove_target_sections (so, so->abfd); Thus I think(!) removing the bfd argument to remove_target_sections is reasonable and safe: [I'm showing the pre-target-section.bfd removal version of the code here, since this patch could, I think, be applied even if that patch got reverted.] - if (src->key != key || src->bfd != abfd) + if (src->key != key) It's also a bit cleaner: add_target_sections doesn't take a bfd argument. Could be wrong of course, let me know your thoughts. The name "key" is now a bit less clear than it could be, I like "owner" better. Another change in this patch worth pointing out is in solib.c:clear_solib: - if (so->abfd) - remove_target_sections (so, so->abfd); + remove_target_sections (so); There's no need for the test AFAICT: If the so doesn't have a bfd then it won't be in the target section table. [In which case we'll be calling remove_target_sections unnecessarily, but the speedup is a micro-optimization. Otherwise the reader has to reason about why the test is there: Is it more than just a (micro-)optimization?] So how about this patch? Regression tested on amd64-linux with --enable-libmcheck, and verified it fixes the reread.exp regression. 2013-07-18 Doug Evans * exec.h (remove_target_sections): Delete arg abfd. * exec.c (remove_target_sections): Delete arg abfd. (exec_close): Update call to remove_target_sections. * solib.c (update_solib_list): Ditto. (reload_shared_libraries_1): Ditto. (clear_solib): Ditto, and unconditionally call remove_target_sections. Index: exec.c =================================================================== RCS file: /cvs/src/src/gdb/exec.c,v retrieving revision 1.126 diff -u -p -r1.126 exec.c --- exec.c 16 Jul 2013 20:41:55 -0000 1.126 +++ exec.c 19 Jul 2013 00:11:29 -0000 @@ -101,7 +101,7 @@ exec_close (void) exec_bfd = NULL; exec_bfd_mtime = 0; - remove_target_sections (&exec_bfd, abfd); + remove_target_sections (&exec_bfd); } } @@ -339,7 +339,7 @@ add_to_section_table (bfd *abfd, struct if (!(aflag & SEC_ALLOC)) return; - (*table_pp)->key = NULL; + (*table_pp)->owner = NULL; (*table_pp)->the_bfd_section = asect; (*table_pp)->addr = bfd_section_vma (abfd, asect); (*table_pp)->endaddr = (*table_pp)->addr + bfd_section_size (abfd, asect); @@ -397,7 +397,7 @@ build_section_table (struct bfd *some_bf current set of target sections. */ void -add_target_sections (void *key, +add_target_sections (void *owner, struct target_section *sections, struct target_section *sections_end) { @@ -414,7 +414,7 @@ add_target_sections (void *key, for (i = 0; i < count; ++i) { table->sections[space + i] = sections[i]; - table->sections[space + i].key = key; + table->sections[space + i].owner = owner; } /* If these are the first file sections we can provide memory @@ -427,17 +427,20 @@ add_target_sections (void *key, } } -/* Remove all target sections taken from ABFD. */ +/* Remove all target sections owned by OWNER. + OWNER must be the same value passed to add_target_sections. */ void -remove_target_sections (void *key, bfd *abfd) +remove_target_sections (void *owner) { struct target_section *src, *dest; struct target_section_table *table = current_target_sections; + gdb_assert (owner != NULL); + dest = table->sections; for (src = table->sections; src < table->sections_end; src++) - if (src->key != key || src->the_bfd_section->owner != abfd) + if (src->owner != owner) { /* Keep this section. */ if (dest < src) Index: exec.h =================================================================== RCS file: /cvs/src/src/gdb/exec.h,v retrieving revision 1.20 diff -u -p -r1.20 exec.h --- exec.h 1 Jan 2013 06:32:42 -0000 1.20 +++ exec.h 19 Jul 2013 00:11:29 -0000 @@ -81,14 +81,14 @@ extern int section_table_xfer_memory_par /* Set the loaded address of a section. */ extern void exec_set_section_address (const char *, int, CORE_ADDR); -/* Remove all target sections taken from ABFD. */ +/* Remove all target sections owned by OWNER. */ -extern void remove_target_sections (void *key, bfd *abfd); +extern void remove_target_sections (void *owner); /* Add the sections array defined by [SECTIONS..SECTIONS_END[ to the current set of target sections. */ -extern void add_target_sections (void *key, +extern void add_target_sections (void *owner, struct target_section *sections, struct target_section *sections_end); Index: solib.c =================================================================== RCS file: /cvs/src/src/gdb/solib.c,v retrieving revision 1.176 diff -u -p -r1.176 solib.c --- solib.c 4 Jun 2013 13:17:06 -0000 1.176 +++ solib.c 19 Jul 2013 00:11:29 -0000 @@ -770,7 +770,7 @@ update_solib_list (int from_tty, struct /* Some targets' section tables might be referring to sections from so->abfd; remove them. */ - remove_target_sections (gdb, gdb->abfd); + remove_target_sections (gdb); free_so (gdb); gdb = *gdb_link; @@ -1151,8 +1151,7 @@ clear_solib (void) so_list_head = so->next; observer_notify_solib_unloaded (so); - if (so->abfd) - remove_target_sections (so, so->abfd); + remove_target_sections (so); free_so (so); } @@ -1276,7 +1275,7 @@ reload_shared_libraries_1 (int from_tty) if (so->objfile && ! (so->objfile->flags & OBJF_USERLOADED) && !solib_used (so)) free_objfile (so->objfile); - remove_target_sections (so, so->abfd); + remove_target_sections (so); clear_so (so); } Index: target.h =================================================================== RCS file: /cvs/src/src/gdb/target.h,v retrieving revision 1.265 diff -u -p -r1.265 target.h --- target.h 16 Jul 2013 20:41:55 -0000 1.265 +++ target.h 19 Jul 2013 00:11:29 -0000 @@ -1893,11 +1893,12 @@ struct target_section struct bfd_section *the_bfd_section; - /* A given BFD may appear multiple times in the target section - list, so each BFD is associated with a given key. The key is - just some convenient pointer that can be used to differentiate - the BFDs. These are managed only by convention. */ - void *key; + /* The "owner" of the section. + It can be any unique value. It is set by add_target_sections + and used by remove_target_sections. + For example, for executables it is a pointer to exec_bfd and + for shlibs it is the so_list pointer. */ + void *owner; }; /* Holds an array of target sections. Defined by [SECTIONS..SECTIONS_END[. */