[patch] build_section_addr_info* SEC_ALLOC/SEC_LOAD unification

[patch] build_section_addr_info* SEC_ALLOC/SEC_LOAD unification

[Jan Kratochvil]

Hi Tristan,

http://sourceware.org/ml/gdb-patches/2010-01/msg00111.html
http://sourceware.org/ml/gdb-cvs/2010-01/msg00051.html
3bfec189bb0fa1a2a44f1645dd68a9572e7a841c
2010-01-07  Tristan Gingold  <gingold@adacore.com>

	* symfile.c (build_section_addr_info_from_objfile): New function.
	(symbol_file_add_separate): Don't use offsets from objfile but
	built an addr info.

this new function creates the address information for _all_ the sections while
former build_section_addr_info_from_section_table creates it only if section
is SEC_ALLOC or SEC_LOAD.

While I have no countercase I do not see a reason for such difference, do you?
My previous unchecked-in patch had implemented this function on top of
build_section_addr_info_from_section_table and thus conforming to this
SEC_ALLOC or SEC_LOAD conditional:
	[patch 06/15] PIE: Fix displacement of separate debug info files
	http://sourceware.org/ml/gdb-patches/2009-11/msg00173.html


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


2010-02-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* symfile.c (build_section_addr_info_from_objfile): Include sections
	only if they are SEC_ALLOC or SEC_LOAD.

--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -371,16 +371,16 @@ build_section_addr_info_from_objfile (const struct objfile *objfile)
     mask = ((CORE_ADDR) 1 << addr_bit) - 1;
 
   sap = alloc_section_addr_info (objfile->num_sections);
-  for (i = 0, sec = objfile->obfd->sections;
-       i < objfile->num_sections;
-       i++, sec = sec->next)
-    {
-      gdb_assert (sec != NULL);
-      sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
-                            + objfile->section_offsets->offsets[i]) & mask;
-      sap->other[i].name = xstrdup (bfd_get_section_name (objfile->obfd, sec));
-      sap->other[i].sectindex = sec->index;
-    }
+  for (i = 0, sec = objfile->obfd->sections; sec != NULL; sec = sec->next)
+    if (bfd_get_section_flags (objfile->obfd, sec) & (SEC_ALLOC | SEC_LOAD))
+      {
+	sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
+			      + objfile->section_offsets->offsets[i]) & mask;
+	sap->other[i].name = xstrdup (bfd_get_section_name (objfile->obfd,
+							    sec));
+	sap->other[i].sectindex = sec->index;
+	i++;
+      }
   return sap;
 }
 

Re: [patch] build_section_addr_info* SEC_ALLOC/SEC_LOAD unification

[Tristan Gingold]

On Feb 13, 2010, at 11:34 PM, Jan Kratochvil wrote:

> Hi Tristan,
> 
> http://sourceware.org/ml/gdb-patches/2010-01/msg00111.html
> http://sourceware.org/ml/gdb-cvs/2010-01/msg00051.html
> 3bfec189bb0fa1a2a44f1645dd68a9572e7a841c
> 2010-01-07  Tristan Gingold  <gingold@adacore.com>
> 
> 	* symfile.c (build_section_addr_info_from_objfile): New function.
> 	(symbol_file_add_separate): Don't use offsets from objfile but
> 	built an addr info.
> 
> this new function creates the address information for _all_ the sections while
> former build_section_addr_info_from_section_table creates it only if section
> is SEC_ALLOC or SEC_LOAD.
> 
> While I have no countercase I do not see a reason for such difference, do you?

No.  I think I simply removed this line inadvertently.

Thanks for catching this,
Tristan.

> My previous unchecked-in patch had implemented this function on top of
> build_section_addr_info_from_section_table and thus conforming to this
> SEC_ALLOC or SEC_LOAD conditional:
> 	[patch 06/15] PIE: Fix displacement of separate debug info files
> 	http://sourceware.org/ml/gdb-patches/2009-11/msg00173.html
> 
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.
> 
> 
> Thanks,
> Jan
> 
> 
> 2010-02-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* symfile.c (build_section_addr_info_from_objfile): Include sections
> 	only if they are SEC_ALLOC or SEC_LOAD.
> 
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -371,16 +371,16 @@ build_section_addr_info_from_objfile (const struct objfile *objfile)
>     mask = ((CORE_ADDR) 1 << addr_bit) - 1;
> 
>   sap = alloc_section_addr_info (objfile->num_sections);
> -  for (i = 0, sec = objfile->obfd->sections;
> -       i < objfile->num_sections;
> -       i++, sec = sec->next)
> -    {
> -      gdb_assert (sec != NULL);
> -      sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
> -                            + objfile->section_offsets->offsets[i]) & mask;
> -      sap->other[i].name = xstrdup (bfd_get_section_name (objfile->obfd, sec));
> -      sap->other[i].sectindex = sec->index;
> -    }
> +  for (i = 0, sec = objfile->obfd->sections; sec != NULL; sec = sec->next)
> +    if (bfd_get_section_flags (objfile->obfd, sec) & (SEC_ALLOC | SEC_LOAD))
> +      {
> +	sap->other[i].addr = (bfd_get_section_vma (objfile->obfd, sec)
> +			      + objfile->section_offsets->offsets[i]) & mask;
> +	sap->other[i].name = xstrdup (bfd_get_section_name (objfile->obfd,
> +							    sec));
> +	sap->other[i].sectindex = sec->index;
> +	i++;
> +      }
>   return sap;
> }
> 
> 

Re: [patch] build_section_addr_info* SEC_ALLOC/SEC_LOAD unification

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> 2010-02-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* symfile.c (build_section_addr_info_from_objfile): Include sections
Jan> 	only if they are SEC_ALLOC or SEC_LOAD.

This is ok.  Thanks.  And thanks, Tristan, for looking at it.

Tom

Re: [patch] build_section_addr_info* SEC_ALLOC/SEC_LOAD unification

[Jan Kratochvil]

On Wed, 17 Feb 2010 20:04:27 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> 2010-02-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	* symfile.c (build_section_addr_info_from_objfile): Include sections
> Jan> 	only if they are SEC_ALLOC or SEC_LOAD.
> 
> This is ok.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-02/msg00139.html


Thanks,
Jan