[patch] Fix assert crashes with minidebuginfo

[patch] Fix assert crashes with minidebuginfo

[Jan Kratochvil]

Hi Tom,

original bugreport:
	https://bugzilla.redhat.com/show_bug.cgi?id=903522

in some cases GDB added separate debug info to minidebuginfo (which itself is
a separate debug info).  separate debug info of a separate debug info is not
supported by GDB and it caused a gdb_assert failure.

So the added gdb_assert calls print such violation immediately at
add_separate_debug_objfile, not only in a some rare case found by the
bugreport.

And I have found multiple such fragile checks in GDB so I have protected them
all so that minidebuginfo is always only a sole separate debug info.

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

With the new gdb_asserts (and without the fix) GDB crashes on gdb.gdb/ tests
due to current ncurses-libs-5.9-7.20121017.fc19.x86_64 files.


Thanks,
Jan


gdb/
2013-02-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* elfread.c (elf_symfile_read): Limit separate debug info additions to
	files with no separate debug info.
	* objfiles.c (add_separate_debug_objfile): Add gdb_assert calls.
	* symfile.c (read_symbols): Call find_separate_debug_file_in_section
	only for files with no separate debug info.

gdb/testsuite/
2013-02-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/gnu-debugdata.exp): Create ${binfile}.debug,
	${binfile}.mini_debuginfo-debuglink, add -k to xz, use now
	${binfile}.mini_debuginfo-debuglink and
	${binfile}.mini_debuginfo-debuglink.xz.

diff --git a/gdb/elfread.c b/gdb/elfread.c
index 9d630cd..6ca659f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1431,8 +1431,18 @@ elf_symfile_read (struct objfile *objfile, int symfile_flags)
   /* If the file has its own symbol tables it has no separate debug
      info.  `.dynsym'/`.symtab' go to MSYMBOLS, `.debug_info' goes to
      SYMTABS/PSYMTABS.  `.gnu_debuglink' may no longer be present with
-     `.note.gnu.build-id'.  */
-  else if (!objfile_has_partial_symbols (objfile))
+     `.note.gnu.build-id'.
+
+     .gnu_debugdata is !objfile_has_partial_symbols because it contains only
+     .symtab, not .debug_* section.  But if we already added .gnu_debugdata as
+     an objfile via find_separate_debug_file_in_section there was no separate
+     debug info available.  Therefore do not attempt to search for another one,
+     objfile->separate_debug_objfile->separate_debug_objfile GDB guarantees to
+     be NULL and we would possibly violate it.  */
+
+  else if (!objfile_has_partial_symbols (objfile)
+	   && objfile->separate_debug_objfile == NULL
+	   && objfile->separate_debug_objfile_backlink == NULL)
     {
       char *debugfile;
 
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 5232c8f..5829699 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -476,6 +476,9 @@ add_separate_debug_objfile (struct objfile *objfile, struct objfile *parent)
   /* Must not be already in a list.  */
   gdb_assert (objfile->separate_debug_objfile_backlink == NULL);
   gdb_assert (objfile->separate_debug_objfile_link == NULL);
+  gdb_assert (objfile->separate_debug_objfile == NULL);
+  gdb_assert (parent->separate_debug_objfile_backlink == NULL);
+  gdb_assert (parent->separate_debug_objfile_link == NULL);
 
   objfile->separate_debug_objfile_backlink = parent;
   objfile->separate_debug_objfile_link = parent->separate_debug_objfile;
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 63bf329..6f968b7 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -823,7 +823,12 @@ static void
 read_symbols (struct objfile *objfile, int add_flags)
 {
   (*objfile->sf->sym_read) (objfile, add_flags);
-  if (!objfile_has_partial_symbols (objfile))
+
+  /* find_separate_debug_file_in_section should be called only if there is
+     single binary with no existing separate debug info file.  */
+  if (!objfile_has_partial_symbols (objfile)
+      && objfile->separate_debug_objfile == NULL
+      && objfile->separate_debug_objfile_backlink == NULL)
     {
       bfd *abfd = find_separate_debug_file_in_section (objfile);
       struct cleanup *cleanup = make_cleanup_bfd_unref (abfd);
diff --git a/gdb/testsuite/gdb.base/gnu-debugdata.exp b/gdb/testsuite/gdb.base/gnu-debugdata.exp
index f34e4e8..55aa3c6 100644
--- a/gdb/testsuite/gdb.base/gnu-debugdata.exp
+++ b/gdb/testsuite/gdb.base/gnu-debugdata.exp
@@ -127,14 +127,30 @@ if {[run "strip" [transform strip] \
     return -1
 }
 
+# Separate full debug info into ${binfile}.debug.
+remote_file host delete ${binfile}.debug
+if {[run "copydebug" [transform objcopy] \
+	 "--only-keep-debug ${binfile} ${binfile}.debug"]} {
+    return -1
+}
+
+# Add the .gnu_debuglink section to the .gnu_debugdata file.
+# .gnu_debuglink is normally not present in the .gnu_debugdata section but in
+# some files there may be PT_NOTE with NT_GNU_BUILD_ID and GDB could look up
+# the .debug file from it.
+if {[run "addlink" [transform objcopy] \
+	 "--add-gnu-debuglink=${binfile}.debug ${binfile}.mini_debuginfo ${binfile}.mini_debuginfo-debuglink"]} {
+    return -1
+}
+
 # Inject the compressed data into the .gnu_debugdata section of the
 # original binary.
-remote_file host delete ${binfile}.mini_debuginfo.xz
-if {[run "xz" "xz" "${binfile}.mini_debuginfo"]} {
+remote_file host delete ${binfile}.mini_debuginfo-debuglink.xz
+if {[run "xz" "xz" "-k ${binfile}.mini_debuginfo-debuglink"]} {
     return -1
 }
 remote_file host delete ${binfile}.test
-if {[run "objcopy 2" [transform objcopy] "--add-section .gnu_debugdata=${binfile}.mini_debuginfo.xz ${binfile}.strip ${binfile}.test"]} {
+if {[run "objcopy 2" [transform objcopy] "--add-section .gnu_debugdata=${binfile}.mini_debuginfo-debuglink.xz ${binfile}.strip ${binfile}.test"]} {
     return -1
 }
 

Re: [patch] Fix assert crashes with minidebuginfo

[Jan Kratochvil]

On Fri, 01 Feb 2013 19:21:34 +0100, Jan Kratochvil wrote:
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 63bf329..6f968b7 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -823,7 +823,12 @@ static void
>  read_symbols (struct objfile *objfile, int add_flags)
>  {
>    (*objfile->sf->sym_read) (objfile, add_flags);
> -  if (!objfile_has_partial_symbols (objfile))
> +
> +  /* find_separate_debug_file_in_section should be called only if there is
> +     single binary with no existing separate debug info file.  */
> +  if (!objfile_has_partial_symbols (objfile)
> +      && objfile->separate_debug_objfile == NULL
> +      && objfile->separate_debug_objfile_backlink == NULL)
>      {
>        bfd *abfd = find_separate_debug_file_in_section (objfile);
>        struct cleanup *cleanup = make_cleanup_bfd_unref (abfd);

As discussed with Tom off-list:

first entry:
 * SYM_READ reads separate .debug file.

second entry, called for the .debug file:
 * nothing interesting as objfile_has_partial_symbols (objfile).

first entry continues:
 * But the primary file still !objfile_has_partial_symbols (objfile),
   therefore minidebuginfo is read as a secondary debug info file
   (those are supported for MacOSX).

third entry, called for the minidebuginfo
 * SYM_READ starts to search for separate debug info (via its PT_NOTE
   build-id) file for the minidebuginfo file.
=> crash


Regards,
Jan

Re: [patch] Fix assert crashes with minidebuginfo

[Jan Kratochvil]

On Fri, 01 Feb 2013 20:13:23 +0100, Tom Tromey wrote:
> I wonder if this works.

Yes, I had such initial patch, it worked.

Just in this patch - or it can be another patch - I believe it should be more
foolproof against other possibly broken debug infos.


Jan

Re: [patch] Fix assert crashes with minidebuginfo

[Tom Tromey]

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

Jan> first entry continues:
Jan>  * But the primary file still !objfile_has_partial_symbols (objfile),
Jan>    therefore minidebuginfo is read as a secondary debug info file
Jan>    (those are supported for MacOSX).

It seems to me that symfile.c:read_symbols should check for a separate
debug file.  IIUC the intent of the code originally was to skip
minidebuginfo entirely if the separate debuginfo was available -- since
the separate debuginfo is always more complete anyhow.

I wonder if this works.

diff --git a/gdb/symfile.c b/gdb/symfile.c
index 63bf329..0823f16 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -823,7 +823,8 @@ static void
 read_symbols (struct objfile *objfile, int add_flags)
 {
   (*objfile->sf->sym_read) (objfile, add_flags);
-  if (!objfile_has_partial_symbols (objfile))
+  if (!objfile_has_partial_symbols (objfile)
+      && objfile->separate_debug_objfile == NULL)
     {
       bfd *abfd = find_separate_debug_file_in_section (objfile);
       struct cleanup *cleanup = make_cleanup_bfd_unref (abfd);


Tom

[commit] [patch] Fix assert crashes with minidebuginfo

[Jan Kratochvil]

On Fri, 01 Feb 2013 20:21:12 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> 2013-02-01  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> Jan> 	* elfread.c (elf_symfile_read): Limit separate debug info additions to
> Jan> 	files with no separate debug info.
> Jan> 	* objfiles.c (add_separate_debug_objfile): Add gdb_assert calls.
> Jan> 	* symfile.c (read_symbols): Call find_separate_debug_file_in_section
> Jan> 	only for files with no separate debug info.
> 
> Ok, I understand now.
> I think this patch is fine.  Please put it in.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-02/msg00004.html


Thanks,
Jan

Re: [patch] Fix assert crashes with minidebuginfo

[Tom Tromey]

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

Jan> 2013-02-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

Jan> 	* elfread.c (elf_symfile_read): Limit separate debug info additions to
Jan> 	files with no separate debug info.
Jan> 	* objfiles.c (add_separate_debug_objfile): Add gdb_assert calls.
Jan> 	* symfile.c (read_symbols): Call find_separate_debug_file_in_section
Jan> 	only for files with no separate debug info.

Ok, I understand now.
I think this patch is fine.  Please put it in.

Tom