Re: Patches for IRIX6 N32-ABI ld

Re: Patches for IRIX6 N32-ABI ld

[Mark Mitchell]

>>>>> "Ian" == Ian Lance Taylor <ian@zembu.com> writes:

    Ian> I'm not sure why you want to bother, probably because I
    Ian> haven't seen the rest of your patches.

    Ian> Why not have two macros, one for general SGI compatibility,
    Ian> namely the existing SGI_COMPAT, and one new one you can use
    Ian> to check just which sort of SGI compatibility you want for a
    Ian> particular BFD?

Your suggestion is clearly equally expressive, so it's not like
something can be done way and not the other.  So, if you insist on
doing things your way, it's not like we'll lose anything.

I assumed this to be non-controversial, and used things like
`SGI_COMPAT (abfd) == sct_irix6' throughout the rest of my patches.
So, changing this will require a bunch of extra work for me.  That's
not a great argument, but it's accurate.

Also, "general SGI compatibility" is not a very well-defined notion.
The whole reason this patch is necessary is that SGI_COMPAT turns on
some behavior that isn't true in IRIX6; things have changed.  They'll
probably change again if there's ever an IRIX7, etc.

So, saying `if (SGI_COMPAT (abfd))' is a maintenance headache anyhow.
(It certainly was for me, at least, when working on the code.)  My
change makes it possible to gradually obsolete this usage.  In
particular, there's no need to change any existing code, but, on the
other hand, it's easy to change conditions to express just what
versions of IRIX things are appropriate for.

I'm really not looking to get embroiled in line-edits, especially on
the first set of diffs that add up to 3000 lines.  So, I'll not argue
the decision any further; I'll just abide by your next reply.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Patches for IRIX6 N32-ABI ld

[Richard Henderson]

On Sat, Jun 26, 1999 at 11:01:55AM -0700, Mark Mitchell wrote:
> -	  if (SGI_COMPAT (abfd)
> +	  if (SGI_COMPAT (output_bfd)

This isn't correct anymore, now that this is an enumeration.
Otherwise it's fine.


r~

Re: Patches for IRIX6 N32-ABI ld

[Ian Lance Taylor]

   From: Mark Mitchell <mark@codesourcery.com>
   Date: Sat, 26 Jun 1999 13:46:22 -0700

       Ian> In other words, I think either SGI_COMPAT should continue to
       Ian> be defined as a clearly boolean value (e.g., 1) or you should
       Ian> modify all the tests of SGI_COMPAT to work in some different
       Ian> manner.

   I understand what you're saying.  But, this is a common programming
   idiom, and well understood.  How about explicitly setting `sct_none'
   to zero:

     enum {
       sct_none = 0,
       ...
     }

   together with a comment?

I'm not sure why you want to bother, probably because I haven't seen
the rest of your patches.

Why not have two macros, one for general SGI compatibility, namely the
existing SGI_COMPAT, and one new one you can use to check just which
sort of SGI compatibility you want for a particular BFD?

Ian

Re: Patches for IRIX6 N32-ABI ld

[Mark Mitchell]

>>>>> "Ian" == Ian Lance Taylor <ian@zembu.com> writes:

    Ian> as far as I am concerned long term maintainability is the
    Ian> single most important characteristic of all code changes.  I
    Ian> will pick maintainability over features every time.  

You're preaching to the choir. :-) I'm always adamant about this point
myself when in comes to C++ changes in EGCS.

    Ian> I don't really care.  But I agree with Richard: I don't want
    Ian> to see code that will silently fail if a new value is added
    Ian> to the start of an enum.  

The thing we're disagreeing about is the unmainaintainability of the
proposed change.  I'm very used to the idiom in question (we use it
*lots* in GCC; functions return 0 to indicate falsity, and various
integers to indicate differents kinds of truth, which some callers
care about and some don't.)  So, you should be arguing with me about
the maintainability of this idiom, not telling me to prize
maintainability! :-) :-)

But, hey, reasonable people can agree to disagree.  

I'll make the change you want and put it in.
    
--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Patches for IRIX6 N32-ABI ld

[Mark Mitchell]

>>>>> "Ian" == Ian Lance Taylor <ian@zembu.com> writes:

    Ian> I for one would be much more comfortable with testing a
    Ian> boolean expression rather than writing a test based on the
    Ian> assumption that an enum value is and will remain zero.  It
    Ian> seems too easy for somebody to change the enum and thus
    Ian> unexpectedly change the behaviour of the code.  I believe the
    Ian> patch as written introduces a maintenance pitfall.

Wow, I'd hoped only to get controversial at the time we started
talking about relocation processing and stuff like that.  :-) 

    Ian> In other words, I think either SGI_COMPAT should continue to
    Ian> be defined as a clearly boolean value (e.g., 1) or you should
    Ian> modify all the tests of SGI_COMPAT to work in some different
    Ian> manner.

I understand what you're saying.  But, this is a common programming
idiom, and well understood.  How about explicitly setting `sct_none'
to zero:

  enum {
    sct_none = 0,
    ...
  }

together with a comment?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Patches for IRIX6 N32-ABI ld

[Ian Lance Taylor]

   From: Mark Mitchell <mark@codesourcery.com>
   Date: Sat, 26 Jun 1999 12:05:19 -0700

   >>>>> "Richard" == Richard Henderson <rth@cygnus.com> writes:

       Richard> On Sat, Jun 26, 1999 at 11:01:55AM -0700, Mark Mitchell
       Richard> wrote:
       >> - if (SGI_COMPAT (abfd) + if (SGI_COMPAT (output_bfd)

       Richard> This isn't correct anymore, now that this is an
       Richard> enumeration.  Otherwise it's fine.

   Why not?  It's still zero (i.e., sct_none) if we're not trying for
   compatibility and non-zero (i.e., sct_irix5 or sct_irix6) otherwise?
   The point of that change is that there is no `abfd' in that function.
   The BFD is `output_bfd'; before `abfd' was accepted only because
   SGI_COMPAT didn't look at its argument.

   What am I missing?

I for one would be much more comfortable with testing a boolean
expression rather than writing a test based on the assumption that an
enum value is and will remain zero.  It seems too easy for somebody to
change the enum and thus unexpectedly change the behaviour of the
code.  I believe the patch as written introduces a maintenance
pitfall.

In other words, I think either SGI_COMPAT should continue to be
defined as a clearly boolean value (e.g., 1) or you should modify all
the tests of SGI_COMPAT to work in some different manner.

Ian

Patches for IRIX6 N32-ABI ld

[Mark Mitchell]

I'm going to begin submitting the patches required to support the N32
ABI on IRIX6.  Although these have been tested heavily on IRIX6 (a
complete EGCS build and regression test-run), they have not been
tested no any other MIPS platform.  Therefore, it is possible
(likely?) that I have broken something elsewhere.  I hope that people
will test these patches, and report problems; I will, of course,
attempt to fix them as quickly as possible.

I'm going to try to keep the individual patches as simple as possible,
and do the easiest ones first.  That way, we can look at each change
and make sure it makes sense, and avoid getting bogged down.

This first one is very simple.  OK to check in?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

1999-06-26  Mark Mitchell  <mark@codesourcery.com>

	* elf32-mips.c (sgi_compat_t): New enumeration.
	(ABI_N32_P): New macro.
	(SGI_COMPAT): Return what kind of compatibility is being 
	provided.
	(mips_elf_relocate_section): Fix typo.

Index: elf32-mips.c
===================================================================
RCS file: /cvs/binutils/binutils/bfd/elf32-mips.c,v
retrieving revision 1.5
diff -u -p -r1.5 elf32-mips.c
--- elf32-mips.c	1999/06/12 13:08:29	1.5
+++ elf32-mips.c	1999/06/26 17:53:13
@@ -125,12 +125,26 @@ static bfd_byte *elf32_mips_get_relocate
   PARAMS ((bfd *, struct bfd_link_info *, struct bfd_link_order *,
 	   bfd_byte *, boolean, asymbol **));
 
-/* This is true for Irix 5 executables, false for normal MIPS ELF ABI
-   executables.  FIXME: At the moment, we default to always generating
-   Irix 5 executables.  */
+/* The kind of SGI compatibility we're striving for.  */
 
-#define SGI_COMPAT(abfd) (1)
+typedef enum {
+  sct_none,
+  sct_irix5,
+  sct_irix6
+} sgi_compat_t;
 
+/* Nonzero if ABFD is using the N32 ABI.  */
+
+#define ABI_N32_P(abfd) \
+  ((elf_elfheader (abfd)->e_flags & EF_MIPS_ABI2) != 0)
+
+/* What version of Irix we are trying to be compatible with.  FIXME:
+   At the moment, we never generate "normal" MIPS ELF ABI executables;
+   we always use some version of Irix.  */
+
+#define SGI_COMPAT(abfd) \
+  (ABI_N32_P (abfd) ? sct_irix6 : sct_irix5)
+
 /* This structure is used to hold .got information when linking.  It
    is stored in the tdata field of the bfd_elf_section_data structure.  */
 
@@ -5638,7 +5652,7 @@ mips_elf_relocate_section (output_bfd, i
 	      && (r == bfd_reloc_undefined || r == bfd_reloc_overflow))
 	    r = bfd_reloc_ok;
 
-	  if (SGI_COMPAT (abfd)
+	  if (SGI_COMPAT (output_bfd)
 	      && scpt != NULL
 	      && (input_section->flags & SEC_ALLOC) != 0)
 	    {

Re: Patches for IRIX6 N32-ABI ld

[Mark Mitchell]

>>>>> "Richard" == Richard Henderson <rth@cygnus.com> writes:

    Richard> On Sat, Jun 26, 1999 at 11:01:55AM -0700, Mark Mitchell
    Richard> wrote:
    >> - if (SGI_COMPAT (abfd) + if (SGI_COMPAT (output_bfd)

    Richard> This isn't correct anymore, now that this is an
    Richard> enumeration.  Otherwise it's fine.

Why not?  It's still zero (i.e., sct_none) if we're not trying for
compatibility and non-zero (i.e., sct_irix5 or sct_irix6) otherwise?
The point of that change is that there is no `abfd' in that function.
The BFD is `output_bfd'; before `abfd' was accepted only because
SGI_COMPAT didn't look at its argument.

What am I missing?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Patches for IRIX6 N32-ABI ld

[Richard Henderson]

On Sat, Jun 26, 1999 at 02:09:24PM -0700, Mark Mitchell wrote:
> I assumed this to be non-controversial, and used things like
> `SGI_COMPAT (abfd) == sct_irix6' throughout the rest of my patches.

I have no problem with that.  All I was looking for in the
quoted patch was either `SGI_COMPAT (foo) == sct_none' or
`SGI_COMPAT (foo) == sct_irix5' as appropriate.

Especially since SGI_COMPAT had used to mean explicitly 
Irix5, and now that conditional is changed to Irix5 or Irix6.
Which may be correct, but I want to know you meant it.


r~

Re: Patches for IRIX6 N32-ABI ld

[Mark Mitchell]

>>>>> "Richard" == Richard Henderson <rth@cygnus.com> writes:

    Richard> Especially since SGI_COMPAT had used to mean explicitly
    Richard> Irix5, and now that conditional is changed to Irix5 or
    Richard> Irix6.  Which may be correct, but I want to know you
    Richard> meant it.

FWIW, I meant it.  Many, but not all, of the previously SGI_COMPAT
things are stil true under IRIX6.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: Patches for IRIX6 N32-ABI ld

[Ian Lance Taylor]

   From: Mark Mitchell <mark@codesourcery.com>
   Date: Sat, 26 Jun 1999 14:09:24 -0700

       Ian> Why not have two macros, one for general SGI compatibility,
       Ian> namely the existing SGI_COMPAT, and one new one you can use
       Ian> to check just which sort of SGI compatibility you want for a
       Ian> particular BFD?

   Your suggestion is clearly equally expressive, so it's not like
   something can be done way and not the other.  So, if you insist on
   doing things your way, it's not like we'll lose anything.

   I assumed this to be non-controversial, and used things like
   `SGI_COMPAT (abfd) == sct_irix6' throughout the rest of my patches.
   So, changing this will require a bunch of extra work for me.  That's
   not a great argument, but it's accurate.

Then do it the other way.  Change all the existing uses to, e.g.,
SGI_IRIX_COMPAT (a trivial search and replace).  Then use SGI_COMPAT
when you need to decide the exact level of compatibility.

I don't really care.  But I agree with Richard: I don't want to see
code that will silently fail if a new value is added to the start of
an enum.  Although I admit you wouldn't know it to look at the
binutils, as far as I am concerned long term maintainability is the
single most important characteristic of all code changes.  I will pick
maintainability over features every time.  The code should not only
work correctly, it should be written in a natural style, so that
natural changes will not fail in an unnatural way.  Sometimes that
requires major patches when old code was done inappropriately or even
badly; so it goes.

Ian