Strange code in tc-mips.c

Strange code in tc-mips.c

[Thiemo Seufer]

Hi All,

while figuring out what's the use of mips_64 in tc-mips.c I found
this gem:

void
cons_fix_new_mips (frag, where, nbytes, exp)
     fragS *frag ATTRIBUTE_UNUSED;
     int where;
     unsigned int nbytes;
     expressionS *exp;
{
#ifndef OBJ_ELF
  /* If we are assembling in 32 bit mode, turn an 8 byte reloc into a
     4 byte reloc.  */
  if (nbytes == 8 && ! mips_64)
    {
      if (target_big_endian)
        where += 4;
      nbytes = 4;
    }
#endif

According to internals.texi, TC_CONS_FIX_NEW is used for fixups
of data allocation pseudo-ops (like .dword).

I can't see even the intention behind this:
  - A gas with ELF _support_ never uses this code.
  - Without ELF support, the check goes effectively for -mabi=64,
    not for 64bit assembly.
  - The code then silently discards the upper word. If larger
    data definitions are illegal in 32bit ECOFF(?), this should
    be checked earlier and with a warning.

This is (except of the option setting) the only place where
mips_64 is wrongly assumed to work as flag for 64bit assembly
instead of ABI64.

Has somebody a explanation what this code is good for?


Thiemo

Re: Strange code in tc-mips.c

[Ian Lance Taylor]

Thiemo Seufer <ica2_ts@csv.ica.uni-stuttgart.de> writes:

> while figuring out what's the use of mips_64 in tc-mips.c I found
> this gem:
> 
> void
> cons_fix_new_mips (frag, where, nbytes, exp)
>      fragS *frag ATTRIBUTE_UNUSED;
>      int where;
>      unsigned int nbytes;
>      expressionS *exp;
> {
> #ifndef OBJ_ELF
>   /* If we are assembling in 32 bit mode, turn an 8 byte reloc into a
>      4 byte reloc.  */
>   if (nbytes == 8 && ! mips_64)
>     {
>       if (target_big_endian)
>         where += 4;
>       nbytes = 4;
>     }
> #endif

I wrote that code to support 64 bit targets when using a 32 bit object
file format.  64 bit code sometimes naturally uses 64 bit addresses,
particularly when the compiler uses 64 bits for pointers.  However,
the compiler was ahead of the assembler in 64 bit support.  At the
time I wrote that code, ELF support was only available for 32 bit
MIPS.  Even today, ECOFF only supports 32 bit addresses.

That put us in the position of support 64 bit code with a 32 bit
object file format.  This works fine; although the linker of course
can only place sections at addresses which can be named with 32 bits,
the addresses of those sections can be used to initialize 64 bit
pointers.

This was unquestionably a hack.  It was a way to support a 64 bit
compiler without a 64 bit object file format.  In practice this was
enough to permit people to get work done for 64 bit MIPS chips.  Now
it may be OK to delete this code, and tell people using ECOFF that
they must use ELF for 64 bit code.

> According to internals.texi, TC_CONS_FIX_NEW is used for fixups
> of data allocation pseudo-ops (like .dword).

Note that it is only used when a fixup is required.  It is not used
for constants.

> I can't see even the intention behind this:
>   - A gas with ELF _support_ never uses this code.

It did at one time.  Now that 64 bit ELF is supported, it is no longer
necessary to enable this for ELF.

>   - Without ELF support, the check goes effectively for -mabi=64,
>     not for 64bit assembly.

When I wrote the test, it might have been done unconditionally for an
8 byte relocation.  I'm not sure why it is a benefit to test !
mips_64.

>   - The code then silently discards the upper word. If larger
>     data definitions are illegal in 32bit ECOFF(?), this should
>     be checked earlier and with a warning.

I don't think the code silently discards anything.  The code is just
creating a fixup.  If this leads to discarding real data, the checks
in fixup_segment should catch that.

Ian

Re: Strange code in tc-mips.c

[Thiemo Seufer]

Ian Lance Taylor wrote:
[snip]
> This was unquestionably a hack.  It was a way to support a 64 bit
> compiler without a 64 bit object file format.  In practice this was
> enough to permit people to get work done for 64 bit MIPS chips.  Now
> it may be OK to delete this code, and tell people using ECOFF that
> they must use ELF for 64 bit code.

Ah, now I understand the intention (and how the code mutated to
this bizarre thing).

[snip]
> > I can't see even the intention behind this:
> >   - A gas with ELF _support_ never uses this code.
> 
> It did at one time.  Now that 64 bit ELF is supported, it is no longer
> necessary to enable this for ELF.

Note that it's defined out even in an both ELF- and ECOFF-enabled
Assembler. I assume it's never used nowadays.

> >   - Without ELF support, the check goes effectively for -mabi=64,
> >     not for 64bit assembly.
> 
> When I wrote the test, it might have been done unconditionally for an
> 8 byte relocation.  I'm not sure why it is a benefit to test !
> mips_64.

The ChangeLog says it was your idea. :-) AFAICS removing it is
the best solution.


Thiemo


2001-08-02  Thiemo Seufer <seufer@csv.ica.uni-stuttgart.de>

	/gas/ChangeLog
	* tc-mips.c (cons_fix_new_mips): Remove.
	* tc-mips.h (TC_CONS_FIX_NEW): Remove.
	(cons_fix_new_mips): Remove.


diff -BurpNX /bigdisk/src/binutils-exclude src-orig/gas/config/tc-mips.c src/gas/config/tc-mips.c
--- src-orig/gas/config/tc-mips.c	Fri Aug  3 13:26:20 2001
+++ src/gas/config/tc-mips.c	Mon Aug  6 17:47:20 2001
@@ -9324,37 +9628,6 @@ md_pcrel_from (fixP)
   return fixP->fx_size + fixP->fx_where + fixP->fx_frag->fr_address;
 }
 
-/* This is called by emit_expr via TC_CONS_FIX_NEW when creating a
-   reloc for a cons.  We could use the definition there, except that
-   we want to handle 64 bit relocs specially.  */
-
-void
-cons_fix_new_mips (frag, where, nbytes, exp)
-     fragS *frag ATTRIBUTE_UNUSED;
-     int where;
-     unsigned int nbytes;
-     expressionS *exp;
-{
-#ifndef OBJ_ELF
-  /* If we are assembling in 32 bit mode, turn an 8 byte reloc into a
-     4 byte reloc.  */
-  if (nbytes == 8 && ! mips_64)
-    {
-      if (target_big_endian)
-	where += 4;
-      nbytes = 4;
-    }
-#endif
-
-  if (nbytes != 2 && nbytes != 4 && nbytes != 8)
-    as_bad (_("Unsupported reloc size %d"), nbytes);
-
-  fix_new_exp (frag_now, where, (int) nbytes, exp, 0,
-	       (nbytes == 2
-		? BFD_RELOC_16
-		: (nbytes == 4 ? BFD_RELOC_32 : BFD_RELOC_64)));
-}
-
 /* This is called before the symbol table is processed.  In order to
    work with gcc when using mips-tfile, we must keep all local labels.
    However, in other cases, we want to discard them.  If we were
diff -BurpNX /bigdisk/src/binutils-exclude src-orig/gas/config/tc-mips.h src/gas/config/tc-mips.h
--- src-orig/gas/config/tc-mips.h	Wed Mar 14 17:03:22 2001
+++ src/gas/config/tc-mips.h	Mon Aug  6 17:47:02 2001
@@ -102,10 +102,6 @@ extern void mips_frob_file PARAMS ((void
 #define tc_frob_file_after_relocs mips_frob_file_after_relocs
 extern void mips_frob_file_after_relocs PARAMS ((void));
 #endif
-
-#define TC_CONS_FIX_NEW cons_fix_new_mips
-extern void cons_fix_new_mips
-  PARAMS ((struct frag *, int, unsigned int, struct expressionS *));
 
 #define tc_fix_adjustable(fixp) mips_fix_adjustable (fixp)
 extern int mips_fix_adjustable PARAMS ((struct fix *));

Re: Strange code in tc-mips.c

[cgd]

ian@zembu.com ("Ian Lance Taylor") writes:
> > I can't see even the intention behind this:
> >   - A gas with ELF _support_ never uses this code.
> 
> It did at one time.  Now that 64 bit ELF is supported, it is no longer
> necessary to enable this for ELF.

Should this check be guarded with a check that ELF is the actual
object format?  (for the case where you're supporting ELF +
e.g. ECOFF, iirc where OBJ_MAYBE_ELF or something like that is
defined?)


chris

Re: Strange code in tc-mips.c

[Thiemo Seufer]

Thiemo Seufer wrote:
> Ian Lance Taylor wrote:
[snip]
> > When I wrote the test, it might have been done unconditionally for an
> > 8 byte relocation.  I'm not sure why it is a benefit to test !
> > mips_64.
> 
> The ChangeLog says it was your idea. :-) AFAICS removing it is
> the best solution.
> 
> 
> Thiemo
> 
> 
> 2001-08-02  Thiemo Seufer <seufer@csv.ica.uni-stuttgart.de>
> 
> 	/gas/ChangeLog
> 	* tc-mips.c (cons_fix_new_mips): Remove.
> 	* tc-mips.h (TC_CONS_FIX_NEW): Remove.
> 	(cons_fix_new_mips): Remove.

If there aren't objections, I'll apply this patch the next days.


Thiemo

Re: Strange code in tc-mips.c

[Eric Christopher]

> > 2001-08-02  Thiemo Seufer <seufer@csv.ica.uni-stuttgart.de>
> > 
> > 	/gas/ChangeLog
> > 	* tc-mips.c (cons_fix_new_mips): Remove.
> > 	* tc-mips.h (TC_CONS_FIX_NEW): Remove.
> > 	(cons_fix_new_mips): Remove.
> 
> If there aren't objections, I'll apply this patch the next days.
> 

Go ahead. 

-eric

-- 
Look out behind you!