mips gas is horribly broken

mips gas is horribly broken

[H . J . Lu]

Around line 9544 in gas/config/tc-mips.c, there are

        if (value != 0 && ! fixP->fx_pcrel)
          {
            /* In this case, the bfd_install_relocation routine will
               incorrectly add the symbol value back in.  We just want
               the addend to appear in the object file.
               FIXME: If this makes VALUE zero, we're toast.  */
            value -= S_GET_VALUE (fixP->fx_addsy);
          }

I spent several days trying to figure out why libstdc++ was miscompiled
on Linux/mipsel. That was because value was zero. That is totally
unacceptable for gas to knowingly generate incorrect binaries. At
least, we should do

            value -= S_GET_VALUE (fixP->fx_addsy);
	    assert (value != 0);

But I'd like to fix it once for all. Does anyone have any suggestions?

Thanks.


H.J.

Re: mips gas is horribly broken

[Geoff Keating]

> Date: Wed, 6 Jun 2001 09:18:46 -0700
> From: "H . J . Lu" <hjl@lucon.org>
> Cc: linux-mips@oss.sgi.com
> Content-Disposition: inline
> User-Agent: Mutt/1.2.5i
> 
> Around line 9544 in gas/config/tc-mips.c, there are
> 
>         if (value != 0 && ! fixP->fx_pcrel)
>           {
>             /* In this case, the bfd_install_relocation routine will
>                incorrectly add the symbol value back in.  We just want
>                the addend to appear in the object file.
>                FIXME: If this makes VALUE zero, we're toast.  */
>             value -= S_GET_VALUE (fixP->fx_addsy);
>           }
> 
> I spent several days trying to figure out why libstdc++ was miscompiled
> on Linux/mipsel. That was because value was zero. That is totally
> unacceptable for gas to knowingly generate incorrect binaries. At
> least, we should do
> 
>             value -= S_GET_VALUE (fixP->fx_addsy);
> 	    assert (value != 0);
> 
> But I'd like to fix it once for all. Does anyone have any suggestions?

There is no easy fix.  This has been a longstanding problem, but any
change to bfd_install_relocation would require modifying every port in
a corresponding way, see for instance the FIXME at line 4816 or so in
tc-ppc.c.

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: mips gas is horribly broken

[H . J . Lu]

On Wed, Jun 06, 2001 at 12:32:15PM -0700, Geoff Keating wrote:
> > Date: Wed, 6 Jun 2001 09:18:46 -0700
> > From: "H . J . Lu" <hjl@lucon.org>
> > Cc: linux-mips@oss.sgi.com
> > Content-Disposition: inline
> > User-Agent: Mutt/1.2.5i
> > 
> > Around line 9544 in gas/config/tc-mips.c, there are
> > 
> >         if (value != 0 && ! fixP->fx_pcrel)
> >           {
> >             /* In this case, the bfd_install_relocation routine will
> >                incorrectly add the symbol value back in.  We just want
> >                the addend to appear in the object file.
> >                FIXME: If this makes VALUE zero, we're toast.  */
> >             value -= S_GET_VALUE (fixP->fx_addsy);
> >           }
> > 
> > I spent several days trying to figure out why libstdc++ was miscompiled
> > on Linux/mipsel. That was because value was zero. That is totally
> > unacceptable for gas to knowingly generate incorrect binaries. At
> > least, we should do
> > 
> >             value -= S_GET_VALUE (fixP->fx_addsy);
> > 	    assert (value != 0);
> > 
> > But I'd like to fix it once for all. Does anyone have any suggestions?
> 
> There is no easy fix.  This has been a longstanding problem, but any
> change to bfd_install_relocation would require modifying every port in

That is not a good excuse to knowingly generate incorrect binaries.

> a corresponding way, see for instance the FIXME at line 4816 or so in
> tc-ppc.c.

I am willing to spend my time to fix it. Do you have any suggestions
how to proceed?


H.J.

Re: mips gas is horribly broken

[Geoff Keating]

> Date: Wed, 6 Jun 2001 13:15:51 -0700
> From: "H . J . Lu" <hjl@lucon.org>
> Cc: binutils@sourceware.cygnus.com, linux-mips@oss.sgi.com
> Content-Disposition: inline
> User-Agent: Mutt/1.2.5i
> 
> On Wed, Jun 06, 2001 at 12:32:15PM -0700, Geoff Keating wrote:
> > > Date: Wed, 6 Jun 2001 09:18:46 -0700
> > > From: "H . J . Lu" <hjl@lucon.org>
> > > Cc: linux-mips@oss.sgi.com
> > > Content-Disposition: inline
> > > User-Agent: Mutt/1.2.5i
> > > 
> > > Around line 9544 in gas/config/tc-mips.c, there are
> > > 
> > >         if (value != 0 && ! fixP->fx_pcrel)
> > >           {
> > >             /* In this case, the bfd_install_relocation routine will
> > >                incorrectly add the symbol value back in.  We just want
> > >                the addend to appear in the object file.
> > >                FIXME: If this makes VALUE zero, we're toast.  */
> > >             value -= S_GET_VALUE (fixP->fx_addsy);
> > >           }
> > > 
> > > I spent several days trying to figure out why libstdc++ was miscompiled
> > > on Linux/mipsel. That was because value was zero. That is totally
> > > unacceptable for gas to knowingly generate incorrect binaries. At
> > > least, we should do
> > > 
> > >             value -= S_GET_VALUE (fixP->fx_addsy);
> > > 	    assert (value != 0);
> > > 
> > > But I'd like to fix it once for all. Does anyone have any suggestions?
> > 
> > There is no easy fix.  This has been a longstanding problem, but any
> > change to bfd_install_relocation would require modifying every port in
> 
> That is not a good excuse to knowingly generate incorrect binaries.
> 
> > a corresponding way, see for instance the FIXME at line 4816 or so in
> > tc-ppc.c.
> 
> I am willing to spend my time to fix it. Do you have any suggestions
> how to proceed?

First, redesign bfd_install_relocation so that it does the right thing
(there are other cases where the bfd backends have to work around
it).  Then, change all the users to match.  Then, test all the
supported platforms.

It might be worthwhile, before starting this, to look and see what
precisely _are_ the supported platforms.  There are things 
in bfd that probably haven't been used for years.  I guess anything
supported by either GDB or by GCC should still be supported, but there
are some more platforms which are supported only in GAS, typically
because it doesn't make sense to have a compiler for them (small 8-bit
parts, for instance).

-- 
- Geoffrey Keating <geoffk@geoffk.org>

Re: mips gas is horribly broken

[H . J . Lu]

On Wed, Jun 06, 2001 at 02:59:19PM -0700, Geoff Keating wrote:
> 
> First, redesign bfd_install_relocation so that it does the right thing
> (there are other cases where the bfd backends have to work around
> it).  Then, change all the users to match.  Then, test all the
> supported platforms.

Could you please tell me what you meant by "the right thing" and what
exactly is wrong with the current design? On mips, it is addend
which is handled incorrectly. Is this the only problem with
bfd_install_relocation?


H.J.

Re: mips gas is horribly broken

[Ian Lance Taylor]

"H . J . Lu" <hjl@lucon.org> writes:

> On Wed, Jun 06, 2001 at 02:59:19PM -0700, Geoff Keating wrote:
> > 
> > First, redesign bfd_install_relocation so that it does the right thing
> > (there are other cases where the bfd backends have to work around
> > it).  Then, change all the users to match.  Then, test all the
> > supported platforms.
> 
> Could you please tell me what you meant by "the right thing" and what
> exactly is wrong with the current design? On mips, it is addend
> which is handled incorrectly. Is this the only problem with
> bfd_install_relocation?

Here is the comment from bfd_install_relocation:

#if 1
/* For m68k-coff, the addend was being subtracted twice during
   relocation with -r.  Removing the line below this comment
   fixes that problem; see PR 2953.

However, Ian wrote the following, regarding removing the line below,
which explains why it is still enabled:  --djm

If you put a patch like that into BFD you need to check all the COFF
linkers.  I am fairly certain that patch will break coff-i386 (e.g.,
SCO); see coff_i386_reloc in coff-i386.c where I worked around the
problem in a different way.  There may very well be a reason that the
code works as it does.

Hmmm.  The first obvious point is that bfd_install_relocation should
not have any tests that depend upon the flavour.  It's seem like
entirely the wrong place for such a thing.  The second obvious point
is that the current code ignores the reloc addend when producing
relocateable output for COFF.  That's peculiar.  In fact, I really
have no idea what the point of the line you want to remove is.

A typical COFF reloc subtracts the old value of the symbol and adds in
the new value to the location in the object file (if it's a pc
relative reloc it adds the difference between the symbol value and the
location).  When relocating we need to preserve that property.

BFD handles this by setting the addend to the negative of the old
value of the symbol.  Unfortunately it handles common symbols in a
non-standard way (it doesn't subtract the old value) but that's a
different story (we can't change it without losing backward
compatibility with old object files) (coff-i386 does subtract the old
value, to be compatible with existing coff-i386 targets, like SCO).

So everything works fine when not producing relocateable output.  When
we are producing relocateable output, logically we should do exactly
what we do when not producing relocateable output.  Therefore, your
patch is correct.  In fact, it should probably always just set
reloc_entry->addend to 0 for all cases, since it is, in fact, going to
add the value into the object file.  This won't hurt the COFF code,
which doesn't use the addend; I'm not sure what it will do to other
formats (the thing to check for would be whether any formats both use
the addend and set partial_inplace).

When I wanted to make coff-i386 produce relocateable output, I ran
into the problem that you are running into: I wanted to remove that
line.  Rather than risk it, I made the coff-i386 relocs use a special
function; it's coff_i386_reloc in coff-i386.c.  The function
specifically adds the addend field into the object file, knowing that
bfd_install_relocation is not going to.  If you remove that line, then
coff-i386.c will wind up adding the addend field in twice.  It's
trivial to fix; it just needs to be done.

The problem with removing the line is just that it may break some
working code.  With BFD it's hard to be sure of anything.  The right
way to deal with this is simply to build and test at least all the
supported COFF targets.  It should be straightforward if time and disk
space consuming.  For each target:
    1) build the linker
    2) generate some executable, and link it using -r (I would
       probably use paranoia.o and link against newlib/libc.a, which
       for all the supported targets would be available in
       /usr/cygnus/progressive/H-host/target/lib/libc.a).
    3) make the change to reloc.c
    4) rebuild the linker
    5) repeat step 2
    6) if the resulting object files are the same, you have at least
       made it no worse
    7) if they are different you have to figure out which version is
       right
*/


Here are the comments I wrote in bfd/doc/bfdint.texi:

The assembler used @samp{bfd_perform_relocation} for a while.  This
turned out to be the wrong thing to do, since
@samp{bfd_perform_relocation} was written to handle relocations on an
existing object file, while the assembler needed to create relocations
in a new object file.  The assembler was changed to use the new function
@samp{bfd_install_relocation} instead, and @samp{bfd_install_relocation}
was created as a copy of @samp{bfd_perform_relocation}.

Unfortunately, the work did not progress any farther, so
@samp{bfd_install_relocation} remains a simple copy of
@samp{bfd_perform_relocation}, with all the odd special cases and
confusing code.  This again is difficult to change, because again any
change can affect any assembler target, and so is difficult to test.

Ian

Re: mips gas is horribly broken

[Geoff Keating]

> Date: Wed, 6 Jun 2001 17:02:21 -0700
> From: "H . J . Lu" <hjl@lucon.org>
> Cc: binutils@sourceware.cygnus.com, linux-mips@oss.sgi.com
> Content-Disposition: inline
> User-Agent: Mutt/1.2.5i
> 
> On Wed, Jun 06, 2001 at 02:59:19PM -0700, Geoff Keating wrote:
> > 
> > First, redesign bfd_install_relocation so that it does the right thing
> > (there are other cases where the bfd backends have to work around
> > it).  Then, change all the users to match.  Then, test all the
> > supported platforms.
> 
> Could you please tell me what you meant by "the right thing" and what
> exactly is wrong with the current design? On mips, it is addend
> which is handled incorrectly. Is this the only problem with
> bfd_install_relocation?

There are four occurrences of 'FIXME', and one comment starting 'Wait
for the day'.  It would be nice to look at them all...

The particular problem you're having is possibly this line of code:

#if 1
/* For m68k-coff, the addend was being subtracted twice during
   relocation with -r.  Removing the line below this comment
   fixes that problem; see PR 2953.

However, Ian wrote the following, regarding removing the line below,
which explains why it is still enabled:  --djm

If you put a patch like that into BFD you need to check all the COFF
linkers.  I am fairly certain that patch will break coff-i386 (e.g.,
SCO); see coff_i386_reloc in coff-i386.c where I worked around the
problem in a different way.  There may very well be a reason that the
code works as it does.

Hmmm.  The first obvious point is that bfd_install_relocation should
not have any tests that depend upon the flavour.  It's seem like
entirely the wrong place for such a thing.  The second obvious point
is that the current code ignores the reloc addend when producing
relocateable output for COFF.  That's peculiar.  In fact, I really
have no idea what the point of the line you want to remove is.

A typical COFF reloc subtracts the old value of the symbol and adds in
the new value to the location in the object file (if it's a pc
relative reloc it adds the difference between the symbol value and the
location).  When relocating we need to preserve that property.

BFD handles this by setting the addend to the negative of the old
value of the symbol.  Unfortunately it handles common symbols in a
non-standard way (it doesn't subtract the old value) but that's a
different story (we can't change it without losing backward
compatibility with old object files) (coff-i386 does subtract the old
value, to be compatible with existing coff-i386 targets, like SCO).

So everything works fine when not producing relocateable output.  When
we are producing relocateable output, logically we should do exactly
what we do when not producing relocateable output.  Therefore, your
patch is correct.  In fact, it should probably always just set
reloc_entry->addend to 0 for all cases, since it is, in fact, going to
add the value into the object file.  This won't hurt the COFF code,
which doesn't use the addend; I'm not sure what it will do to other
formats (the thing to check for would be whether any formats both use
the addend and set partial_inplace).

When I wanted to make coff-i386 produce relocateable output, I ran
into the problem that you are running into: I wanted to remove that
line.  Rather than risk it, I made the coff-i386 relocs use a special
function; it's coff_i386_reloc in coff-i386.c.  The function
specifically adds the addend field into the object file, knowing that
bfd_install_relocation is not going to.  If you remove that line, then
coff-i386.c will wind up adding the addend field in twice.  It's
trivial to fix; it just needs to be done.

The problem with removing the line is just that it may break some
working code.  With BFD it's hard to be sure of anything.  The right
way to deal with this is simply to build and test at least all the
supported COFF targets.  It should be straightforward if time and disk
space consuming.  For each target:
    1) build the linker
    2) generate some executable, and link it using -r (I would
       probably use paranoia.o and link against newlib/libc.a, which
       for all the supported targets would be available in
       /usr/cygnus/progressive/H-host/target/lib/libc.a).
    3) make the change to reloc.c
    4) rebuild the linker
    5) repeat step 2
    6) if the resulting object files are the same, you have at least
       made it no worse
    7) if they are different you have to figure out which version is
       right
*/
	  relocation -= reloc_entry->addend;
#endif


but I might have misread the code.  Or the comment.  There's certainly
much more of the comment to misread :-).

-- 
- Geoffrey Keating <geoffk@geoffk.org>