[PATCH/RFA] Don't gdbarch_init for core files

[PATCH/RFA] Don't gdbarch_init for core files

[Jason R Thorpe]

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

As discussed on gdb@, there is a problem involving gdbarch and core
files.

At least GNU/Linux and NetBSD identify executables using note sections.
Many targets use this to select the OS/ABI variant for the target.

The problem is that if you are debugging a core file, the core file is
loaded after the executable, and the current code re-initializes the
current gdbarch based on the core file.

Since the core file lacks the same markings as the executable, the
gdbarch that results is unable to debug the executable+core.

There are other problems, as well.  The core file often doesn't have
the same flags as an executable -- consider the flags the MIPS target
uses to decide between o32, o64, n32, etc.  These flags may not be
present in the core file (indeed -- the core file is just a memory image,
and doens't really have an "ABI", per se).  The ABI, again, really comes
from the executable.

At the very least, it has befuddled Daniel and me :-)

The following patch fixes this problem (which can be easily demonstrated
by simply doing "gdb a.out a.out.core" on any target that supports OS/ABI
variants).

	* corelow.c (core_open): Don't reinitialize the current
	gdbarch.

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

[-- Attachment #2: core-patch --]
[-- Type: text/plain, Size: 506 bytes --]

Index: corelow.c
===================================================================
RCS file: /cvs/src/src/gdb/corelow.c,v
retrieving revision 1.21
diff -u -r1.21 corelow.c
--- corelow.c	5 May 2002 01:15:13 -0000	1.21
+++ corelow.c	10 May 2002 01:40:13 -0000
@@ -321,8 +321,6 @@
     error ("\"%s\": Can't find sections: %s",
 	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
 
-  set_gdbarch_from_file (core_bfd);
-
   ontop = !push_target (&core_ops);
   discard_cleanups (old_chain);
 

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Andrew Cagney]

> As discussed on gdb@, there is a problem involving gdbarch and core
> files.
> 
> At least GNU/Linux and NetBSD identify executables using note sections.
> Many targets use this to select the OS/ABI variant for the target.
> 
> The problem is that if you are debugging a core file, the core file is
> loaded after the executable, and the current code re-initializes the
> current gdbarch based on the core file.
> 
> Since the core file lacks the same markings as the executable, the
> gdbarch that results is unable to debug the executable+core.
> 
> There are other problems, as well.  The core file often doesn't have
> the same flags as an executable -- consider the flags the MIPS target
> uses to decide between o32, o64, n32, etc.  These flags may not be
> present in the core file (indeed -- the core file is just a memory image,
> and doens't really have an "ABI", per se).  The ABI, again, really comes
> from the executable.
> 
> At the very least, it has befuddled Daniel and me :-)
> 
> The following patch fixes this problem (which can be easily demonstrated
> by simply doing "gdb a.out a.out.core" on any target that supports OS/ABI
> variants).
> 
> 	* corelow.c (core_open): Don't reinitialize the current
> 	gdbarch.

Given this original patch:

   http://sources.redhat.com/ml/gdb-patches/2000-04/msg00483.html

is the exact reverse, something else must be needed.

Instead of removing the call, is it possible to reasonably detect if the 
core file belongs to the current architecture (either in corefile.c) or 
in gdbarch_FOO_init().

Andrew

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Jason R Thorpe]

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

On Thu, May 09, 2002 at 11:04:27PM -0400, Andrew Cagney wrote:

 > Instead of removing the call, is it possible to reasonably detect if the 
 > core file belongs to the current architecture (either in corefile.c) or 
 > in gdbarch_FOO_init().

How about this?

	* corelow.c (core_open): If the core's arch is known and
	different from the current, reinitialize gdbarch based on
	the core file.

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

[-- Attachment #2: core-patch --]
[-- Type: text/plain, Size: 1021 bytes --]

Index: corelow.c
===================================================================
RCS file: /cvs/src/src/gdb/corelow.c,v
retrieving revision 1.21
diff -u -r1.21 corelow.c
--- corelow.c	5 May 2002 01:15:13 -0000	1.21
+++ corelow.c	10 May 2002 04:14:05 -0000
@@ -264,6 +264,7 @@
   bfd *temp_bfd;
   int ontop;
   int scratch_chan;
+  const struct bfd_arch_info *current_arch_info;
 
   target_preopen (from_tty);
   if (!filename)
@@ -321,7 +322,13 @@
     error ("\"%s\": Can't find sections: %s",
 	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
 
-  set_gdbarch_from_file (core_bfd);
+  /* If the core's arch is known and different from the current,
+     reinitialize gdbarch based on the core file.  */
+
+  current_arch_info = gdbarch_bfd_arch_info (current_gdbarch);
+  if (core_bfd->arch_info->arch != bfd_arch_unknown
+      && core_bfd->arch_info->arch != current_arch_info->arch)
+    set_gdbarch_from_file (core_bfd);
 
   ontop = !push_target (&core_ops);
   discard_cleanups (old_chain);

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Andrew Cagney]

> How about this?
> 
> 	* corelow.c (core_open): If the core's arch is known and
> 	different from the current, reinitialize gdbarch based on
> 	the core file.
> 

Hmm, thing is:

> -  set_gdbarch_from_file (core_bfd);
> +  /* If the core's arch is known and different from the current,
> +     reinitialize gdbarch based on the core file.  */
> +
> +  current_arch_info = gdbarch_bfd_arch_info (current_gdbarch);
> +  if (core_bfd->arch_info->arch != bfd_arch_unknown
> +      && core_bfd->arch_info->arch != current_arch_info->arch)
> +    set_gdbarch_from_file (core_bfd);

is [almost] no different to deleting the call - GDB isn't yet built with 
multiple architectures so the two architectures will always be identical.

Looking at the date/author of the original patch [and making a wild 
guess], I think the original change was related to debugging 32 bit core 
files on a SPARC64 system.  Michael?

For the moment, bfd's compatible() might be the best test (does it give 
the effect you're looking for?).  The other approach is to enhance the 
relevant architecture vectors so that they don't change the architecture 
for cases like this.  I think, eventually, the ABI/OS stuff will help 
solve this problem.  Anway, what ever the change, it will need plenty 
comments :-)

enjoy,
Andrew

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Jason R Thorpe]

On Sat, May 11, 2002 at 10:48:13PM -0400, Andrew Cagney wrote:

 > is [almost] no different to deleting the call - GDB isn't yet built with 
 > multiple architectures so the two architectures will always be identical.
 > 
 > Looking at the date/author of the original patch [and making a wild 
 > guess], I think the original change was related to debugging 32 bit core 
 > files on a SPARC64 system.  Michael?

Well, I know Solaris dumps a 32-bit core file for a 32-bit binary,
and a 64-bit core file for a 64-bit binary.

I simply fail to see any reason why you'd want to re-initialize the
gdbarch for a core file.

I guess I really do need to know why the change was added in the first
place (the message with the original patch doesn't describe the problem
the patch is trying to solve).

 > For the moment, bfd's compatible() might be the best test (does it give 
 > the effect you're looking for?).  The other approach is to enhance the 
 > relevant architecture vectors so that they don't change the architecture 
 > for cases like this.  I think, eventually, the ABI/OS stuff will help 
 > solve this problem.  Anway, what ever the change, it will need plenty 
 > comments :-)

Well, the question is -- how are the arch vectors supposed to tell
when they're supposed to update it and when they're not supposed to
update it?

Sigh, in any case, the current situation really sucks, as core file
handling is somewhat broken on any platform that has gdbarch'd OS ABI
handling.

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Daniel Jacobowitz]

On Sat, May 11, 2002 at 08:30:35PM -0700, Jason R Thorpe wrote:
> On Sat, May 11, 2002 at 10:48:13PM -0400, Andrew Cagney wrote:
> 
>  > is [almost] no different to deleting the call - GDB isn't yet built with 
>  > multiple architectures so the two architectures will always be identical.
>  > 
>  > Looking at the date/author of the original patch [and making a wild 
>  > guess], I think the original change was related to debugging 32 bit core 
>  > files on a SPARC64 system.  Michael?
> 
> Well, I know Solaris dumps a 32-bit core file for a 32-bit binary,
> and a 64-bit core file for a 64-bit binary.
> 
> I simply fail to see any reason why you'd want to re-initialize the
> gdbarch for a core file.
> 
> I guess I really do need to know why the change was added in the first
> place (the message with the original patch doesn't describe the problem
> the patch is trying to solve).

Just a guess - debugging a core file without an original binary?

> 
>  > For the moment, bfd's compatible() might be the best test (does it give 
>  > the effect you're looking for?).  The other approach is to enhance the 
>  > relevant architecture vectors so that they don't change the architecture 
>  > for cases like this.  I think, eventually, the ABI/OS stuff will help 
>  > solve this problem.  Anway, what ever the change, it will need plenty 
>  > comments :-)
> 
> Well, the question is -- how are the arch vectors supposed to tell
> when they're supposed to update it and when they're not supposed to
> update it?
> 
> Sigh, in any case, the current situation really sucks, as core file
> handling is somewhat broken on any platform that has gdbarch'd OS ABI
> handling.

<nod>

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Sat, May 11, 2002 at 08:30:35PM -0700, Jason R Thorpe wrote:
> > On Sat, May 11, 2002 at 10:48:13PM -0400, Andrew Cagney wrote:
> >
> >  > is [almost] no different to deleting the call - GDB isn't yet built with
> >  > multiple architectures so the two architectures will always be identical.
> >  >
> >  > Looking at the date/author of the original patch [and making a wild
> >  > guess], I think the original change was related to debugging 32 bit core
> >  > files on a SPARC64 system.  Michael?
> >
> > Well, I know Solaris dumps a 32-bit core file for a 32-bit binary,
> > and a 64-bit core file for a 64-bit binary.
> >
> > I simply fail to see any reason why you'd want to re-initialize the
> > gdbarch for a core file.
> >
> > I guess I really do need to know why the change was added in the first
> > place (the message with the original patch doesn't describe the problem
> > the patch is trying to solve).
> 
> Just a guess - debugging a core file without an original binary?

Bingo!   ;-)

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Michael Snyder]

Andrew Cagney wrote:
> 
> > How about this?
> >
> >       * corelow.c (core_open): If the core's arch is known and
> >       different from the current, reinitialize gdbarch based on
> >       the core file.
> >
> 
> Hmm, thing is:
> 
> > -  set_gdbarch_from_file (core_bfd);
> > +  /* If the core's arch is known and different from the current,
> > +     reinitialize gdbarch based on the core file.  */
> > +
> > +  current_arch_info = gdbarch_bfd_arch_info (current_gdbarch);
> > +  if (core_bfd->arch_info->arch != bfd_arch_unknown
> > +      && core_bfd->arch_info->arch != current_arch_info->arch)
> > +    set_gdbarch_from_file (core_bfd);
> 
> is [almost] no different to deleting the call - GDB isn't yet built with
> multiple architectures so the two architectures will always be identical.

Will they?  What if, for instance, gdb defaults to sparc32, but
the corefile is sparc64?

> Looking at the date/author of the original patch [and making a wild
> guess], I think the original change was related to debugging 32 bit core
> files on a SPARC64 system.  Michael?

Correct.  And I think the worrisome case is when there is no symbol
file.

> For the moment, bfd's compatible() might be the best test (does it give
> the effect you're looking for?).  The other approach is to enhance the
> relevant architecture vectors so that they don't change the architecture
> for cases like this.  I think, eventually, the ABI/OS stuff will help
> solve this problem.  Anway, what ever the change, it will need plenty
> comments :-)
> 
> enjoy,
> Andrew

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Andrew Cagney]

> -  set_gdbarch_from_file (core_bfd);
>> > +  /* If the core's arch is known and different from the current,
>> > +     reinitialize gdbarch based on the core file.  */
>> > +
>> > +  current_arch_info = gdbarch_bfd_arch_info (current_gdbarch);
>> > +  if (core_bfd->arch_info->arch != bfd_arch_unknown
>> > +      && core_bfd->arch_info->arch != current_arch_info->arch)
>> > +    set_gdbarch_from_file (core_bfd);
> 
>> 
>> is [almost] no different to deleting the call - GDB isn't yet built with
>> multiple architectures so the two architectures will always be identical.
> 
> 
> Will they?  What if, for instance, gdb defaults to sparc32, but
> the corefile is sparc64?

The bfd_arch_sparc would be the same in both cases but bfd_mach... would 
be different.

Any way my bfd_arch_compatible() suggestion has a bug mind:

const bfd_arch_info_type *
bfd_arch_get_compatible PARAMS ((
     const bfd *abfd,
     const bfd *bbfd));

A new interface would be needed.

Andrew

Re: [PATCH/RFA] Don't gdbarch_init for core files

[m.m.kettenis]

Michael Snyder wrote:
> > > I guess I really do need to know why the change was added in the first
> > > place (the message with the original patch doesn't describe the problem
> > > the patch is trying to solve).
> > > Just a guess - debugging a core file without an original binary?
>
> Bingo!   ;-)

Ah, but in that case, why not call set_gdbarch_from_file() only when exec_bfd is NULL?  As in:

  if (!exec_bfd)
    set_gdbarch_from_file (core_bfd);

I tested this, and it seems to solve the problems for me when testing my pending i386 multi-arch patches on Linux.

Mark

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Jason R Thorpe]

On Fri, May 17, 2002 at 03:58:49PM +0200, m.m.kettenis@chello.nl wrote:

 >   if (!exec_bfd)
 >     set_gdbarch_from_file (core_bfd);
 > 
 > I tested this, and it seems to solve the problems for me when testing
 > my pending i386 multi-arch patches on Linux.

Hm, why didn't I think of that :-)

This would be totally cool with me.

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Michael Snyder]

Jason R Thorpe wrote:
> 
> On Fri, May 17, 2002 at 03:58:49PM +0200, m.m.kettenis@chello.nl wrote:
> 
>  >   if (!exec_bfd)
>  >     set_gdbarch_from_file (core_bfd);
>  >
>  > I tested this, and it seems to solve the problems for me when testing
>  > my pending i386 multi-arch patches on Linux.
> 
> Hm, why didn't I think of that :-)
> 
> This would be totally cool with me.

Let's do it.

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Mark Kettenis]

   Date: Fri, 17 May 2002 10:45:35 -0700
   From: Michael Snyder <msnyder@redhat.com>

   Jason R Thorpe wrote:
   > 
   > On Fri, May 17, 2002 at 03:58:49PM +0200, m.m.kettenis@chello.nl wrote:
   > 
   >  >   if (!exec_bfd)
   >  >     set_gdbarch_from_file (core_bfd);
   >  >
   >  > I tested this, and it seems to solve the problems for me when testing
   >  > my pending i386 multi-arch patches on Linux.
   > 
   > Hm, why didn't I think of that :-)
   > 
   > This would be totally cool with me.

   Let's do it.

OK, I checked in the attached patch.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* corelow.c (core_open): Only call set_gdbarch_from_file if
	exec_bfd is NULL.

Index: corelow.c
===================================================================
RCS file: /cvs/src/src/gdb/corelow.c,v
retrieving revision 1.21
diff -u -p -r1.21 corelow.c
--- corelow.c 5 May 2002 01:15:13 -0000 1.21
+++ corelow.c 17 May 2002 23:04:13 -0000
@@ -321,7 +321,12 @@ core_open (char *filename, int from_tty)
     error ("\"%s\": Can't find sections: %s",
 	   bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
 
-  set_gdbarch_from_file (core_bfd);
+  /* If we have no exec file, try to set the architecture from the
+     core file.  We don't do this unconditionally since an exec file
+     typically contains more information that helps us determine the
+     architecture than a core file.  */
+  if (!exec_bfd)
+    set_gdbarch_from_file (core_bfd);
 
   ontop = !push_target (&core_ops);
   discard_cleanups (old_chain);

Re: [PATCH/RFA] Don't gdbarch_init for core files

[Michael Snyder]

m.m.kettenis@chello.nl wrote:
> 
> Michael Snyder wrote:
> > > > I guess I really do need to know why the change was added in the first
> > > > place (the message with the original patch doesn't describe the problem
> > > > the patch is trying to solve).
> > > > Just a guess - debugging a core file without an original binary?
> >
> > Bingo!   ;-)
> 
> Ah, but in that case, why not call set_gdbarch_from_file() only when exec_bfd is NULL?  As in:
> 
>   if (!exec_bfd)
>     set_gdbarch_from_file (core_bfd);
> 
> I tested this, and it seems to solve the problems for me when testing my pending i386 multi-arch patches on Linux.

I cannot immediately think of any objections to this.
Michael