RFC: fix latent bug in syms_from_objfile_1

RFC: fix latent bug in syms_from_objfile_1

[Tom Tromey]

I recently upgraded to Fedora 18.  I randomly see this from gdb:

    warning: no loadable sections found in added symbol-file system-supplied DSO at 0x7ffff7ffd000

I tracked this warning down to a buglet in syms_from_objfile_1, which
references addrs->other[0] without first checking addrs->num_sections.

This patch fixes the problem.

Built and regtested on x86-64 Fedora 18.
This actually improves the test results for me by removing a number of
bogus FAILs caused when the warning is emitted.

Any comments?

Tom

2013-03-20  Tom Tromey  <tromey@redhat.com>

	* symfile.c (syms_from_objfile_1): Check num_sections before
	calling addr_info_make_relative.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.367
diff -u -r1.367 symfile.c
--- symfile.c	14 Mar 2013 20:26:19 -0000	1.367
+++ symfile.c	20 Mar 2013 18:19:42 -0000
@@ -997,7 +997,7 @@
 
      We no longer warn if the lowest section is not a text segment (as
      happens for the PA64 port.  */
-  if (addrs && addrs->other[0].name)
+  if (addrs && addrs->num_sections > 0 && addrs->other[0].name)
     addr_info_make_relative (addrs, objfile->obfd);
 
   /* Initialize symbol reading routines for this objfile, allow complaints to

Re: RFC: fix latent bug in syms_from_objfile_1

[Jan Kratochvil]

On Wed, 20 Mar 2013 19:22:49 +0100, Tom Tromey wrote:
> --- symfile.c	14 Mar 2013 20:26:19 -0000	1.367
> +++ symfile.c	20 Mar 2013 18:19:42 -0000
> @@ -997,7 +997,7 @@
>  
>       We no longer warn if the lowest section is not a text segment (as
>       happens for the PA64 port.  */
> -  if (addrs && addrs->other[0].name)
> +  if (addrs && addrs->num_sections > 0 && addrs->other[0].name)
>      addr_info_make_relative (addrs, objfile->obfd);
>  
>    /* Initialize symbol reading routines for this objfile, allow complaints to

I find it just fixes a regression introduced by:
	commit 2679ab4be931bedfc7987d215bc336b912364906
	Author: Elena Zannoni <ezannoni@kwikemart.cygnus.com>
	Date:   Fri Jun 27 13:11:17 2003 +0000

And it also matches how addr_info_make_relative and other code handles ADDRS.

The problem is that alloc_section_addr_info sets NUM_SECTIONS while they are
not yet filled in which leads to those ugly "&& ->other[x].name" checks...


Thanks,
Jan

Re: RFC: fix latent bug in syms_from_objfile_1

[Tom Tromey]

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

Jan> The problem is that alloc_section_addr_info sets NUM_SECTIONS while
Jan> they are not yet filled in which leads to those ugly "&&
Jan> ->other[x].name" checks...

Thanks for the review and the hint.

What do you think of this?
It passes regtesting and removes all the name checks that I could find.

Tom

2013-03-20  Tom Tromey  <tromey@redhat.com>

	* symfile.c (alloc_section_addr_info): Update header.  Handle case
	where NUM_SECTIONS is 0.  Don't set 'num_sections' field.
	(build_section_addr_info_from_section_table): Set 'num_sections'.
	(build_section_addr_info_from_bfd): Likewise.
	(build_section_addr_info_from_objfile): Remove dead loop
	condition.
	(free_section_addr_info): Unconditionally call xfree.
	(relative_addr_info_to_section_offsets, addrs_section_sort)
	(addr_info_make_relative, syms_from_objfile_1): Remove dead loop
	condition.
	(syms_from_objfile_1): Remove dead 'if' condition.  Check
	'num_sections'.
	(add_symbol_file_command): Set 'num_sections'.
	* symfile-mem.c (symbol_file_add_from_memory): Set
	'num_sections'.
	* somread.c (som_symfile_offsets): Remove dead loop condition.
	* machoread.c (macho_symfile_offsets): Remove dead 'if'.
	* jit.c (jit_bfd_try_read_symtab): Set 'num_sections'.

diff --git a/gdb/jit.c b/gdb/jit.c
index ecf7317..1ea8e85 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -923,6 +923,7 @@ JITed symbol file is not an object file, ignoring it.\n"));
         sai->other[i].sectindex = sec->index;
         ++i;
       }
+  sai->num_sections = i;
 
   /* This call does not take ownership of SAI.  */
   make_cleanup_bfd_unref (nbfd);
diff --git a/gdb/machoread.c b/gdb/machoread.c
index fe77a2d..eff8bdf 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -991,9 +991,6 @@ macho_symfile_offsets (struct objfile *objfile,
 
   for (i = 0; i < addrs->num_sections; i++)
     {
-      if (addrs->other[i].name == NULL)
-	continue;
-
       ALL_OBJFILE_OSECTIONS (objfile, osect)
 	{
 	  const char *bfd_sect_name = osect->the_bfd_section->name;
diff --git a/gdb/somread.c b/gdb/somread.c
index d9d3e7b..0aa3dce 100644
--- a/gdb/somread.c
+++ b/gdb/somread.c
@@ -394,7 +394,7 @@ som_symfile_offsets (struct objfile *objfile, struct section_addr_info *addrs)
       /* Note: Here is OK to compare with ".text" because this is the
          name that gdb itself gives to that section, not the SOM
          name.  */
-      for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+      for (i = 0; i < addrs->num_sections; i++)
 	if (strcmp (addrs->other[i].name, ".text") == 0)
 	  break;
       text_addr = addrs->other[i].addr;
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index bb9bbd8..e148d09 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -127,6 +127,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name,
 	sai->other[i].sectindex = sec->index;
 	++i;
       }
+  sai->num_sections = i;
 
   objf = symbol_file_add_from_bfd (nbfd, from_tty ? SYMFILE_VERBOSE : 0,
                                    sai, OBJF_SHARED, NULL);
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2abe3f8..2b9b817 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -197,7 +197,9 @@ find_lowest_section (bfd *abfd, asection *sect, void *obj)
     *lowest = sect;
 }
 
-/* Create a new section_addr_info, with room for NUM_SECTIONS.  */
+/* Create a new section_addr_info, with room for NUM_SECTIONS.  The
+   new object's 'num_sections' field is set to 0; it must be updated
+   by the caller.  */
 
 struct section_addr_info *
 alloc_section_addr_info (size_t num_sections)
@@ -205,11 +207,14 @@ alloc_section_addr_info (size_t num_sections)
   struct section_addr_info *sap;
   size_t size;
 
+  /* Make sure the size calculation turns out ok.  */
+  if (num_sections == 0)
+    ++num_sections;
+
   size = (sizeof (struct section_addr_info)
 	  +  sizeof (struct other_sections) * (num_sections - 1));
   sap = (struct section_addr_info *) xmalloc (size);
   memset (sap, 0, size);
-  sap->num_sections = num_sections;
 
   return sap;
 }
@@ -241,6 +246,8 @@ build_section_addr_info_from_section_table (const struct target_section *start,
 	}
     }
 
+  sap->num_sections = oidx;
+
   return sap;
 }
 
@@ -262,6 +269,9 @@ build_section_addr_info_from_bfd (bfd *abfd)
 	sap->other[i].sectindex = sec->index;
 	i++;
       }
+
+  sap->num_sections = i;
+
   return sap;
 }
 
@@ -277,7 +287,7 @@ build_section_addr_info_from_objfile (const struct objfile *objfile)
      gdb_assert (objfile->num_sections == bfd_count_sections (objfile->obfd));
      */
   sap = build_section_addr_info_from_bfd (objfile->obfd);
-  for (i = 0; i < sap->num_sections && sap->other[i].name; i++)
+  for (i = 0; i < sap->num_sections; i++)
     {
       int sectindex = sap->other[i].sectindex;
 
@@ -294,8 +304,7 @@ free_section_addr_info (struct section_addr_info *sap)
   int idx;
 
   for (idx = 0; idx < sap->num_sections; idx++)
-    if (sap->other[idx].name)
-      xfree (sap->other[idx].name);
+    xfree (sap->other[idx].name);
   xfree (sap);
 }
 
@@ -446,7 +455,7 @@ relative_addr_info_to_section_offsets (struct section_offsets *section_offsets,
   memset (section_offsets, 0, SIZEOF_N_SECTION_OFFSETS (num_sections));
 
   /* Now calculate offsets for section that were specified by the caller.  */
-  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+  for (i = 0; i < addrs->num_sections; i++)
     {
       struct other_sections *osp;
 
@@ -506,7 +515,7 @@ addrs_section_sort (struct section_addr_info *addrs)
 
   /* `+ 1' for the NULL terminator.  */
   array = xmalloc (sizeof (*array) * (addrs->num_sections + 1));
-  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+  for (i = 0; i < addrs->num_sections; i++)
     array[i] = &addrs->other[i];
   array[i] = NULL;
 
@@ -605,7 +614,7 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
      (the loadable section directly below it in memory).
      this_offset = lower_offset = lower_addr - lower_orig_addr */
 
-  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+  for (i = 0; i < addrs->num_sections; i++)
     {
       struct other_sections *sect = addrs_to_abfd_addrs[i];
 
@@ -997,7 +1006,7 @@ syms_from_objfile_1 (struct objfile *objfile,
 
      We no longer warn if the lowest section is not a text segment (as
      happens for the PA64 port.  */
-  if (addrs && addrs->other[0].name)
+  if (addrs && addrs->num_sections > 0)
     addr_info_make_relative (addrs, objfile->obfd);
 
   /* Initialize symbol reading routines for this objfile, allow complaints to
@@ -2341,6 +2350,7 @@ add_symbol_file_command (char *args, int from_tty)
 	 At this point, we don't know what file type this is,
 	 so we can't determine what section names are valid.  */
     }
+  section_addrs->num_sections = sec_num;
 
   if (from_tty && (!query ("%s", "")))
     error (_("Not confirmed."));

Re: RFC: fix latent bug in syms_from_objfile_1

[Jan Kratochvil]

On Wed, 20 Mar 2013 20:50:10 +0100, Tom Tromey wrote:
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
[...]
> @@ -205,11 +207,14 @@ alloc_section_addr_info (size_t num_sections)
>    struct section_addr_info *sap;
>    size_t size;
>  
> +  /* Make sure the size calculation turns out ok.  */
> +  if (num_sections == 0)
> +    ++num_sections;

I always thought such sizeof calculation works even with # of elements == 0.
Why not?


> +
>    size = (sizeof (struct section_addr_info)
>  	  +  sizeof (struct other_sections) * (num_sections - 1));
>    sap = (struct section_addr_info *) xmalloc (size);
>    memset (sap, 0, size);
> -  sap->num_sections = num_sections;
>  
>    return sap;
>  }


I am OK with the patch, thanks for the cleanup waiting for so many years.


Jan

Re: RFC: fix latent bug in syms_from_objfile_1

[Tom Tromey]

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

Jan> On Wed, 20 Mar 2013 20:50:10 +0100, Tom Tromey wrote:
>> --- a/gdb/symfile.c
>> +++ b/gdb/symfile.c
Jan> [...]
>> @@ -205,11 +207,14 @@ alloc_section_addr_info (size_t num_sections)
>> struct section_addr_info *sap;
>> size_t size;
>> 
>> +  /* Make sure the size calculation turns out ok.  */
>> +  if (num_sections == 0)
>> +    ++num_sections;

Jan> I always thought such sizeof calculation works even with # of
Jan> elements == 0.  Why not?

It seemed weird to me since it would allocate an object smaller than a
struct section_addr_info.

I don't mind dropping that line though.

Jan> I am OK with the patch, thanks for the cleanup waiting for so many years.

Thanks.  I'm going to put it in today, since it is actively messing up
my regression tests.

Tom

Re: RFC: fix latent bug in syms_from_objfile_1

[Jan Kratochvil]

On Thu, 21 Mar 2013 16:30:48 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> I always thought such sizeof calculation works even with # of
> Jan> elements == 0.  Why not?
> 
> It seemed weird to me since it would allocate an object smaller than a
> struct section_addr_info.
> 
> I don't mind dropping that line though.

I find it allocates excessive element otherwise.

There should be "struct other_sections other[0];", that [1] is there only as
a workaround for non-GCC compilers.

Anyway not worth of more talk.


Thanks,
Jan

Re: RFC: fix latent bug in syms_from_objfile_1

[Tom Tromey]

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

Jan> I find it allocates excessive element otherwise.

Here's what I'm checking in.

Tom

2013-03-20  Tom Tromey  <tromey@redhat.com>

	* symfile.c (alloc_section_addr_info): Update header.  Don't set
	'num_sections' field.
	(build_section_addr_info_from_section_table): Set 'num_sections'.
	(build_section_addr_info_from_bfd): Likewise.
	(build_section_addr_info_from_objfile): Remove dead loop
	condition.
	(free_section_addr_info): Unconditionally call xfree.
	(relative_addr_info_to_section_offsets, addrs_section_sort)
	(addr_info_make_relative, syms_from_objfile_1): Remove dead loop
	condition.
	(syms_from_objfile_1): Remove dead 'if' condition.  Check
	'num_sections'.
	(add_symbol_file_command): Set 'num_sections'.
	* symfile-mem.c (symbol_file_add_from_memory): Set
	'num_sections'.
	* somread.c (som_symfile_offsets): Remove dead loop condition.
	* machoread.c (macho_symfile_offsets): Remove dead 'if'.
	* jit.c (jit_bfd_try_read_symtab): Set 'num_sections'.

diff --git a/gdb/jit.c b/gdb/jit.c
index ecf7317..1ea8e85 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -923,6 +923,7 @@ JITed symbol file is not an object file, ignoring it.\n"));
         sai->other[i].sectindex = sec->index;
         ++i;
       }
+  sai->num_sections = i;
 
   /* This call does not take ownership of SAI.  */
   make_cleanup_bfd_unref (nbfd);
diff --git a/gdb/machoread.c b/gdb/machoread.c
index fe77a2d..eff8bdf 100644
--- a/gdb/machoread.c
+++ b/gdb/machoread.c
@@ -991,9 +991,6 @@ macho_symfile_offsets (struct objfile *objfile,
 
   for (i = 0; i < addrs->num_sections; i++)
     {
-      if (addrs->other[i].name == NULL)
-	continue;
-
       ALL_OBJFILE_OSECTIONS (objfile, osect)
 	{
 	  const char *bfd_sect_name = osect->the_bfd_section->name;
diff --git a/gdb/somread.c b/gdb/somread.c
index d9d3e7b..0aa3dce 100644
--- a/gdb/somread.c
+++ b/gdb/somread.c
@@ -394,7 +394,7 @@ som_symfile_offsets (struct objfile *objfile, struct section_addr_info *addrs)
       /* Note: Here is OK to compare with ".text" because this is the
          name that gdb itself gives to that section, not the SOM
          name.  */
-      for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+      for (i = 0; i < addrs->num_sections; i++)
 	if (strcmp (addrs->other[i].name, ".text") == 0)
 	  break;
       text_addr = addrs->other[i].addr;
diff --git a/gdb/symfile-mem.c b/gdb/symfile-mem.c
index bb9bbd8..e148d09 100644
--- a/gdb/symfile-mem.c
+++ b/gdb/symfile-mem.c
@@ -127,6 +127,7 @@ symbol_file_add_from_memory (struct bfd *templ, CORE_ADDR addr, char *name,
 	sai->other[i].sectindex = sec->index;
 	++i;
       }
+  sai->num_sections = i;
 
   objf = symbol_file_add_from_bfd (nbfd, from_tty ? SYMFILE_VERBOSE : 0,
                                    sai, OBJF_SHARED, NULL);
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2abe3f8..d8fabb1 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -197,7 +197,9 @@ find_lowest_section (bfd *abfd, asection *sect, void *obj)
     *lowest = sect;
 }
 
-/* Create a new section_addr_info, with room for NUM_SECTIONS.  */
+/* Create a new section_addr_info, with room for NUM_SECTIONS.  The
+   new object's 'num_sections' field is set to 0; it must be updated
+   by the caller.  */
 
 struct section_addr_info *
 alloc_section_addr_info (size_t num_sections)
@@ -209,7 +211,6 @@ alloc_section_addr_info (size_t num_sections)
 	  +  sizeof (struct other_sections) * (num_sections - 1));
   sap = (struct section_addr_info *) xmalloc (size);
   memset (sap, 0, size);
-  sap->num_sections = num_sections;
 
   return sap;
 }
@@ -241,6 +242,8 @@ build_section_addr_info_from_section_table (const struct target_section *start,
 	}
     }
 
+  sap->num_sections = oidx;
+
   return sap;
 }
 
@@ -262,6 +265,9 @@ build_section_addr_info_from_bfd (bfd *abfd)
 	sap->other[i].sectindex = sec->index;
 	i++;
       }
+
+  sap->num_sections = i;
+
   return sap;
 }
 
@@ -277,7 +283,7 @@ build_section_addr_info_from_objfile (const struct objfile *objfile)
      gdb_assert (objfile->num_sections == bfd_count_sections (objfile->obfd));
      */
   sap = build_section_addr_info_from_bfd (objfile->obfd);
-  for (i = 0; i < sap->num_sections && sap->other[i].name; i++)
+  for (i = 0; i < sap->num_sections; i++)
     {
       int sectindex = sap->other[i].sectindex;
 
@@ -294,8 +300,7 @@ free_section_addr_info (struct section_addr_info *sap)
   int idx;
 
   for (idx = 0; idx < sap->num_sections; idx++)
-    if (sap->other[idx].name)
-      xfree (sap->other[idx].name);
+    xfree (sap->other[idx].name);
   xfree (sap);
 }
 
@@ -446,7 +451,7 @@ relative_addr_info_to_section_offsets (struct section_offsets *section_offsets,
   memset (section_offsets, 0, SIZEOF_N_SECTION_OFFSETS (num_sections));
 
   /* Now calculate offsets for section that were specified by the caller.  */
-  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+  for (i = 0; i < addrs->num_sections; i++)
     {
       struct other_sections *osp;
 
@@ -506,7 +511,7 @@ addrs_section_sort (struct section_addr_info *addrs)
 
   /* `+ 1' for the NULL terminator.  */
   array = xmalloc (sizeof (*array) * (addrs->num_sections + 1));
-  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+  for (i = 0; i < addrs->num_sections; i++)
     array[i] = &addrs->other[i];
   array[i] = NULL;
 
@@ -605,7 +610,7 @@ addr_info_make_relative (struct section_addr_info *addrs, bfd *abfd)
      (the loadable section directly below it in memory).
      this_offset = lower_offset = lower_addr - lower_orig_addr */
 
-  for (i = 0; i < addrs->num_sections && addrs->other[i].name; i++)
+  for (i = 0; i < addrs->num_sections; i++)
     {
       struct other_sections *sect = addrs_to_abfd_addrs[i];
 
@@ -997,7 +1002,7 @@ syms_from_objfile_1 (struct objfile *objfile,
 
      We no longer warn if the lowest section is not a text segment (as
      happens for the PA64 port.  */
-  if (addrs && addrs->other[0].name)
+  if (addrs && addrs->num_sections > 0)
     addr_info_make_relative (addrs, objfile->obfd);
 
   /* Initialize symbol reading routines for this objfile, allow complaints to
@@ -2341,6 +2346,7 @@ add_symbol_file_command (char *args, int from_tty)
 	 At this point, we don't know what file type this is,
 	 so we can't determine what section names are valid.  */
     }
+  section_addrs->num_sections = sec_num;
 
   if (from_tty && (!query ("%s", "")))
     error (_("Not confirmed."));