Re: MIPS embedded PIC bug.

[cgd at sibyte dot com (Chris G. Demetriou)]

First off, I'd like to thank you for responding to my message in a
timely manner.  8-)


Geoff Keating <geoffk@geoffk.org> writes:
> > Code for MIPS embedded pic switches looks like:
> > 
> > $LS20:
> > 	...
> >         lw      $3,$L20-$LS20($2)	# can be 'ld' for 64-bit GPRs.
> > 	...
> > $L20:
> > 	... table ...
> > 
> > Where $LS20 is in the current section, but $L20 is undefined.  The
> > change I made to the embedded PIC hack broke that.
> 
> How can $L20 be undefined?  It is defined by the line

It's undefined at the time the 'lw' is assembled.  Yes, it'll be fixed
up later, typically.  (My patch was broken in this regard.)


> In fact, the code would still be valid even if $L20 was never defined,
> because (IIRC) the right kind of 16-bit PC-relative reloc is available.

Really?  My understanding of this says that if it can't be resolved
into something PC-relative, you need to emit more complex code, and a
sample like the above, but with L20 not defined in the same segment,
can't be resolved into something PC-relative with a single
instruction.

The embedded PIC code (before I got to it 8-) assumed that as long as
LS20 is in the current segment, you can do that (regardless of where
L20 is defined).


> > The problem is, with the embedded PIC hack as general as it was,
> > is that several cases in the MIPS 'empic' testcase (before I got to
> > it, honest!!! 8-) would _fail_ because of the excessive generality of
> > the test.  It was letting references to undefined symbols through,
> > and gas couldn't properly emit relocations for them.  (To demonstrate
> > this, unapply the first patch mentioned above, and make a mips-elf
> > toolchain, and look at the errors generated by the MIPS empic test.)
> 
> The test didn't fail when I wrote it, did something break since and
> no-one fix it?

Hmm.  Looks like it went in on 2000-03-10 (according to the
ChangeLogs.

I just did a checkout of binutils using date 2000-03-15 (mmm, the Ides
of March 8-), using the command:

cvs -z 9 -d :pserver:anoncvs@anoncvs.cygnus.com:/cvs/src co -D"2000-03-15" binutils

I then compiled it up for mips-elf.  It fails in exactly the same way
as I'd seen all along:

../as-new  -membedded-pic -mips3 -o dump.o
/home/cgd/reloc-bug/src/gas/testsuite
/gas/mips/empic.s
/home/cgd/reloc-bug/src/gas/testsuite/gas/mips/empic.s: Assembler
messages:
/home/cgd/reloc-bug/src/gas/testsuite/gas/mips/empic.s:21: Error: Cannot make BFD_RELOC_LO16 relocation PC relative
/home/cgd/reloc-bug/src/gas/testsuite/gas/mips/empic.s:23: Error: Cannot make BFD_RELOC_LO16 relocation PC relative
/home/cgd/reloc-bug/src/gas/testsuite/gas/mips/empic.s:68: Error: Cannot make BFD_RELOC_LO16 relocation PC relative
/home/cgd/reloc-bug/src/gas/testsuite/gas/mips/empic.s:70: Error: Cannot make BFD_RELOC_LO16 relocation PC relative
[ ... ]

I've not gone to the trouble of inspecting diffs betwen the exact
moments your change went in and 2000-03-15, but the ChangeLogs don't
show anything interesting that i'm seeing.


A bit more discussion:

Those testsuite errors are for instructions (really in this case
pseudo-ops) like:

        la      $3,g1-l3        # R_MIPS_GNU_REL_HI16   g1   0
                                # R_MIPS_GNU_REL_LO16   g1   C
        la      $3,l1-l3        # R_MIPS_GNU_REL_HI16   .foo 0
                                # R_MIPS_GNU_REL_LO16   .foo 114

(lines 21 and 23).  g1 is not defined in the file, l1 is defined in a
different section later in the file, and l3 is defined earlier in the
currently-assembling section.

Your 'o' operand code change checks for:

              if (c == 0
                  && (offset_expr.X_op != O_constant
                      || offset_expr.X_add_number >= 0x8000
                      || offset_expr.X_add_number < -0x8000)  
                  && (mips_pic != EMBEDDED_PIC
                      || offset_expr.X_op != O_subtract
                      || (S_GET_SEGMENT (offset_expr.X_op_symbol)
                          != now_seg)))
                break;

(circa line 7817 in gas/config/tc-mips.c of that date).

'break' indicates that arguments don't match.  If you do 'break' here
in this case for the instructions that fail the testcase, they're
handled properly: you get the REL_LO16 and REL_HI16 relocs, etc.

No break indicates this can be handled as a BFD_RELOC_LO16.  That
can't be converted into something PC-relative unless it happens to
resolve into a constant by the symbol being defined in the same
section, later in the file.

If you assume that the compiler's generating the code, you hit that
perfectly: the undefined X_add_symbol will be defined very shortly in
the current segment, so if you emit the RELOC_LO16 you win and end up
generating a single instruction.

I don't know what you'd do to handle the case properly, without any
consession to the compiler.  If you simply nuke the EMBEDDED_PIC test,
then you end up with:

	"Error: expression too complex"

in cases of -membedded-pic switches.  (That was the error in our local
compiler that my coworker reported, that caused me to reexamine this
issue.)  It could very well be that the only sane solution is to have
the compiler generate an la (which can be made pc-relative in a
generic way) followed by an lw...  but that's wasteful.



> Remember, people are permitted to write their own assembler, and many
> do.  The assembler doesn't just handle compiler-generated output.  The
> testcase was designed to try all the cases.

yes, of course!

By checking for specific opcodes, I was trying to tune the code that
existed before I got to it, so that it would properly assemble the
(seemingly hand-generated) testcase!  8-)



chris