[patch] Set bfd field in target_section

[patch] Set bfd field in target_section

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

Hello,

I believe this is related to Pedro's patch from 03-Jun-09. I 
didn't see where we set target_section.bfd field - maybe I 
am overlooking something, but in bfd-target, in function 
target_bfd_xclose we will call bfd_close 
(table->sections->bfd); bfd_close doesn't like NULL argument.

Am I missing something, or is this (the patch) missing?

Patch attached.

Thanks,

-- 
Aleksandar Ristovski
QNX Software Systems

ChangeLog:

* exec.c (build_section_table): Setup section_table bfd field.

[-- Attachment #2: exec_c-20090728.diff --]
[-- Type: text/x-patch, Size: 641 bytes --]

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:21:09 -0000
@@ -441,6 +441,7 @@ build_section_table (struct bfd *some_bf
   bfd_map_over_sections (some_bfd, add_to_section_table, (char *) end);
   if (*end > *start + count)
     internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
+  (*start)->bfd = (*end)->bfd = some_bfd;
   /* We could realloc the table, but it probably loses for most files.  */
   return 0;
 }

Re: [patch] Set bfd field in target_section

[Pedro Alves]

On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
> Hello,
> 
> I believe this is related to Pedro's patch from 03-Jun-09. I 
> didn't see where we set target_section.bfd field - maybe I 
> am overlooking something, but in bfd-target, in function 
> target_bfd_xclose we will call bfd_close 
> (table->sections->bfd); bfd_close doesn't like NULL argument.
> 
> Am I missing something, or is this (the patch) missing?

Doesn't add_to_section_table set the bfd in each new
target section?

> 
> Patch attached.
> 
> Thanks,
> 
> -- 
> Aleksandar Ristovski
> QNX Software Systems
> 
> ChangeLog:
> 
> * exec.c (build_section_table): Setup section_table bfd field.
> exec_c-20090728.diff
>   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:21:09 -0000
> @@ -441,6 +441,7 @@ build_section_table (struct bfd *some_bf
>    bfd_map_over_sections (some_bfd, add_to_section_table, (char *) end);
>    if (*end > *start + count)
>      internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
> +  (*start)->bfd = (*end)->bfd = some_bfd;
>    /* We could realloc the table, but it probably loses for most files.  */
>    return 0;
>  }



-- 
Pedro Alves

Re: [patch] Set bfd field in target_section

[Pedro Alves]

On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
> Hello,
> 
> I believe this is related to Pedro's patch from 03-Jun-09. I 
> didn't see where we set target_section.bfd field - maybe I 
> am overlooking something, but in bfd-target, in function 
> target_bfd_xclose we will call bfd_close 
> (table->sections->bfd); bfd_close doesn't like NULL argument.
> 
> Am I missing something, or is this (the patch) missing?

Doesn't add_to_section_table set the bfd in each new
target section?

> 
> Patch attached.
> 
> Thanks,
> 
> -- 
> Aleksandar Ristovski
> QNX Software Systems
> 
> ChangeLog:
> 
> * exec.c (build_section_table): Setup section_table bfd field.
> exec_c-20090728.diff
>   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:21:09 -0000
> @@ -441,6 +441,7 @@ build_section_table (struct bfd *some_bf
>    bfd_map_over_sections (some_bfd, add_to_section_table, (char *) end);
>    if (*end > *start + count)
>      internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
> +  (*start)->bfd = (*end)->bfd = some_bfd;
>    /* We could realloc the table, but it probably loses for most files.  */
>    return 0;
>  }



-- 
Pedro Alves

Re: [patch] Set bfd field in target_section

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
>> Hello,
>>
>> I believe this is related to Pedro's patch from 03-Jun-09. I 
>> didn't see where we set target_section.bfd field - maybe I 
>> am overlooking something, but in bfd-target, in function 
>> target_bfd_xclose we will call bfd_close 
>> (table->sections->bfd); bfd_close doesn't like NULL argument.
>>
>> Am I missing something, or is this (the patch) missing?
> 
> Doesn't add_to_section_table set the bfd in each new
> target section?

Indeed it does. However, the problem is if we don't find any 
sections in a bfd, it will exit and will leave bfd field 0.

I get this situation at the moment because I broke my 
xfer_partial, but I think it could happen in general?

---
Aleksandar

Re: [patch] Set bfd field in target_section

[Pedro Alves]

On Tuesday 28 July 2009 15:45:25, Aleksandar Ristovski wrote:
> Pedro Alves wrote:
> > On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
> >> Hello,
> >>
> >> I believe this is related to Pedro's patch from 03-Jun-09. I 
> >> didn't see where we set target_section.bfd field - maybe I 
> >> am overlooking something, but in bfd-target, in function 
> >> target_bfd_xclose we will call bfd_close 
> >> (table->sections->bfd); bfd_close doesn't like NULL argument.
> >>
> >> Am I missing something, or is this (the patch) missing?
> > 
> > Doesn't add_to_section_table set the bfd in each new
> > target section?
> 
> Indeed it does. However, the problem is if we don't find any 
> sections in a bfd, it will exit and will leave bfd field 0.

Right, but table->sections will be equal to table->sections_end,
meaning the table is empty.  Your fix isn't correct, since you
should never write to *sections_end, which is one-past-the-end
of the sections in the table.  In the degenerate case of
bfd_count_sections == 0 (not 0 ALLOC sections), xmalloc will
still return something non-NULL, but, writing to this pointer
invokes undefined behaviour.

> I get this situation at the moment because I broke my 
> xfer_partial, but I think it could happen in general?

Testing finished succesfully, so I've applied the patch
with this ChangeLog entry.  Let me know if something is still wrong.

2009-07-28  Pedro Alves  <pedro@codesourcery.com>

	* bfd-target.c (target_bfd_xclose): Only close the bfd if the
	section table is not empty.
	(target_bfd_reopen): If the section table ends up empty, close the
	bfd here.

-- 
Pedro Alves

Re: [patch] Set bfd field in target_section

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Tuesday 28 July 2009 15:45:25, Aleksandar Ristovski wrote:
>> Pedro Alves wrote:
>>> On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
>>>> Hello,
>>>>
>>>> I believe this is related to Pedro's patch from 03-Jun-09. I 
>>>> didn't see where we set target_section.bfd field - maybe I 
>>>> am overlooking something, but in bfd-target, in function 
>>>> target_bfd_xclose we will call bfd_close 
>>>> (table->sections->bfd); bfd_close doesn't like NULL argument.
>>>>
>>>> Am I missing something, or is this (the patch) missing?
>>> Doesn't add_to_section_table set the bfd in each new
>>> target section?
>> Indeed it does. However, the problem is if we don't find any 
>> sections in a bfd, it will exit and will leave bfd field 0.
> 
> Right, but table->sections will be equal to table->sections_end,
> meaning the table is empty.  Your fix isn't correct, since you
> should never write to *sections_end, which is one-past-the-end
> of the sections in the table.  In the degenerate case of
> bfd_count_sections == 0 (not 0 ALLOC sections), xmalloc will
> still return something non-NULL, but, writing to this pointer
> invokes undefined behaviour.
> 
>> I get this situation at the moment because I broke my 
>> xfer_partial, but I think it could happen in general?
> 
> Testing finished succesfully, so I've applied the patch
> with this ChangeLog entry.  Let me know if something is still wrong.
> 
> 2009-07-28  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* bfd-target.c (target_bfd_xclose): Only close the bfd if the
> 	section table is not empty.
> 	(target_bfd_reopen): If the section table ends up empty, close the
> 	bfd here.
> 

I think now you broke it in a different way. Now we can end 
up trying to read from a closed bfd.


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);



-- 
Aleksandar Ristovski
QNX Software Systems

Re: [patch] Set bfd field in target_section

[Pedro Alves]

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

Re: [patch] Set bfd field in target_section

[Aleksandar Ristovski]

Pedro Alves wrote:
> 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.
> 

I tried it, it works.


Thanks,
-- 
Aleksandar Ristovski
QNX Software Systems

Re: [patch] Set bfd field in target_section

[Pedro Alves]

On Tuesday 28 July 2009 20:51:34, Aleksandar Ristovski wrote:
> Pedro Alves wrote:
> > 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.
> > 

(actually, there never was such a thing, I must have
dreamed about it. :-)  The bfd used to be stored directly in
target_ops->to_data, and once upon a time, there used to be
target_ops->to_sections|target_ops->to_sections_end pointers.)

> > Could you try it?  Thanks.
> > 

> I tried it, it works.

Thanks, I've applied it.

-- 
Pedro Alves

Re: [patch] Set bfd field in target_section

[Pedro Alves]

On Tuesday 28 July 2009 15:34:41, Pedro Alves wrote:
> On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
> > Hello,
> > 
> > I believe this is related to Pedro's patch from 03-Jun-09. I 
> > didn't see where we set target_section.bfd field - maybe I 
> > am overlooking something, but in bfd-target, in function 
> > target_bfd_xclose we will call bfd_close 
> > (table->sections->bfd); bfd_close doesn't like NULL argument.
> > 
> > Am I missing something, or is this (the patch) missing?
> 
> Doesn't add_to_section_table set the bfd in each new
> target section?

I think you've got yourself an executable without any ALLOC
section, hence the section table ends up empty.  I think something
like this will fix the problem.  I'll give a test spin and
apply it.

-- 
Pedro Alves

---
 gdb/bfd-target.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: src/gdb/bfd-target.c
===================================================================
--- src.orig/gdb/bfd-target.c	2009-07-28 15:43:27.000000000 +0100
+++ src/gdb/bfd-target.c	2009-07-28 15:47:33.000000000 +0100
@@ -54,7 +54,10 @@ static void
 target_bfd_xclose (struct target_ops *t, int quitting)
 {
   struct target_section_table *table = t->to_data;
-  if (table->sections)
+
+  /* If the target sections table is empty, the bfd had already been
+     closed.  */
+  if (table->sections != table->sections_end)
     bfd_close (table->sections->bfd);
   xfree (table->sections);
   xfree (table);
@@ -70,6 +73,12 @@ target_bfd_reopen (struct bfd *bfd)
   table = XZALLOC (struct target_section_table);
   build_section_table (bfd, &table->sections, &table->sections_end);
 
+  /* No use keeping the bfd open if there are no target sections we
+     care about.  This way, we avoid keeping the bfd pointer stored
+     somewhere so that target_bfd_xclose could use it.  */
+  if (table->sections == table->sections_end)
+    bfd_close (bfd);
+
   t = XZALLOC (struct target_ops);
   t->to_shortname = "bfd";
   t->to_longname = _("BFD backed target");

Re: [patch] Set bfd field in target_section

[Aleksandar Ristovski]

Pedro Alves wrote:
> On Tuesday 28 July 2009 15:34:41, Pedro Alves wrote:
>> On Tuesday 28 July 2009 15:28:33, Aleksandar Ristovski wrote:
>>> Hello,
>>>
>>> I believe this is related to Pedro's patch from 03-Jun-09. I 
>>> didn't see where we set target_section.bfd field - maybe I 
>>> am overlooking something, but in bfd-target, in function 
>>> target_bfd_xclose we will call bfd_close 
>>> (table->sections->bfd); bfd_close doesn't like NULL argument.
>>>
>>> Am I missing something, or is this (the patch) missing?
>> Doesn't add_to_section_table set the bfd in each new
>> target section?
> 
> I think you've got yourself an executable without any ALLOC
> section, hence the section table ends up empty.  I think something
> like this will fix the problem.  I'll give a test spin and
> apply it.
> 

Actually, I got it by mismatching libraries.