This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Set bfd field in target_section


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  <pedro@codesourcery.com>

	* 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;
 }


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]