[RFA] Check solib bfd arch

[RFA] Check solib bfd arch

[Hui Zhu]

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

Hi,

In cross-compile remote debug environment, the gdb will load the solib
of local host if user doesn't set sysroot or something.
Load the wrong lib will make gdb get error.

So I make a patch check the solib bfd arch after load it.  I am not
check the other thing in arch_info because the mach of solib and
inferior is not same (powerpc) sometime.

This patch is tested in a i386 ubuntu and powerpc linux.

Thanks,
Hui

2009-07-03  Hui Zhu  <teawater@gmail.com>

	* solib.c (solib_bfd_open): Check solib bfd arch.

---
 solib.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/solib.c
+++ b/solib.c
@@ -311,6 +311,14 @@ solib_bfd_open (char *pathname)
 	     found_pathname, bfd_errmsg (bfd_get_error ()));
     }

+  /* Check bfd arch.  */
+  if (bfd_get_arch (abfd) != gdbarch_bfd_arch_info (target_gdbarch)->arch)
+    {
+      bfd_close (abfd);
+      make_cleanup (xfree, found_pathname);
+      error (_("`%s': ARCH not same with inferior."), found_pathname);
+    }
+
   return abfd;
 }

[-- Attachment #2: solib-check-bfd_arch_info.txt --]
[-- Type: text/plain, Size: 489 bytes --]

---
 solib.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/solib.c
+++ b/solib.c
@@ -311,6 +311,14 @@ solib_bfd_open (char *pathname)
 	     found_pathname, bfd_errmsg (bfd_get_error ()));
     }
 
+  /* Check bfd arch.  */
+  if (bfd_get_arch (abfd) != gdbarch_bfd_arch_info (target_gdbarch)->arch)
+    {
+      bfd_close (abfd);
+      make_cleanup (xfree, found_pathname);
+      error (_("`%s': ARCH not same with inferior."), found_pathname);
+    }
+
   return abfd;
 }
 

Re: [RFA] Check solib bfd arch

[Ulrich Weigand]

Hui Zhu wrote:

> +  /* Check bfd arch.  */
> +  if (bfd_get_arch (abfd) != gdbarch_bfd_arch_info (target_gdbarch)->arch)

This test seems to strict; there are cases where code of a different
architecture *can* actually run on the current architecture (e.g.
bfd_arch_rs6000 vs. bfd_arch_powerpc).

The correct check would be something alone the lines of
osabi.c:can_run_code_for.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Check solib bfd arch

[Hui Zhu]

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

On Mon, Jul 6, 2009 at 05:35, Ulrich Weigand<uweigand@de.ibm.com> wrote:
> Hui Zhu wrote:
>
>> +  /* Check bfd arch.  */
>> +  if (bfd_get_arch (abfd) != gdbarch_bfd_arch_info (target_gdbarch)->arch)
>
> This test seems to strict; there are cases where code of a different
> architecture *can* actually run on the current architecture (e.g.
> bfd_arch_rs6000 vs. bfd_arch_powerpc).
>
> The correct check would be something alone the lines of
> osabi.c:can_run_code_for.
>

Thanks Ulrich.

I make a new patch that use "compatible".  Please help me review it.

Thanks,
Hui

2009-07-06  Hui Zhu  <teawater@gmail.com>

	* solib.c (solib_bfd_open): Check solib bfd arch.

---
 solib.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/solib.c
+++ b/solib.c
@@ -289,6 +289,7 @@ solib_bfd_open (char *pathname)
   char *found_pathname;
   int found_file;
   bfd *abfd;
+  const struct bfd_arch_info *b;

   /* Use target-specific override if present.  */
   if (ops->bfd_open)
@@ -311,6 +312,15 @@ solib_bfd_open (char *pathname)
 	     found_pathname, bfd_errmsg (bfd_get_error ()));
     }

+  /* Check bfd arch.  */
+  b = gdbarch_bfd_arch_info (target_gdbarch);
+  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
+    {
+      bfd_close (abfd);
+      make_cleanup (xfree, found_pathname);
+      error (_("`%s': ARCH not same with inferior."), found_pathname);
+    }
+
   return abfd;
 }

[-- Attachment #2: solib-check-bfd_arch_info.txt --]
[-- Type: text/plain, Size: 739 bytes --]

---
 solib.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- a/solib.c
+++ b/solib.c
@@ -289,6 +289,7 @@ solib_bfd_open (char *pathname)
   char *found_pathname;
   int found_file;
   bfd *abfd;
+  const struct bfd_arch_info *b;
 
   /* Use target-specific override if present.  */
   if (ops->bfd_open)
@@ -311,6 +312,15 @@ solib_bfd_open (char *pathname)
 	     found_pathname, bfd_errmsg (bfd_get_error ()));
     }
 
+  /* Check bfd arch.  */
+  b = gdbarch_bfd_arch_info (target_gdbarch);
+  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
+    {
+      bfd_close (abfd);
+      make_cleanup (xfree, found_pathname);
+      error (_("`%s': ARCH not same with inferior."), found_pathname);
+    }
+
   return abfd;
 }
 

Re: [RFA] Check solib bfd arch

[Ulrich Weigand]

Hui Zhu wrote:

> +  /* Check bfd arch.  */
> +  b = gdbarch_bfd_arch_info (target_gdbarch);
> +  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
> +    {
> +      bfd_close (abfd);
> +      make_cleanup (xfree, found_pathname);
> +      error (_("`%s': ARCH not same with inferior."), found_pathname);
> +    }

I'm not sure this should be an error; we allow debugging a main
executable whose architecture does not match the target architecture
(if the user knows what they're doing), so IMO we should allow this
for shared libraries too.  A *warning* would be more appropriate.

Also, I think it would be good if the message mentioned the architecture
in question, along the lines of:

  warning (_("`%s': Shared library architecture %s is not compatible "
             "with target architecture %s."), found_pathname,
           bfd_get_arch_info (abfd)->printable_name, b->printable_name);

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Check solib bfd arch

[Hui Zhu]

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

On Wed, Jul 8, 2009 at 00:45, Ulrich Weigand<uweigand@de.ibm.com> wrote:
> Hui Zhu wrote:
>
>> +  /* Check bfd arch.  */
>> +  b = gdbarch_bfd_arch_info (target_gdbarch);
>> +  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
>> +    {
>> +      bfd_close (abfd);
>> +      make_cleanup (xfree, found_pathname);
>> +      error (_("`%s': ARCH not same with inferior."), found_pathname);
>> +    }
>
> I'm not sure this should be an error; we allow debugging a main
> executable whose architecture does not match the target architecture
> (if the user knows what they're doing), so IMO we should allow this
> for shared libraries too.  A *warning* would be more appropriate.
>
> Also, I think it would be good if the message mentioned the architecture
> in question, along the lines of:
>
>  warning (_("`%s': Shared library architecture %s is not compatible "
>             "with target architecture %s."), found_pathname,
>           bfd_get_arch_info (abfd)->printable_name, b->printable_name);
>

Agree with you.  I make a new patch for it.
Please help me review it.  Thanks.

Hui

2009-07-08  Hui Zhu  <teawater@gmail.com>

	* solib.c (solib_bfd_open): Output a warning if solib's
	architecture is not compatible with inferior's architecture.

---
 solib.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/solib.c
+++ b/solib.c
@@ -289,6 +289,7 @@ solib_bfd_open (char *pathname)
   char *found_pathname;
   int found_file;
   bfd *abfd;
+  const struct bfd_arch_info *b;

   /* Use target-specific override if present.  */
   if (ops->bfd_open)
@@ -311,6 +312,13 @@ solib_bfd_open (char *pathname)
 	     found_pathname, bfd_errmsg (bfd_get_error ()));
     }

+  /* Check bfd arch.  */
+  b = gdbarch_bfd_arch_info (target_gdbarch);
+  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
+    warning (_("`%s': Shared library architecture %s is not compatible "
+               "with target architecture %s."), found_pathname,
+             bfd_get_arch_info (abfd)->printable_name, b->printable_name);
+
   return abfd;
 }

[-- Attachment #2: solib-check-bfd_arch_info.txt --]
[-- Type: text/plain, Size: 795 bytes --]

---
 solib.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/solib.c
+++ b/solib.c
@@ -289,6 +289,7 @@ solib_bfd_open (char *pathname)
   char *found_pathname;
   int found_file;
   bfd *abfd;
+  const struct bfd_arch_info *b;
 
   /* Use target-specific override if present.  */
   if (ops->bfd_open)
@@ -311,6 +312,13 @@ solib_bfd_open (char *pathname)
 	     found_pathname, bfd_errmsg (bfd_get_error ()));
     }
 
+  /* Check bfd arch.  */
+  b = gdbarch_bfd_arch_info (target_gdbarch);
+  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
+    warning (_("`%s': Shared library architecture %s is not compatible "
+               "with target architecture %s."), found_pathname,
+             bfd_get_arch_info (abfd)->printable_name, b->printable_name);
+
   return abfd;
 }
 

Re: [RFA] Check solib bfd arch

[Ulrich Weigand]

Hui Zhu wrote:

> 2009-07-08  Hui Zhu  <teawater@gmail.com>
> 
> 	* solib.c (solib_bfd_open): Output a warning if solib's
> 	architecture is not compatible with inferior's architecture.

This is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Check solib bfd arch

[Hui Zhu]

On Thu, Jul 9, 2009 at 20:21, Ulrich Weigand<uweigand@de.ibm.com> wrote:
> Hui Zhu wrote:
>
>> 2009-07-08  Hui Zhu  <teawater@gmail.com>
>>
>>       * solib.c (solib_bfd_open): Output a warning if solib's
>>       architecture is not compatible with inferior's architecture.
>
> This is OK.
>

Checked in.

Thanks,
Hui

Re: [RFA] Check solib bfd arch

[Joel Brobecker]

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

Hello,

The following patch is causing trouble on sparc-solaris:

> 2009-07-08  Hui Zhu  <teawater@gmail.com>
> 
> 	* solib.c (solib_bfd_open): Output a warning if solib's
> 	architecture is not compatible with inferior's architecture.

I now see:

    (gdb) run
    Starting program: /[...]/sparc64/ex/task_switch 
    warning: `/usr/platform/SUNW,Sun-Fire-V440/lib/sparcv9/libc_psr.so.1': Shared library architecture sparc:v9a is not compatible with target architecture sparc:v9.

> +  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)

In my case, b->compatible is bfd_default_compatible. the architecture
is set to sparc:v9, and the shared library's architecture is sparc:v9a.
The problem is that b->compatible is returning the architecture that
is "more featureful" of the two, which in this case is sparc:v9a.
As a result, we emit the warning.

Looks to me like the check is too aggressive and should be changed
to == 0. Would that be correct?

2009-09-16  Joel Brobecker  <brobecker@adacore.com>

        * solib.c (solib_bfd_open): Relax a bit the compatibility check.

Tested on sparc64-solaris.

-- 
Joel

[-- Attachment #2: solib.diff --]
[-- Type: text/x-diff, Size: 557 bytes --]

diff --git a/gdb/solib.c b/gdb/solib.c
index c7fd0fc..a2ad0c4 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -310,7 +310,7 @@ solib_bfd_open (char *pathname)
 
   /* Check bfd arch.  */
   b = gdbarch_bfd_arch_info (target_gdbarch);
-  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
+  if (!b->compatible (b, bfd_get_arch_info (abfd)))
     warning (_("`%s': Shared library architecture %s is not compatible "
                "with target architecture %s."), found_pathname,
              bfd_get_arch_info (abfd)->printable_name, b->printable_name);

Re: [RFA] Check solib bfd arch

[Hui Zhu]

Hi Joel,

Thanks for you work.

Could you please try it with "bfd_arch_rs6000 and bfd_arch_powerpc"?
Or Ulrich, maybe you can help us with it.  :)

Thanks,
Hui

On Fri, Sep 18, 2009 at 06:56, Joel Brobecker <brobecker@adacore.com> wrote:
> Hello,
>
> The following patch is causing trouble on sparc-solaris:
>
>> 2009-07-08  Hui Zhu  <teawater@gmail.com>
>>
>>       * solib.c (solib_bfd_open): Output a warning if solib's
>>       architecture is not compatible with inferior's architecture.
>
> I now see:
>
>    (gdb) run
>    Starting program: /[...]/sparc64/ex/task_switch
>    warning: `/usr/platform/SUNW,Sun-Fire-V440/lib/sparcv9/libc_psr.so.1': Shared library architecture sparc:v9a is not compatible with target architecture sparc:v9.
>
>> +  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
>
> In my case, b->compatible is bfd_default_compatible. the architecture
> is set to sparc:v9, and the shared library's architecture is sparc:v9a.
> The problem is that b->compatible is returning the architecture that
> is "more featureful" of the two, which in this case is sparc:v9a.
> As a result, we emit the warning.
>
> Looks to me like the check is too aggressive and should be changed
> to == 0. Would that be correct?
>
> 2009-09-16  Joel Brobecker  <brobecker@adacore.com>
>
>        * solib.c (solib_bfd_open): Relax a bit the compatibility check.
>
> Tested on sparc64-solaris.
>
> --
> Joel
>

Re: [RFA] Check solib bfd arch

[Ulrich Weigand]

Joel Brobecker wrote:

> I now see:
> 
>     (gdb) run
>     Starting program: /[...]/sparc64/ex/task_switch 
>     warning: `/usr/platform/SUNW,Sun-Fire-V440/lib/sparcv9/libc_psr.so.1': Shared library architecture sparc:v9a is not compatible with target architecture sparc:v9.
> 
> > +  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
> 
> In my case, b->compatible is bfd_default_compatible. the architecture
> is set to sparc:v9, and the shared library's architecture is sparc:v9a.
> The problem is that b->compatible is returning the architecture that
> is "more featureful" of the two, which in this case is sparc:v9a.
> As a result, we emit the warning.
> 
> Looks to me like the check is too aggressive and should be changed
> to == 0. Would that be correct?

Well, the check is modeled after the one in osabi.c:can_run_code_for
which carries the comment:

  /* BFD's 'A->compatible (A, B)' functions return zero if A and B are
     incompatible.  But if they are compatible, it returns the 'more
     featureful' of the two arches.  That is, if A can run code
     written for B, but B can't run code written for A, then it'll
     return A.

In your particular case, the result of compatible appears to indicate
that the target architecture sparc:v9 *cannot* run code written for
the architecture sparc:v9a; if this were true (I'm not sure about 
such sparc architecture details), then it would be correct to reject
those shared libraries ...

In any case, I do not think the checks in osabi.c (for the main
executable) and in solib.c (for shared libraries) should be different.

Hui Zhu wrote:

> Could you please try it with "bfd_arch_rs6000 and bfd_arch_powerpc"?
> Or Ulrich, maybe you can help us with it.  :)

Unfortunately, I don't have access to an old-style RS/6000 machine
myself ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Check solib bfd arch

[Ulrich Weigand]

I wrote:

> In your particular case, the result of compatible appears to indicate
> that the target architecture sparc:v9 *cannot* run code written for
> the architecture sparc:v9a; if this were true (I'm not sure about 
> such sparc architecture details), then it would be correct to reject
> those shared libraries ...

Following up on myself, here's what the Sun Studio 12: C User's Guide
says on the topic (http://docs.sun.com/app/docs/doc/819-5265/bjasq):

   Object binary files (.o) compiled with v9 and v9a can be linked and
   can execute together, but will run only on a SPARC V9a compatible
   platform.

So maybe your problem is really that the target platform is actually
v9a, but it is incorrectly detected as just v9 ?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Check solib bfd arch

[Daniel Jacobowitz]

On Fri, Sep 18, 2009 at 02:50:22PM +0200, Ulrich Weigand wrote:
> In your particular case, the result of compatible appears to indicate
> that the target architecture sparc:v9 *cannot* run code written for
> the architecture sparc:v9a; if this were true (I'm not sure about 
> such sparc architecture details), then it would be correct to reject
> those shared libraries ...

Where are we getting the architecture settings from?

I'll pick an example I know better.  Suppose a binary is built for
a PowerPC 603 but it loads a shared library built for a PowerPC 7400
with Altivec.  The check would fail in the way Joel described if we're
getting the architecture from the input file, but this is a valid
operation.  If we're getting it from the target, then we've got the
wrong architecture if PowerPC 7400 binaries were supported - clearly
it isn't a 603.  But not all targets report the architecture.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Check solib bfd arch

[Joel Brobecker]

> I'll pick an example I know better.  Suppose a binary is built for
> a PowerPC 603 but it loads a shared library built for a PowerPC 7400
> with Altivec.  The check would fail in the way Joel described if we're
> getting the architecture from the input file, but this is a valid
> operation.  If we're getting it from the target, then we've got the
> wrong architecture if PowerPC 7400 binaries were supported - clearly
> it isn't a 603.  But not all targets report the architecture.

That's exactly what I thought was happening. I remembered that the
architecture is determined from the binary, so I assumed that my
executable was built as a sparcv9 but it was linking in a sparcv9a
shared library.

Marc's suggestion of enhancing the nat module for Sparc to detect
if the machine can run Sparcv9 is interesting, but we would still
have the same issue if we were cross debugging, no?

-- 
Joel

Re: [RFA] Check solib bfd arch

[Mark Kettenis]

> Date: Fri, 18 Sep 2009 14:50:22 +0200 (CEST)
> From: "Ulrich Weigand" <uweigand@de.ibm.com>
> 
> In your particular case, the result of compatible appears to indicate
> that the target architecture sparc:v9 *cannot* run code written for
> the architecture sparc:v9a; if this were true (I'm not sure about 
> such sparc architecture details), then it would be correct to reject
> those shared libraries ...

Yes, v9a is an extension to v9, so a target that only has support for
v9 cannot run v9a binaries.

I suppose we needsto add the necessary magic to sparc-sol-nat.c to set
the proper architecture.  I'll need to figure out how to extract that
information from the system.  Is it possible to create a target
description that just contains the architecture and not all the
register stuff?

Re: [RFA] Check solib bfd arch

[Mark Kettenis]

> Date: Fri, 18 Sep 2009 16:07:46 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> 
> > Date: Fri, 18 Sep 2009 14:50:22 +0200 (CEST)
> > From: "Ulrich Weigand" <uweigand@de.ibm.com>
> > 
> > In your particular case, the result of compatible appears to indicate
> > that the target architecture sparc:v9 *cannot* run code written for
> > the architecture sparc:v9a; if this were true (I'm not sure about 
> > such sparc architecture details), then it would be correct to reject
> > those shared libraries ...
> 
> Yes, v9a is an extension to v9, so a target that only has support for
> v9 cannot run v9a binaries.
> 
> I suppose we needsto add the necessary magic to sparc-sol-nat.c to set
> the proper architecture.  I'll need to figure out how to extract that
> information from the system.  Is it possible to create a target
> description that just contains the architecture and not all the
> register stuff?

Oh, and for 7.0 I think we should simply disable that check.

Re: [RFA] Check solib bfd arch

[Daniel Jacobowitz]

On Fri, Sep 18, 2009 at 04:07:46PM +0200, Mark Kettenis wrote:
> I suppose we needsto add the necessary magic to sparc-sol-nat.c to set
> the proper architecture.  I'll need to figure out how to extract that
> information from the system.  Is it possible to create a target
> description that just contains the architecture and not all the
> register stuff?

Yes, this is easy.  If there aren't any registers, the register bits
will be disabled.  The XML version would be something like:

<!DOCTYPE target SYSTEM "gdb-target.dtd">
<target>
  <architecture>sparc:sparcv9</architecture>
  <osabi>Solaris</osabi>
</target>

The C version could be generated from that, or if you want to do it
dynamically:

  struct target_desc *result = allocate_target_description ();
  set_tdesc_architecture (result, my_bfd_arch_info_ptr);

After that, mips_linux_read_description is a simple example of how to
supply the description.

I suspect the check is going to be problematic whether we do this or
not, though.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Check solib bfd arch

[Pedro Alves]

On Friday 18 September 2009 15:39:09, Daniel Jacobowitz wrote:
> I suspect the check is going to be problematic whether we do this or
> not, though.

Could we compare the solib's target arch with
tdesc_architecture(target_read_description()) directly --- but only
if available ---, instead of comparing it with target_gdbarch?

In pseudo-patch:

-  b = gdbarch_bfd_arch_info (target_gdbarch);
-  if (b->compatible (b, bfd_get_arch_info (abfd)) != b)
+  tdesc = target_read_description ();
+  b = (tdesc != NULL) ? gdbarch_bfd_arch_info (tdesc_architecture (tdesc)) : NULL;
+  if (b != NULL && b->compatible (b, bfd_get_arch_info (abfd)) != b)
    warning (_("`%s': Shared library architecture %s is not compatible "
               "with target architecture %s."), found_pathname,
             bfd_get_arch_info (abfd)->printable_name, b->printable_name);

-- 
Pedro Alves

Re: [RFA] Check solib bfd arch

[Ulrich Weigand]

Pedro Alves wrote:

> Could we compare the solib's target arch with
> tdesc_architecture(target_read_description()) directly --- but only
> if available ---, instead of comparing it with target_gdbarch?

Hmm, this would ignore the work done by choose_architecture_for_target.

Maybe we should compare against target_gdbarch, but only if a target
description is present?  Just wrap the whole test as-is in a 
  if (target_read_description ())
    ...

In any case, at this point the best option for 7.0 might indeed be
to just disable the test.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFA] Check solib bfd arch

[Daniel Jacobowitz]

On Fri, Sep 18, 2009 at 06:14:32PM +0200, Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
> > Could we compare the solib's target arch with
> > tdesc_architecture(target_read_description()) directly --- but only
> > if available ---, instead of comparing it with target_gdbarch?
> 
> Hmm, this would ignore the work done by choose_architecture_for_target.
> 
> Maybe we should compare against target_gdbarch, but only if a target
> description is present?  Just wrap the whole test as-is in a 
>   if (target_read_description ())
>     ...

A description can cover many areas, but is not required to cover any
of them.  I don't think anything should rely on "is there a
description".

It does seem to me like the test should be symmetric here:
  a = bfd_get_arch_info (abfd);
  if (b->compatible (b, a) != b
      && a->compatible (a, b) != a)

The current version is only right if we know the most specific
supported architecture for the target, which I think is not the
question we ought to ask.  We get away with this in osabi.c because we
know that handler->arch_info is the most general supported
architecture for the OSABI handler.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Check solib bfd arch

[Pedro Alves]

On Friday 18 September 2009 17:25:11, Daniel Jacobowitz wrote:

> A description can cover many areas, but is not required to cover any
> of them.  I don't think anything should rely on "is there a
> description".

Good point, although we could check if the description
included an arch.

> It does seem to me like the test should be symmetric here:
>   a = bfd_get_arch_info (abfd);
>   if (b->compatible (b, a) != b
>       && a->compatible (a, b) != a)

I think that's the same in practice as Joel's original
patch then, since `compatible' either returns either
NULL, a or b.  This still catches the
obviously-incompatible cases, so may be worth keeping,
while anything smarter is probably not worth the effort
then.

-- 
Pedro Alves

Re: [RFA] Check solib bfd arch

[Daniel Jacobowitz]

On Fri, Sep 18, 2009 at 05:41:43PM +0100, Pedro Alves wrote:
> > It does seem to me like the test should be symmetric here:
> >   a = bfd_get_arch_info (abfd);
> >   if (b->compatible (b, a) != b
> >       && a->compatible (a, b) != a)
> 
> I think that's the same in practice as Joel's original
> patch then, since `compatible' either returns either
> NULL, a or b.  This still catches the
> obviously-incompatible cases, so may be worth keeping,
> while anything smarter is probably not worth the effort
> then.

You're right, this is the same in practice.  I don't have a strong
preference as to which form we use; whichever folks find clearer.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Check solib bfd arch

[Pedro Alves]

On Friday 18 September 2009 17:41:43, Pedro Alves wrote:
> I think that's the same in practice as Joel's original
> patch then, since `compatible' either returns either
> NULL, a or b.  This still catches the
> obviously-incompatible cases, so may be worth keeping,
> while anything smarter is probably not worth the effort
> then.
> 

Grrrr...

     Some targets (e.g. MIPS as of 2006-12-04) don't fully
     implement this, instead always returning NULL or the first
     argument.  We detect that case by checking both directions.  */

  compat1 = selected->compatible (selected, from_target);
  compat2 = from_target->compatible (from_target, selected);

-- 
Pedro Alves

Re: [RFA] Check solib bfd arch

[Joel Brobecker]

>      Some targets (e.g. MIPS as of 2006-12-04) don't fully
>      implement this, instead always returning NULL or the first
>      argument.  We detect that case by checking both directions.  */
> 
>   compat1 = selected->compatible (selected, from_target);
>   compat2 = from_target->compatible (from_target, selected);

Re-confirmed:

static const bfd_arch_info_type *
mips_compatible (const bfd_arch_info_type *a, const bfd_arch_info_type *b)
{
  if (a->arch != b->arch)
    return NULL;

  /* Machine compatibility is checked in
     _bfd_mips_elf_merge_private_bfd_data.  */

  return a;
}

I will update my patch accordingly... This probably warrants a function,
probably in gdbarch-utils.

-- 
Joel

Re: [RFA] Check solib bfd arch

[Joel Brobecker]

> >      Some targets (e.g. MIPS as of 2006-12-04) don't fully
> >      implement this, instead always returning NULL or the first
> >      argument.  We detect that case by checking both directions.  */
> > 
> >   compat1 = selected->compatible (selected, from_target);
> >   compat2 = from_target->compatible (from_target, selected);
[...]
> I will update my patch accordingly... This probably warrants a function,
> probably in gdbarch-utils.

Actually, I don't think that this will be needed. Regardless of
the order we do things, it's always going to return non-null if
both architectures are mips, which is the best we can check in
that case.

-- 
Joel

Re: [RFA] Check solib bfd arch

[Joel Brobecker]

> 2009-09-16  Joel Brobecker  <brobecker@adacore.com>
> 
>         * solib.c (solib_bfd_open): Relax a bit the compatibility check.

I just checked this patch in.

-- 
Joel