This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
MIPS embedded PIC bug.
- To: binutils at sourceware dot cygnus dot com
- Subject: MIPS embedded PIC bug.
- From: cgd at sibyte dot com (Chris G. Demetriou)
- Date: 07 Feb 2001 17:45:48 -0800
- Cc: geoffk at sourceware dot cygnus dot com, nickc at sourceware dot cygnus dot com
Hi,
It's come to my attention (internal problem report, and subsequent
debugging), that the patches:
http://sources.redhat.com/ml/binutils/2000-10/msg00042.html
http://sources.redhat.com/ml/binutils/2000-10/msg00043.html
are, in fact, incorrect. (Only a portion of the latter patch is
incorrect, though, and I don't completely understand _why_ the
expected value is, in fact, to be expected.) I'm kinda surprised that
nobody noticed -- must be ~nobody using -membedded-pic in the current
tools.
The first patch changed the insn argument 'o' format code to modify
the embedded-pic hack (in a way that I thought was correct, but is
not).
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.
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.)
To my mind, the solution to the problem is to make the empic hack test
more specific. Since this is a workaround for a compiler generated
code sequence which otherwise wouldn't be legal, it seems reasonable
to actually test for the specific instructions that the compiler will
emit, when enabling the workaround.
The second patch incorrectly changed the expectation of relocations
emitted by the compiler. When the first patch and the second patches
were applied, the empic test would fail. With the first patch
corrected as described above, the new empic tests again fails. I
don't _understand_ why the old output is correct -- I'd think the
relocations in question could be reduced to a constant difference.In
particular, the code in question is:
l5:
...
2:
...
la $3,2b-l5 # R_MIPS_GNU_REL_HI16 .text 0
# R_MIPS_GNU_REL_LO16 .text 10C
la $3,2b+8-l5 # R_MIPS_GNU_REL_HI16 .text 0
# R_MIPS_GNU_REL_LO16 .text 11C
Do those have to be output as relocations because of instruction
removal (relaxation) done at link time? or is it just that for some
reason the assembler isn't smart enough to fold them into constants?
The patch that I am using is below. All in all, it:
(1) backs out the non-comment-fix changes to empic.s and empic.d (and
fixes the comments in the previously-deleted chunk),
(2) adds in explicit tests for "lw" and "ld" to the empic hack in
mips_ip(), and
(3) cleans up the code that decides whether the 'o' format is usable,
so that it's significantly more readable. When I had to resort to pencil
and paper to figure out how to add the opcode checks to the existing
logic, I figured that it might be good to clean it up.
The diff is long, but the majority of that is moving & breaking up a
comment and breaking up a big expression. To my mind, in terms of
copyright issues, this is a trivial patch. That having been said...
Unfortunately, my employer copyright disclaimer status is kinda fuzzy.
I had a (future) disclaimer on file from my employer, but then we got
bought. I'm in the process of working with the people who bought us
to sort out a new disclaimer. If that's an issue for accepting this
patch, I'll be glad to keep y'all posted.
passes all tests with mips-elf, and in addition allows successful
compilation of embedded pic code.
*sigh* Sorry 'bout that!
chris
======================================================================
for gas/ChangeLog:
2001-02-07 Chris Demetriou <cgd@broadcom.com>
* config/tc-mips.c (mips_ip): In the 'o'-format operand case,
revert the embedded PIC functionality to the way it worked
prior to the change on 2000-12-01. Adjust it to more
carefully check the instructions to which it will be applied.
Clean up that code to make it more readable.
for gas/testsuite/ChangeLog:
2001-02-07 Chris Demetriou <cgd@broadcom.com>
* gas/mips/empic.d: Revert changes made on 2000-12-01, keeping
comment corrections.
* gas/mips/empic.s: Likewise.
Index: config/tc-mips.c
===================================================================
RCS file: /cvsroot/systemsw/tools/src/binutils/gas/config/tc-mips.c,v
retrieving revision 1.13
diff -c -r1.13 tc-mips.c
*** tc-mips.c 2000/11/10 18:34:49 1.13
--- tc-mips.c 2001/02/08 01:12:28
***************
*** 8025,8050 ****
case 'o': /* 16 bit offset */
c = my_getSmallExpression (&offset_expr, s);
! /* If this value won't fit into a 16 bit offset, then go
! find a macro that will generate the 32 bit offset
! code pattern. As a special hack, we accept the
! difference of two local symbols as a constant. This
! is required to suppose embedded PIC switches, which
! use an instruction which looks like
! lw $4,$L12-$LS12($4)
! The problem with handling this in a more general
! fashion is that the macro function doesn't expect to
! see anything which can be handled in a single
! constant instruction. */
! 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_add_symbol)
! != S_GET_SEGMENT (offset_expr.X_op_symbol))))
! break;
if (c == 'h' || c == 'H')
{
--- 8025,8064 ----
case 'o': /* 16 bit offset */
c = my_getSmallExpression (&offset_expr, s);
! if (c == 0)
! {
! int ok_constant;
! int ok_empic_hack;
!
! /* Constant values are OK if they fit into a 16-bit offset.
! If not, we have to find a macro that will generate a 32
! bit offset code pattern. */
! ok_constant = (offset_expr.X_op == O_constant
! && offset_expr.X_add_number >= -0x8000
! && offset_expr.X_add_number < 0x8000);
!
! /* As a special hack, if the instruction is an "lw", we
! accept the difference between an undefined symbol and
! a symbol defined in the current segment. This is
! done to support embedded PIC switches, which use
! instructions like:
! lw $4,$L12-$LS12($4)
! The problem with handling this in a more general
! fashion is that the macro function doesn't expect to
! see anything which can be handled in a single
! constant instruction. */
! ok_empic_hack = (mips_pic == EMBEDDED_PIC
! && (strcmp(str, "lw") == 0
! || strcmp(str, "ld") == 0)
! && offset_expr.X_op == O_subtract
! && (S_GET_SEGMENT (offset_expr.X_op_symbol)
! == now_seg)
! && (S_GET_SEGMENT (offset_expr.X_add_symbol)
! == undefined_section));
!
! if (!ok_constant && !ok_empic_hack)
! break;
! }
if (c == 'h' || c == 'H')
{
Index: testsuite/gas/mips/empic.d
===================================================================
RCS file: /cvsroot/systemsw/tools/src/binutils/gas/testsuite/gas/mips/empic.d,v
retrieving revision 1.4
diff -c -r1.4 empic.d
*** empic.d 2000/10/20 17:26:25 1.4
--- empic.d 2001/02/08 01:01:18
***************
*** 55,62 ****
0+00000b8 R_MIPS_64 \.text
0+00000cc R_MIPS_GNU_REL16_S2 \.text
0+00000d0 R_MIPS_GNU_REL16_S2 \.text
! 0+00000dc R_MIPS_32 \.text
! 0+00000e8 R_MIPS_64 \.text
RELOCATION RECORDS FOR \[\.foo\]:
--- 55,66 ----
0+00000b8 R_MIPS_64 \.text
0+00000cc R_MIPS_GNU_REL16_S2 \.text
0+00000d0 R_MIPS_GNU_REL16_S2 \.text
! 0+00000d4 R_MIPS_GNU_REL_HI16 \.text
! 0+00000d8 R_MIPS_GNU_REL_LO16 \.text
! 0+00000dc R_MIPS_GNU_REL_HI16 \.text
! 0+00000e0 R_MIPS_GNU_REL_LO16 \.text
! 0+00000e4 R_MIPS_32 \.text
! 0+00000f0 R_MIPS_64 \.text
RELOCATION RECORDS FOR \[\.foo\]:
***************
*** 122,130 ****
00a0 3c030000 [26]46300d8 3c030000 [26]46300e8 .*
00b0 000000cc 00000034 00000000 000000cc .*
00c0 00000000 00000034 00000000 10000032 .*
! 00d0 10000033 24030034 2403003c 000000cc .*
! 00e0 00000034 00000000 00000000 000000cc .*
! 00f0 00000000 00000034 00000000 00000000 .*
Contents of section \.data:
Contents of section \.reginfo:
0000 80000008 00000000 00000000 00000000 .*
--- 126,134 ----
00a0 3c030000 [26]46300d8 3c030000 [26]46300e8 .*
00b0 000000cc 00000034 00000000 000000cc .*
00c0 00000000 00000034 00000000 10000032 .*
! 00d0 10000033 3c030000 [26]463010c 3c030000 .*
! 00e0 [26]463011c 000000cc 00000034 00000000 .*
! 00f0 00000000 000000cc 00000000 00000034 .*
Contents of section \.data:
Contents of section \.reginfo:
0000 80000008 00000000 00000000 00000000 .*
Index: testsuite/gas/mips/empic.s
===================================================================
RCS file: /cvsroot/systemsw/tools/src/binutils/gas/testsuite/gas/mips/empic.s,v
retrieving revision 1.4
diff -c -r1.4 empic.s
*** empic.s 2000/10/20 17:26:25 1.4
--- empic.s 2001/02/08 01:01:18
***************
*** 52,59 ****
2: # at address 0xCC.
b 2b # R_MIPS_GNU_REL16_S2 .text 32
b 2b+4 # R_MIPS_GNU_REL16_S2 .text 33
! la $3,2b-l5 # 34
! la $3,2b+8-l5 # 3C
.word 2b # R_MIPS_32 .text CC
.word 2b-l5 # R_MIPS_PC32 .text 11C or 34
nop
--- 52,61 ----
2: # at address 0xCC.
b 2b # R_MIPS_GNU_REL16_S2 .text 32
b 2b+4 # R_MIPS_GNU_REL16_S2 .text 33
! la $3,2b-l5 # R_MIPS_GNU_REL_HI16 .text 0
! # R_MIPS_GNU_REL_LO16 .text 10C
! la $3,2b+8-l5 # R_MIPS_GNU_REL_HI16 .text 0
! # R_MIPS_GNU_REL_LO16 .text 11C
.word 2b # R_MIPS_32 .text CC
.word 2b-l5 # R_MIPS_PC32 .text 11C or 34
nop