RFA: osabi: correct test for compatible handlers

RFA: osabi: correct test for compatible handlers

[Jim Blandy]

I think I've found the answer to the question Kris and Daniel were
discussing back in June:

http://sources.redhat.com/ml/gdb/2003-06/msg00323.html

The following patch would, I believe, fix Kris's problem, but the
comments also explain why the patch he proposed isn't the right fix.

The effect of the patch is to change the existing test:
        
  arch_info->compatible (arch_info, handler->arch_info) == handler->arch_info

to:

  arch_info == handler->arch_info
  || arch_info->compatible (arch_info, handler->arch_info) == arch_info

but it's wrapped up and commented in a way that makes it clearer why
that is correct.


2003-10-21  Jim Blandy  <jimb@redhat.com>

	* osabi.c (gdbarch_init_osabi): A handler is okay if it's for an
	architecture the current arch can run code for --- but not if it's
	a superset.
	(can_run_code_for): New function.

Index: gdb/osabi.c
===================================================================
RCS file: /cvs/src/src/gdb/osabi.c,v
retrieving revision 1.17
diff -c -r1.17 osabi.c
*** gdb/osabi.c	24 Aug 2003 11:47:18 -0000	1.17
--- gdb/osabi.c	21 Oct 2003 22:15:18 -0000
***************
*** 283,293 ****
      return match;
  }
  
  void
  gdbarch_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
  {
    const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (gdbarch);
-   const struct bfd_arch_info *compatible;
    struct gdb_osabi_handler *handler;
  
    if (info.osabi == GDB_OSABI_UNKNOWN)
--- 283,311 ----
      return match;
  }
  
+ 
+ /* Return non-zero if architecture A can run code written for
+    architecture B.  */
+ static int
+ can_run_code_for (const struct bfd_arch_info *a, const struct bfd_arch_info *b)
+ {
+   /* 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.
+ 
+      struct bfd_arch_info objects are atoms: that is, there's supposed
+      to be exactly one instance for a given machine.  So you can tell
+      whether two are equivalent by comparing pointers.  */
+   return (a == b || a->compatible (a, b) == a);
+ }
+ 
+ 
  void
  gdbarch_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
  {
    const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (gdbarch);
    struct gdb_osabi_handler *handler;
  
    if (info.osabi == GDB_OSABI_UNKNOWN)
***************
*** 303,318 ****
        if (handler->osabi != info.osabi)
  	continue;
  
!       /* Check whether the machine type and architecture of the
!          handler are compatible with the desired machine type and
!          architecture.
  
! 	 NOTE: kettenis/20021027: There may be more than one machine
  	 type that is compatible with the desired machine type.  Right
  	 now we simply return the first match, which is fine for now.
  	 However, we might want to do something smarter in the future.  */
!       compatible = arch_info->compatible (arch_info, handler->arch_info);
!       if (compatible == handler->arch_info)
  	{
  	  (*handler->init_osabi) (info, gdbarch);
  	  return;
--- 321,339 ----
        if (handler->osabi != info.osabi)
  	continue;
  
!       /* If the architecture described by ARCH_INFO can run code for
!          the architcture we registered the handler for, then the
!          handler is applicable.  Note, though, that if the handler is
!          for an architecture that is a superset of ARCH_INFO, we can't
!          use that --- it would be perfectly correct for it to install
!          gdbarch methods that refer to registers / instructions /
!          other facilities ARCH_INFO doesn't have.
  
!          NOTE: kettenis/20021027: There may be more than one machine
  	 type that is compatible with the desired machine type.  Right
  	 now we simply return the first match, which is fine for now.
  	 However, we might want to do something smarter in the future.  */
!       if (can_run_code_for (arch_info, handler->arch_info))
  	{
  	  (*handler->init_osabi) (info, gdbarch);
  	  return;

Re: RFA: osabi: correct test for compatible handlers

[Andrew Cagney]

> +   /* 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.
> + 
> +      struct bfd_arch_info objects are atoms: that is, there's supposed
> +      to be exactly one instance for a given machine.  So you can tell
> +      whether two are equivalent by comparing pointers.  */
> +   return (a == b || a->compatible (a, b) == a);

Hey, nice.

Don't worry about a can_run_code_for function though, having the logic 
inline makes what's happening easier to understand (and will simplify a 
follow-on wild-card patch I've got pending).

Andrew

Re: RFA: osabi: correct test for compatible handlers

[Daniel Jacobowitz]

On Wed, Oct 22, 2003 at 02:49:12PM -0400, Andrew Cagney wrote:
> >+   /* 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.
> >+ 
> >+      struct bfd_arch_info objects are atoms: that is, there's supposed
> >+      to be exactly one instance for a given machine.  So you can tell
> >+      whether two are equivalent by comparing pointers.  */
> >+   return (a == b || a->compatible (a, b) == a);
> 
> Hey, nice.
> 
> Don't worry about a can_run_code_for function though, having the logic 
> inline makes what's happening easier to understand (and will simplify a 
> follow-on wild-card patch I've got pending).

I'm curious, what do you mean by wild-card that isn't handled by
correcting various ->compatible functions?

[Stylisticly I prefer the code out-of-line since the name describes the
test so well.]

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA: osabi: correct test for compatible handlers

[Andrew Cagney]

> 
> I'm curious, what do you mean by wild-card that isn't handled by
> correcting various ->compatible functions?

Here's the bug report.
Rgdbarch_register_osabi machine wildcard (-1)
http://sources.redhat.com/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gdb&pr=1395
The background discussion occured not too long ago.

Andrew

Re: RFA: osabi: correct test for compatible handlers

[Jim Blandy]

Andrew Cagney <ac131313@redhat.com> writes:

> > +   /* 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.
> > + +      struct bfd_arch_info objects are atoms: that is, there's
> > supposed
> > +      to be exactly one instance for a given machine.  So you can tell
> > +      whether two are equivalent by comparing pointers.  */
> > +   return (a == b || a->compatible (a, b) == a);
> 
> Hey, nice.
> 
> Don't worry about a can_run_code_for function though, having the logic
> inline makes what's happening easier to understand (and will simplify
> a follow-on wild-card patch I've got pending).

It may be easier for you, but the original author did get the test
backwards, and I had to go through an embarrassing number of wrong
tries before I got it right.  I'd really like to leave the function
separate.

Re: RFA: osabi: correct test for compatible handlers

[Andrew Cagney]

> Andrew Cagney <ac131313@redhat.com> writes:
> 
> 
>> > +   /* 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.
>> > + +      struct bfd_arch_info objects are atoms: that is, there's
>> > supposed
>> > +      to be exactly one instance for a given machine.  So you can tell
>> > +      whether two are equivalent by comparing pointers.  */
>> > +   return (a == b || a->compatible (a, b) == a);
> 
>> 
>> Hey, nice.
>> 
>> Don't worry about a can_run_code_for function though, having the logic
>> inline makes what's happening easier to understand (and will simplify
>> a follow-on wild-card patch I've got pending).
> 
> 
> It may be easier for you, but the original author did get the test
> backwards, and I had to go through an embarrassing number of wrong
> tries before I got it right.  I'd really like to leave the function
> separate.

I had to go through an equally enbarrassing number of tries before I 
established exactly what the patch was doing.  Changing:

	if (compatible == handler->arch_info)

to:

	if (compatible == arch_info)

(correct?)  The really important thing here is your commentary as that 
explains exactly what is going on.  Having it as close as possible to 
the problem (the call site) is, I think, going to make things easier to 
understand.

Andrew

Re: RFA: osabi: correct test for compatible handlers

[Jim Blandy]

Andrew Cagney <ac131313@redhat.com> writes:

> > Andrew Cagney <ac131313@redhat.com> writes:
> >
> >> > +   /* 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.
> >> > + +      struct bfd_arch_info objects are atoms: that is, there's
> >> > supposed
> >> > +      to be exactly one instance for a given machine.  So you can tell
> >> > +      whether two are equivalent by comparing pointers.  */
> >> > +   return (a == b || a->compatible (a, b) == a);
> >
> >> Hey, nice.
> >> Don't worry about a can_run_code_for function though, having the
> >> logic
> >> inline makes what's happening easier to understand (and will simplify
> >> a follow-on wild-card patch I've got pending).
> > It may be easier for you, but the original author did get the test
> > backwards, and I had to go through an embarrassing number of wrong
> > tries before I got it right.  I'd really like to leave the function
> > separate.
> 
> I had to go through an equally enbarrassing number of tries before I
> established exactly what the patch was doing.  Changing:
> 
> 	if (compatible == handler->arch_info)
> 
> to:
> 
> 	if (compatible == arch_info)
> 
> (correct?)  The really important thing here is your commentary as that
> explains exactly what is going on.  Having it as close as possible to
> the problem (the call site) is, I think, going to make things easier
> to understand.

I agree it's harder to see what the *patch* does when I pull
everything out into a separate function --- you have to expand the
function in-place, and then compare before and after.

But I think it's easier to see what the *resulting code* does with the
function in place.  We should put the readability of the resultant
code above readability of the change.  You say, "A can use a handler
for B if A can run code for B", and then you can make a separate check
to see whether can_run_code_for is correct.

Re: RFA: osabi: correct test for compatible handlers

[David Carlton]

On 22 Oct 2003 18:16:31 -0500, Jim Blandy <jimb@redhat.com> said:

> But I think it's easier to see what the *resulting code* does with
> the function in place.  We should put the readability of the
> resultant code above readability of the change.  You say, "A can use
> a handler for B if A can run code for B", and then you can make a
> separate check to see whether can_run_code_for is correct.

I very much agree with this.

David Carlton
carlton@kealia.com

Re: RFA: osabi: correct test for compatible handlers

[Mark Kettenis]

   From: David Carlton <carlton@kealia.com>
   Date: Wed, 22 Oct 2003 16:28:10 -0700

   On 22 Oct 2003 18:16:31 -0500, Jim Blandy <jimb@redhat.com> said:

   > But I think it's easier to see what the *resulting code* does with
   > the function in place.  We should put the readability of the
   > resultant code above readability of the change.  You say, "A can use
   > a handler for B if A can run code for B", and then you can make a
   > separate check to see whether can_run_code_for is correct.

   I very much agree with this.

Being the person who got the test wrong in the first place I say:
Definitely!

Mark

Re: RFA: osabi: correct test for compatible handlers

[Jim Blandy]

Since the only concerns raised about this change are stylistic, and
the general sense seems to be that it's preferable as is, I've
committed this change.  If people are very concerned about whether
can_run_code_for is a separate function or not, we can always discuss
that as a separate patch.

Re: RFA: osabi: correct test for compatible handlers

[Andrew Cagney]

> But I think it's easier to see what the *resulting code* does with the
> function in place.  We should put the readability of the resultant
> code above readability of the change.  You say, "A can use a handler
> for B if A can run code for B", and then you can make a separate check
> to see whether can_run_code_for is correct.

I am thinking of the readability of the final code and putting that 
first.  Please don't suggest otherwize.

Andrew