[RFA] Remove target_section.bfd

[RFA] Remove target_section.bfd

[Doug Evans]

Hi.
While reviewing the remove-symbol-file patchset I came across
target_section.bfd.
Unnecessarily storing copies of things can lead to confusion and bugs.
This patch isn't intended to be a space savings, simply a cleanup.

We already liberally reference bfd_section.owner, so I have no problem
with adding more references here.
[Ideally bfd would provide accessor functions/macros for struct
bfd_section, but none exist at all, and I'm not inclined to add all of
them just for this patch.]

Regression tested on amd64-linux.

Ok to check in?

2013-07-15  Doug Evans  <dje@google.com>

	* target.h (struct target_section): Delete member bfd.
	All users updated to use the_bfd_section->owner instead.
	* exec.c (add_to_section_table): Assert bfd is expected value.
	Remove initialization of target_section.bfd.
	(remove_target_sections): Update.
	(section_table_available_memory): Update.
	(section_table_xfer_memory_partial): Update.
	(print_section_info): Update.
	(exec_set_section_address): Update.
	* record-full.c (record_full_core_xfer_partial): Update.
	* solib-svr4.c (svr4_relocate_section_addresses): Update.
	* solib-target.c (solib_target_relocate_section_addresses): Update.
	* symfile.c (build_section_addr_info_from_section_table): Update.
	* target.c (memory_xfer_live_readonly_partial): Update.
	(memory_xfer_partial_1): Update.

Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.264
diff -u -p -r1.264 target.h
--- target.h	27 Jun 2013 19:52:41 -0000	1.264
+++ target.h	15 Jul 2013 22:30:09 -0000
@@ -1898,8 +1898,6 @@ struct target_section
        just some convenient pointer that can be used to differentiate
        the BFDs.  These are managed only by convention.  */
     void *key;
-
-    bfd *bfd;			/* BFD file pointer */
   };
 
 /* Holds an array of target sections.  Defined by [SECTIONS..SECTIONS_END[.  */
Index: exec.c
===================================================================
RCS file: /cvs/src/src/gdb/exec.c,v
retrieving revision 1.125
diff -u -p -r1.125 exec.c
--- exec.c	6 May 2013 14:15:49 -0000	1.125
+++ exec.c	15 Jul 2013 22:30:08 -0000
@@ -328,6 +328,8 @@ add_to_section_table (bfd *abfd, struct 
   struct target_section **table_pp = (struct target_section **) table_pp_char;
   flagword aflag;
 
+  gdb_assert (abfd == asect->owner);
+
   /* 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
@@ -338,7 +340,6 @@ add_to_section_table (bfd *abfd, struct 
     return;
 
   (*table_pp)->key = NULL;
-  (*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);
@@ -436,7 +437,7 @@ remove_target_sections (void *key, bfd *
 
   dest = table->sections;
   for (src = table->sections; src < table->sections_end; src++)
-    if (src->key != key || src->bfd != abfd)
+    if (src->key != key || src->the_bfd_section->owner != abfd)
       {
 	/* Keep this section.  */
 	if (dest < src)
@@ -479,7 +480,8 @@ section_table_available_memory (VEC(mem_
 
   for (p = sections; p < sections_end; p++)
     {
-      if ((bfd_get_section_flags (p->bfd, p->the_bfd_section)
+      if ((bfd_get_section_flags (p->the_bfd_section->owner,
+				  p->the_bfd_section)
 	   & SEC_READONLY) == 0)
 	continue;
 
@@ -523,7 +525,10 @@ section_table_xfer_memory_partial (gdb_b
 
   for (p = sections; p < sections_end; p++)
     {
-      if (section_name && strcmp (section_name, p->the_bfd_section->name) != 0)
+      struct bfd_section *asect = p->the_bfd_section;
+      bfd *abfd = asect->owner;
+
+      if (section_name && strcmp (section_name, asect->name) != 0)
 	continue;		/* not the section we need.  */
       if (memaddr >= p->addr)
         {
@@ -531,11 +536,11 @@ section_table_xfer_memory_partial (gdb_b
 	    {
 	      /* Entire transfer is within this section.  */
 	      if (writebuf)
-		res = bfd_set_section_contents (p->bfd, p->the_bfd_section,
+		res = bfd_set_section_contents (abfd, asect,
 						writebuf, memaddr - p->addr,
 						len);
 	      else
-		res = bfd_get_section_contents (p->bfd, p->the_bfd_section,
+		res = bfd_get_section_contents (abfd, asect,
 						readbuf, memaddr - p->addr,
 						len);
 	      return (res != 0) ? len : 0;
@@ -550,11 +555,11 @@ section_table_xfer_memory_partial (gdb_b
 	      /* This section overlaps the transfer.  Just do half.  */
 	      len = p->endaddr - memaddr;
 	      if (writebuf)
-		res = bfd_set_section_contents (p->bfd, p->the_bfd_section,
+		res = bfd_set_section_contents (abfd, asect,
 						writebuf, memaddr - p->addr,
 						len);
 	      else
-		res = bfd_get_section_contents (p->bfd, p->the_bfd_section,
+		res = bfd_get_section_contents (abfd, asect,
 						readbuf, memaddr - p->addr,
 						len);
 	      return (res != 0) ? len : 0;
@@ -610,17 +615,18 @@ print_section_info (struct target_sectio
 
       for (p = t->sections; p < t->sections_end; p++)
 	{
-	  asection *asect = p->the_bfd_section;
+	  struct bfd_section *psect = p->the_bfd_section;
+	  bfd *pbfd = psect->owner;
 
-	  if ((bfd_get_section_flags (abfd, asect) & (SEC_ALLOC | SEC_LOAD))
+	  if ((bfd_get_section_flags (pbfd, psect) & (SEC_ALLOC | SEC_LOAD))
 	      != (SEC_ALLOC | SEC_LOAD))
 	    continue;
 
-	  if (bfd_get_section_vma (abfd, asect) <= abfd->start_address
-	      && abfd->start_address < (bfd_get_section_vma (abfd, asect)
-					+ bfd_get_section_size (asect)))
+	  if (bfd_get_section_vma (pbfd, psect) <= abfd->start_address
+	      && abfd->start_address < (bfd_get_section_vma (pbfd, psect)
+					+ bfd_get_section_size (psect)))
 	    {
-	      displacement = p->addr - bfd_get_section_vma (abfd, asect);
+	      displacement = p->addr - bfd_get_section_vma (pbfd, psect);
 	      break;
 	    }
 	}
@@ -636,6 +642,9 @@ print_section_info (struct target_sectio
     }
   for (p = t->sections; p < t->sections_end; p++)
     {
+      struct bfd_section *psect = p->the_bfd_section;
+      bfd *pbfd = psect->owner;
+
       printf_filtered ("\t%s", hex_string_custom (p->addr, wid));
       printf_filtered (" - %s", hex_string_custom (p->endaddr, wid));
 
@@ -647,11 +656,10 @@ print_section_info (struct target_sectio
       /* FIXME: i18n: Need to rewrite this sentence.  */
       if (info_verbose)
 	printf_filtered (" @ %s",
-			 hex_string_custom (p->the_bfd_section->filepos, 8));
-      printf_filtered (" is %s", bfd_section_name (p->bfd,
-						   p->the_bfd_section));
-      if (p->bfd != abfd)
-	printf_filtered (" in %s", bfd_get_filename (p->bfd));
+			 hex_string_custom (psect->filepos, 8));
+      printf_filtered (" is %s", bfd_section_name (pbfd, psect));
+      if (pbfd != abfd)
+	printf_filtered (" in %s", bfd_get_filename (pbfd));
       printf_filtered ("\n");
     }
 }
@@ -720,7 +728,7 @@ exec_set_section_address (const char *fi
   table = current_target_sections;
   for (p = table->sections; p < table->sections_end; p++)
     {
-      if (filename_cmp (filename, p->bfd->filename) == 0
+      if (filename_cmp (filename, p->the_bfd_section->owner->filename) == 0
 	  && index == p->the_bfd_section->index)
 	{
 	  p->endaddr += address - p->addr;
Index: record-full.c
===================================================================
RCS file: /cvs/src/src/gdb/record-full.c,v
retrieving revision 1.9
diff -u -p -r1.9 record-full.c
--- record-full.c	14 May 2013 20:30:48 -0000	1.9
+++ record-full.c	15 Jul 2013 22:30:08 -0000
@@ -2219,9 +2219,10 @@ record_full_core_xfer_partial (struct ta
 			    xmalloc
 			    (sizeof (struct record_full_core_buf_entry));
 			  entry->p = p;
-			  if (!bfd_malloc_and_get_section (p->bfd,
-							   p->the_bfd_section,
-							   &entry->buf))
+			  if (!bfd_malloc_and_get_section
+			        (p->the_bfd_section->owner,
+				 p->the_bfd_section,
+				 &entry->buf))
 			    {
 			      xfree (entry);
 			      return 0;
Index: solib-svr4.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-svr4.c,v
retrieving revision 1.180
diff -u -p -r1.180 solib-svr4.c
--- solib-svr4.c	24 Jun 2013 22:18:32 -0000	1.180
+++ solib-svr4.c	15 Jul 2013 22:30:09 -0000
@@ -2907,10 +2907,10 @@ static void
 svr4_relocate_section_addresses (struct so_list *so,
                                  struct target_section *sec)
 {
-  sec->addr    = svr4_truncate_ptr (sec->addr    + lm_addr_check (so,
-								  sec->bfd));
-  sec->endaddr = svr4_truncate_ptr (sec->endaddr + lm_addr_check (so,
-								  sec->bfd));
+  bfd *abfd = sec->the_bfd_section->owner;
+
+  sec->addr = svr4_truncate_ptr (sec->addr + lm_addr_check (so, abfd));
+  sec->endaddr = svr4_truncate_ptr (sec->endaddr + lm_addr_check (so, abfd));
 }
 \f
 
Index: solib-target.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-target.c,v
retrieving revision 1.30
diff -u -p -r1.30 solib-target.c
--- solib-target.c	24 Jun 2013 22:18:32 -0000	1.30
+++ solib-target.c	15 Jul 2013 22:30:09 -0000
@@ -456,8 +456,9 @@ Could not relocate shared library \"%s\"
 	}
     }
 
-  offset = so->lm_info->offsets->offsets[gdb_bfd_section_index (sec->bfd,
-								sec->the_bfd_section)];
+  offset = so->lm_info->offsets->offsets[gdb_bfd_section_index
+					 (sec->the_bfd_section->owner,
+					  sec->the_bfd_section)];
   sec->addr += offset;
   sec->endaddr += offset;
 }
Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.380
diff -u -p -r1.380 symfile.c
--- symfile.c	19 Jun 2013 22:20:58 -0000	1.380
+++ symfile.c	15 Jul 2013 22:30:09 -0000
@@ -229,15 +229,15 @@ build_section_addr_info_from_section_tab
 
   for (stp = start, oidx = 0; stp != end; stp++)
     {
-      if (bfd_get_section_flags (stp->bfd,
-				 stp->the_bfd_section) & (SEC_ALLOC | SEC_LOAD)
+      struct bfd_section *asect = stp->the_bfd_section;
+      bfd *abfd = asect->owner;
+
+      if (bfd_get_section_flags (abfd, asect) & (SEC_ALLOC | SEC_LOAD)
 	  && oidx < end - start)
 	{
 	  sap->other[oidx].addr = stp->addr;
-	  sap->other[oidx].name
-	    = xstrdup (bfd_section_name (stp->bfd, stp->the_bfd_section));
-	  sap->other[oidx].sectindex
-	    = gdb_bfd_section_index (stp->bfd, stp->the_bfd_section);
+	  sap->other[oidx].name = xstrdup (bfd_section_name (abfd, asect));
+	  sap->other[oidx].sectindex = gdb_bfd_section_index (abfd, asect);
 	  oidx++;
 	}
     }
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.338
diff -u -p -r1.338 target.c
--- target.c	27 Jun 2013 19:52:41 -0000	1.338
+++ target.c	15 Jul 2013 22:30:09 -0000
@@ -1396,7 +1396,8 @@ memory_xfer_live_readonly_partial (struc
 
   secp = target_section_by_addr (ops, memaddr);
   if (secp != NULL
-      && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
+      && (bfd_get_section_flags (secp->the_bfd_section->owner,
+				 secp->the_bfd_section)
 	  & SEC_READONLY))
     {
       struct target_section *p;
@@ -1475,7 +1476,8 @@ memory_xfer_partial_1 (struct target_ops
 
       secp = target_section_by_addr (ops, memaddr);
       if (secp != NULL
-	  && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
+	  && (bfd_get_section_flags (secp->the_bfd_section->owner,
+				     secp->the_bfd_section)
 	      & SEC_READONLY))
 	{
 	  table = target_get_section_table (ops);

Re: [RFA] Remove target_section.bfd

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> While reviewing the remove-symbol-file patchset I came across
Doug> target_section.bfd.
Doug> Unnecessarily storing copies of things can lead to confusion and bugs.
Doug> This patch isn't intended to be a space savings, simply a cleanup.

I think it is a nice cleanup.  Thanks.

Doug> We already liberally reference bfd_section.owner, so I have no problem
Doug> with adding more references here.

The only danger is that some BFD sections do not have an owner.  For
example, this is true of the absolute section, which is shared by all
BFDs, something I found out the hard way.  Now, I think this is wrong of
BFD to do, but fixing it seemed hard.

Sometimes it is safe to use the section owner nevertheless; for instance
if you're sure that an ownerless section will never be used in this
context.

I don't know whether this applies in this case.  Perhaps not because
build_section_table uses bfd_map_over_sections, and I think that doesn't
include the ownerless sections.

Doug> [Ideally bfd would provide accessor functions/macros for struct
Doug> bfd_section, but none exist at all, and I'm not inclined to add all of
Doug> them just for this patch.]

I went through that same thought process.

Doug> Ok to check in?

I think it is fine; but if you could verify about bfd_map_over_sections,
that would be good.

Tom

Re: [RFA] Remove target_section.bfd

[Doug Evans]

On Tue, Jul 16, 2013 at 11:13 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> While reviewing the remove-symbol-file patchset I came across
> Doug> target_section.bfd.
> Doug> Unnecessarily storing copies of things can lead to confusion and bugs.
> Doug> This patch isn't intended to be a space savings, simply a cleanup.
>
> I think it is a nice cleanup.  Thanks.
>
> Doug> We already liberally reference bfd_section.owner, so I have no problem
> Doug> with adding more references here.
>
> The only danger is that some BFD sections do not have an owner.  For
> example, this is true of the absolute section, which is shared by all
> BFDs, something I found out the hard way.  Now, I think this is wrong of
> BFD to do, but fixing it seemed hard.

Righto.  I've taken those into account (or at least tried to ...).

[There's a FIXME in bfd for this btw.  Don't know if it'll ever get
implemented.]

> Sometimes it is safe to use the section owner nevertheless; for instance
> if you're sure that an ownerless section will never be used in this
> context.
>
> I don't know whether this applies in this case.  Perhaps not because
> build_section_table uses bfd_map_over_sections, and I think that doesn't
> include the ownerless sections.

I looked at this.

bfd_map_over_sections does this:

void
bfd_map_over_sections (bfd *abfd,
                       void (*operation) (bfd *, asection *, void *),
                       void *user_storage)
{
  asection *sect;
  unsigned int i = 0;

  for (sect = abfd->sections; sect != NULL; i++, sect = sect->next)
    (*operation) (abfd, sect, user_storage);

  if (i != abfd->section_count) /* Debugging */
    abort ();
}

That can't work for "global" sections (abs,com,und, etc.).
And that's the only way the sections we care about here get added in
gdb (AFAICT).

> Doug> [Ideally bfd would provide accessor functions/macros for struct
> Doug> bfd_section, but none exist at all, and I'm not inclined to add all of
> Doug> them just for this patch.]
>
> I went through that same thought process.
>
> Doug> Ok to check in?
>
> I think it is fine; but if you could verify about bfd_map_over_sections,
> that would be good.
>
> Tom

Re: [RFA] Remove target_section.bfd

[Tom Tromey]

Doug> Righto.  I've taken those into account (or at least tried to ...).

Super, thanks.  I think it is good.

Doug> [There's a FIXME in bfd for this btw.  Don't know if it'll ever get
Doug> implemented.]

It seems unlikely at this point :}

Tom

Build regression with --enable-targets=all [Re: [RFA] Remove target_section.bfd]

[Jan Kratochvil]

On Tue, 16 Jul 2013 00:42:33 +0200, Doug Evans wrote:
> 2013-07-15  Doug Evans  <dje@google.com>
> 
> 	* target.h (struct target_section): Delete member bfd.
> 	All users updated to use the_bfd_section->owner instead.
> 	* exec.c (add_to_section_table): Assert bfd is expected value.
> 	Remove initialization of target_section.bfd.
> 	(remove_target_sections): Update.
> 	(section_table_available_memory): Update.
> 	(section_table_xfer_memory_partial): Update.
> 	(print_section_info): Update.
> 	(exec_set_section_address): Update.
> 	* record-full.c (record_full_core_xfer_partial): Update.
> 	* solib-svr4.c (svr4_relocate_section_addresses): Update.
> 	* solib-target.c (solib_target_relocate_section_addresses): Update.
> 	* symfile.c (build_section_addr_info_from_section_table): Update.
> 	* target.c (memory_xfer_live_readonly_partial): Update.
> 	(memory_xfer_partial_1): Update.

With --enable-targets=all I get:

nto-tdep.c: In function ‘nto_relocate_section_addresses’:
nto-tdep.c:309:48: error: ‘struct target_section’ has no member named ‘bfd’
make: *** [nto-tdep.o] Error 1
ppc64-tdep.c: In function ‘ppc64_convert_from_func_ptr_addr’:
ppc64-tdep.c:386:40: error: ‘struct target_section’ has no member named ‘bfd’
make: *** [ppc64-tdep.o] Error 1
solib-aix.c: In function ‘solib_aix_relocate_section_addresses’:
solib-aix.c:393:18: error: ‘struct target_section’ has no member named ‘bfd’
make: *** [solib-aix.o] Error 1
s390-tdep.c: In function ‘s390_load’:
s390-tdep.c:1182:15: error: ‘struct target_section’ has no member named ‘bfd’
s390-tdep.c:1182:15: error: left-hand operand of comma expression has no effect [-Werror=unused-value]
cc1: all warnings being treated as errors

You need gcc<=4.7 as otherwise you will hit:
	build regression w/ --enable-targets=all && gcc-4.8+ [Re: [committed] Add structures to describe MIPS operands]
	http://sourceware.org/ml/binutils/2013-07/msg00154.html
	Message-ID: <20130717042139.GA2444@host2.jankratochvil.net>


Jan

Re: Build regression with --enable-targets=all [Re: [RFA] Remove target_section.bfd]

[Doug Evans]

Jan Kratochvil writes:
 > On Tue, 16 Jul 2013 00:42:33 +0200, Doug Evans wrote:
 > > 2013-07-15  Doug Evans  <dje@google.com>
 > > 
 > > 	* target.h (struct target_section): Delete member bfd.
 > > 	All users updated to use the_bfd_section->owner instead.
 > > 	* exec.c (add_to_section_table): Assert bfd is expected value.
 > > 	Remove initialization of target_section.bfd.
 > > 	(remove_target_sections): Update.
 > > 	(section_table_available_memory): Update.
 > > 	(section_table_xfer_memory_partial): Update.
 > > 	(print_section_info): Update.
 > > 	(exec_set_section_address): Update.
 > > 	* record-full.c (record_full_core_xfer_partial): Update.
 > > 	* solib-svr4.c (svr4_relocate_section_addresses): Update.
 > > 	* solib-target.c (solib_target_relocate_section_addresses): Update.
 > > 	* symfile.c (build_section_addr_info_from_section_table): Update.
 > > 	* target.c (memory_xfer_live_readonly_partial): Update.
 > > 	(memory_xfer_partial_1): Update.
 > 
 > With --enable-targets=all I get:
 > 
 > nto-tdep.c: In function \x18nto_relocate_section_addresses\x19:
 > nto-tdep.c:309:48: error: \x18struct target_section\x19 has no member named \x18bfd\x19
 > make: *** [nto-tdep.o] Error 1
 > ppc64-tdep.c: In function \x18ppc64_convert_from_func_ptr_addr\x19:
 > ppc64-tdep.c:386:40: error: \x18struct target_section\x19 has no member named \x18bfd\x19
 > make: *** [ppc64-tdep.o] Error 1
 > solib-aix.c: In function \x18solib_aix_relocate_section_addresses\x19:
 > solib-aix.c:393:18: error: \x18struct target_section\x19 has no member named \x18bfd\x19
 > make: *** [solib-aix.o] Error 1
 > s390-tdep.c: In function \x18s390_load\x19:
 > s390-tdep.c:1182:15: error: \x18struct target_section\x19 has no member named \x18bfd\x19
 > s390-tdep.c:1182:15: error: left-hand operand of comma expression has no effect [-Werror=unused-value]
 > cc1: all warnings being treated as errors

Yikes.  Committed.

2013-07-16  Doug Evans  <dje@google.com>

	* nto-tdep.c (nto_relocate_section_addresses): Update,
	target_section.bfd deleted.
	* ppc64-tdep.c (ppc64_convert_from_func_ptr_addr): Ditto.
	* s390-tdep.c (s390_load): Ditto.
	* solib-aix.c (solib_aix_relocate_section_addresses): Ditto.

Index: nto-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/nto-tdep.c,v
retrieving revision 1.50
diff -u -p -r1.50 nto-tdep.c
--- nto-tdep.c	16 Jul 2013 20:43:48 -0000	1.50
+++ nto-tdep.c	17 Jul 2013 05:20:19 -0000
@@ -306,7 +306,7 @@ nto_relocate_section_addresses (struct s
   /* Neutrino treats the l_addr base address field in link.h as different than
      the base address in the System V ABI and so the offset needs to be
      calculated and applied to relocations.  */
-  Elf_Internal_Phdr *phdr = find_load_phdr (sec->bfd);
+  Elf_Internal_Phdr *phdr = find_load_phdr (sec->the_bfd_section->owner);
   unsigned vaddr = phdr ? phdr->p_vaddr : 0;
 
   sec->addr = nto_truncate_ptr (sec->addr + lm_addr (so) - vaddr);
Index: ppc64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc64-tdep.c,v
retrieving revision 1.3
diff -u -p -r1.3 ppc64-tdep.c
--- ppc64-tdep.c	4 Jun 2013 02:44:35 -0000	1.3
+++ ppc64-tdep.c	17 Jul 2013 05:20:19 -0000
@@ -383,7 +383,8 @@ ppc64_convert_from_func_ptr_addr (struct
       gdb_byte buf[8];
       int res;
 
-      res = bfd_get_section_contents (s->bfd, s->the_bfd_section,
+      res = bfd_get_section_contents (s->the_bfd_section->owner,
+				      s->the_bfd_section,
 				      &buf, addr - s->addr, 8);
       if (res != 0)
 	return extract_unsigned_integer (buf, 8, byte_order)
Index: s390-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/s390-tdep.c,v
retrieving revision 1.212
diff -u -p -r1.212 s390-tdep.c
--- s390-tdep.c	24 Jun 2013 22:18:31 -0000	1.212
+++ s390-tdep.c	17 Jul 2013 05:20:19 -0000
@@ -1179,7 +1179,8 @@ s390_load (struct s390_prologue_data *da
       struct target_section *secp;
       secp = target_section_by_addr (&current_target, addr.k);
       if (secp != NULL
-          && (bfd_get_section_flags (secp->bfd, secp->the_bfd_section)
+          && (bfd_get_section_flags (secp->the_bfd_section->owner,
+				     secp->the_bfd_section)
               & SEC_READONLY))
         return pv_constant (read_memory_integer (addr.k, size,
 						 data->byte_order));
Index: solib-aix.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-aix.c,v
retrieving revision 1.7
diff -u -p -r1.7 solib-aix.c
--- solib-aix.c	30 May 2013 17:31:00 -0000	1.7
+++ solib-aix.c	17 Jul 2013 05:20:19 -0000
@@ -390,8 +390,8 @@ static void
 solib_aix_relocate_section_addresses (struct so_list *so,
 				      struct target_section *sec)
 {
-  bfd *abfd = sec->bfd;
   struct bfd_section *bfd_sect = sec->the_bfd_section;
+  bfd *abfd = bfd_sect->owner;
   const char *section_name = bfd_section_name (abfd, bfd_sect);
   struct lm_info *info = so->lm_info;
 

Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Jan Kratochvil]

On Tue, 16 Jul 2013 00:42:33 +0200, Doug Evans wrote:
> 2013-07-15  Doug Evans  <dje@google.com>
> 
> 	* target.h (struct target_section): Delete member bfd.
> 	All users updated to use the_bfd_section->owner instead.
> 	* exec.c (add_to_section_table): Assert bfd is expected value.
> 	Remove initialization of target_section.bfd.
> 	(remove_target_sections): Update.
> 	(section_table_available_memory): Update.
> 	(section_table_xfer_memory_partial): Update.
> 	(print_section_info): Update.
> 	(exec_set_section_address): Update.
> 	* record-full.c (record_full_core_xfer_partial): Update.
> 	* solib-svr4.c (svr4_relocate_section_addresses): Update.
> 	* solib-target.c (solib_target_relocate_section_addresses): Update.
> 	* symfile.c (build_section_addr_info_from_section_table): Update.
> 	* target.c (memory_xfer_live_readonly_partial): Update.
> 	(memory_xfer_partial_1): Update.

 Running gdb/testsuite/gdb.base/reread.exp ...
-PASS: gdb.base/reread.exp: second pass: breakpoint foo in first file
+FAIL: gdb.base/reread.exp: second pass: breakpoint foo in first file
-PASS: gdb.base/reread.exp: second pass: run to foo() second time
+FAIL: gdb.base/reread.exp: second pass: run to foo() second time (timeout)

This happens on all tested platforms like Fedora 19 or also on CentOS-6 etc.
Everything for x86_64/x86_64-m32/i686.  I do not have it tested for CentOS-5.

Occasional gdb.ada/exec_changed.exp and gdb.base/chng-syms.exp FAILs are also
related I guess but we will see after a fix.


Jan

Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Doug Evans]

On Thu, Jul 18, 2013 at 3:54 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 16 Jul 2013 00:42:33 +0200, Doug Evans wrote:
>> 2013-07-15  Doug Evans  <dje@google.com>
>>
>>       * target.h (struct target_section): Delete member bfd.
>>       All users updated to use the_bfd_section->owner instead.
>>       * exec.c (add_to_section_table): Assert bfd is expected value.
>>       Remove initialization of target_section.bfd.
>>       (remove_target_sections): Update.
>>       (section_table_available_memory): Update.
>>       (section_table_xfer_memory_partial): Update.
>>       (print_section_info): Update.
>>       (exec_set_section_address): Update.
>>       * record-full.c (record_full_core_xfer_partial): Update.
>>       * solib-svr4.c (svr4_relocate_section_addresses): Update.
>>       * solib-target.c (solib_target_relocate_section_addresses): Update.
>>       * symfile.c (build_section_addr_info_from_section_table): Update.
>>       * target.c (memory_xfer_live_readonly_partial): Update.
>>       (memory_xfer_partial_1): Update.
>
>  Running gdb/testsuite/gdb.base/reread.exp ...
> -PASS: gdb.base/reread.exp: second pass: breakpoint foo in first file
> +FAIL: gdb.base/reread.exp: second pass: breakpoint foo in first file
> -PASS: gdb.base/reread.exp: second pass: run to foo() second time
> +FAIL: gdb.base/reread.exp: second pass: run to foo() second time (timeout)
>
> This happens on all tested platforms like Fedora 19 or also on CentOS-6 etc.
> Everything for x86_64/x86_64-m32/i686.  I do not have it tested for CentOS-5.
>
> Occasional gdb.ada/exec_changed.exp and gdb.base/chng-syms.exp FAILs are also
> related I guess but we will see after a fix.

Blech, I can't recreate this on an ubuntu or fc17 system.

Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Jan Kratochvil]

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


Jan

Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Doug Evans]

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  <dje@google.com>

	* 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[.  */

Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Doug Evans]

I wrote:
> 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  <dje@google.com>
> 
> 	* 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.

Aaahhh.
This time with the right ChangeLog entry.

2013-07-18  Doug Evans  <dje@google.com>

	* exec.h (remove_target_sections): Delete arg abfd.
	* exec.c (exec_close): Update call to remove_target_sections.
	(remove_target_sections): Delete arg abfd.
	* solib.c (update_solib_list): Ditto.
	(reload_shared_libraries_1): Ditto.
	(clear_solib): Ditto, and unconditionally call remove_target_sections.
	* target.h (struct target_section): Rename key to owner.
	All uses updated.

Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> The change was added here:
Doug> http://sourceware.org/ml/gdb-patches/2012-07/msg00742.html
Doug> Tom: Can you remember the reproducer for the issue fixed by this patch?

This is all I have:

    http://sourceware.org/ml/gdb-patches/2011-12/msg00493.html

Basically, I tripped across a case where gdb assumed that BFDs were not
shared.

Doug> Note that all calls to remove_target_sections pass the bfd from "key":

I didn't understand this, but reading the patch made it all clear.
It looks good to me, thanks.

Tom

[commit] Re: Runtime regression for gdb.base/reread.exp & co. [Re: [RFA] Remove target_section.bfd]

[Jan Kratochvil]

On Mon, 22 Jul 2013 18:24:55 +0200, Tom Tromey wrote:
> It looks good to me, thanks.

I find the patch also OK and to make all the regressions maze easier I have
therefore checked it in:
	http://sourceware.org/ml/gdb-cvs/2013-07/msg00101.html
	http://sourceware.org/ml/gdb-cvs/2013-07/msg00102.html


Thanks,
Jan