From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29008 invoked by alias); 28 Jul 2009 15:57:42 -0000 Received: (qmail 28999 invoked by uid 22791); 28 Jul 2009 15:57:41 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 28 Jul 2009 15:57:34 +0000 Received: (qmail 25357 invoked from network); 28 Jul 2009 15:57:30 -0000 Received: from unknown (HELO orlando) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 28 Jul 2009 15:57:30 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch] Set bfd field in target_section Date: Tue, 28 Jul 2009 16:37:00 -0000 User-Agent: KMail/1.9.10 Cc: Aleksandar Ristovski References: <200907281606.05571.pedro@codesourcery.com> <4A6F1647.70907@qnx.com> In-Reply-To: <4A6F1647.70907@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200907281657.29337.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2009-07/txt/msg00684.txt.bz2 On Tuesday 28 July 2009 16:16:23, Aleksandar Ristovski wrote: > Pedro Alves wrote: > I think now you broke it in a different way. Now we can end > up trying to read from a closed bfd. Ooops, you're right. There are uses of the bfd after transfering its ownership to the bfd target with target_bfd_reopen, and closing the bfd target (target_close)... This target is modeled on fdopen, which is why I blindly assumed it wasn't happening. I've reverted that patch. > And just wondering, why not simply: > > Index: gdb/exec.c > =================================================================== > RCS file: /cvs/src/src/gdb/exec.c,v > retrieving revision 1.90 > diff -u -p -r1.90 exec.c > --- gdb/exec.c 2 Jul 2009 17:21:06 -0000 1.90 > +++ gdb/exec.c 28 Jul 2009 14:58:16 -0000 > @@ -381,6 +381,7 @@ add_to_section_table (bfd *abfd, struct > struct target_section **table_pp = (struct > target_section **) table_pp_char; > flagword aflag; > > + (*table_pp)->bfd = abfd; > /* Check the section flags, but do not discard > zero-length sections, since > some symbols may still be attached to this section. > For instance, we > encountered on sparc-solaris 2.10 a shared library > with an empty .bss > @@ -390,7 +391,6 @@ add_to_section_table (bfd *abfd, struct > if (!(aflag & SEC_ALLOC)) > return; > > - (*table_pp)->bfd = abfd; > (*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); Still not right, because that's still papering over the problem, and still hitting undefined behaviour, in the case of the table ending up being empty. When table->sections == table->sections_end, we should never dereference table->sections, like in 'table->sections->bfd'. Notice how the end pointer isn't advanced if !SEC_ALLOC. Consider the fact that we usually xmalloc enough memory that makes that not misbehave a coincidence. E.g., see that with a resize_section_table call, you can end up with table->sections == NULL, and table->section_end == NULL, which is legal, because table->sections is still equal to table->sections_end in that case (meaning empty table). There used to be an extra structure in bfd_target.c to hold the sections and the bfd, which didn't look necessary when I reimplemented bfd-target.c on top of exec.c's functions, but it is clear now that we need it anyway. Here's a patch that brings something like that back. Could you try it? Thanks. -- Pedro Alves 2009-07-28 Pedro Alves * bfd-target.c (struct target_bfd_data): New. (target_bfd_xfer_partial): Adjust to get at the section table from the new structure. (target_bfd_get_section_table): Ditto. (target_bfd_xclose): Ditto. Get the bfd pointer from the target_bfd_data structure, from the section table. (target_bfd_reopen): Store a struct target_bfd_data in the target_ops to_data field, instead of a target_section_table. --- gdb/bfd-target.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) Index: src/gdb/bfd-target.c =================================================================== --- src.orig/gdb/bfd-target.c 2009-07-28 16:20:59.000000000 +0100 +++ src/gdb/bfd-target.c 2009-07-28 16:48:12.000000000 +0100 @@ -22,6 +22,19 @@ #include "bfd-target.h" #include "exec.h" +/* The object that is stored in the target_ops->to_data field has this + type. */ +struct target_bfd_data +{ + /* The BFD we're wrapping. */ + struct bfd *bfd; + + /* The section table build from the ALLOC sections in BFD. Note + that we rely on extracting the BFD from a random section in the + table, since the table can be legitimately empty. */ + struct target_section_table table; +}; + static LONGEST target_bfd_xfer_partial (struct target_ops *ops, enum target_object object, @@ -33,10 +46,10 @@ target_bfd_xfer_partial (struct target_o { case TARGET_OBJECT_MEMORY: { - struct target_section_table *table = ops->to_data; + struct target_bfd_data *data = ops->to_data; return section_table_xfer_memory_partial (readbuf, writebuf, offset, len, - table->sections, - table->sections_end, + data->table.sections, + data->table.sections_end, NULL); } default: @@ -47,17 +60,18 @@ target_bfd_xfer_partial (struct target_o static struct target_section_table * target_bfd_get_section_table (struct target_ops *ops) { - return ops->to_data; + struct target_bfd_data *data = ops->to_data; + return &data->table; } static void target_bfd_xclose (struct target_ops *t, int quitting) { - struct target_section_table *table = t->to_data; - if (table->sections) - bfd_close (table->sections->bfd); - xfree (table->sections); - xfree (table); + struct target_bfd_data *data = t->to_data; + + bfd_close (data->bfd); + xfree (data->table.sections); + xfree (data); xfree (t); } @@ -65,10 +79,11 @@ struct target_ops * target_bfd_reopen (struct bfd *bfd) { struct target_ops *t; - struct target_section_table *table; + struct target_bfd_data *data; - table = XZALLOC (struct target_section_table); - build_section_table (bfd, &table->sections, &table->sections_end); + data = XZALLOC (struct target_bfd_data); + data->bfd = bfd; + build_section_table (bfd, &data->table.sections, &data->table.sections_end); t = XZALLOC (struct target_ops); t->to_shortname = "bfd"; @@ -77,7 +92,7 @@ target_bfd_reopen (struct bfd *bfd) t->to_get_section_table = target_bfd_get_section_table; t->to_xfer_partial = target_bfd_xfer_partial; t->to_xclose = target_bfd_xclose; - t->to_data = table; + t->to_data = data; return t; }