Re: [patch] MIPS: Incorrect calculation for R_MIPS_LO16 relocs

[Richard Sandiford <rsandifo at redhat dot com>]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> On Tue, 29 Jun 2004, Richard Sandiford wrote:
>> > 2. Warning messages for many cases (unfortunately not all, but information
>> > is lost already) when a previously built object is malformed and may
>> > produce a broken binary.  I think this is the case you refer to above,
>> > writing: "[...] it would silently mishandle existing references to
>> > mergeable section symbols + offsets."  It's neither a worse mishandling
>> > than the current one -- the offset will be left intact as for an ordinary
>> > section -- nor actually a silent one.
>> 
>> Well, the current linker correctly handles any LO16 relocations in which
>> the high part of a mergeable section offset is zero.  Even if the LO16
>> relocation isn't paired to a HI16 or GOT16.
>
>  Which of course depends on the actual set of objects used, so a single 
> object may link correctly or not anyway.

I don't see your point.  Mine is that 16-bit offsets from mergeable
section symbols are far more common than non-16-bit offsets.

So yes, of course, if an existing object has non-16-bit offsets, it
probably[*] won't work.  But most objects just have 16-bit offsets,
and those objects correctly under the current scheme.  They won't
work under your patch as posted.

  [*] "probably" because you might get lucky ;)

>> With your patch, orphaned LO16 relocations will take the high bits of
>> the offset from the previous HI16 relocation, whatever that happens to be.
>> That runs a high risk of introducing silent breakage when used with
>> old binaries.
>
>  And gcc 3.4 is out for how long?  Two months.  Do you think all the 
> people rushed out to rebuild all their binaries with a new release?  I 
> don't.

I'm not sure where you get the idea that this is specific to 3.4.
E.g. try compiling:

--------------------------------------------------------------
float g;
extern char big_array[];
void foo (float, char *);
void bar (int n)
{
  float f = 1.0f;
  while (n--)
    foo (1.0f, &big_array[0x20000]);
  g = f;
}
--------------------------------------------------------------

with a 3.3 mips64-elf compiler using "-O2 -G0".  I get the attached code.
The relocation dump is:

--------------------------------------------------------------
0000000c  00000505 R_MIPS_HI16       00000000   .rodata.cst4
0000002c  00000506 R_MIPS_LO16       00000000   .rodata.cst4
00000038  00000506 R_MIPS_LO16       00000000   .rodata.cst4
00000040  00000b04 R_MIPS_26         00000000   foo
00000030  00000a05 R_MIPS_HI16       00000000   big_array
00000044  00000a06 R_MIPS_LO16       00000000   big_array
0000004c  00000506 R_MIPS_LO16       00000000   .rodata.cst4
00000050  00000c05 R_MIPS_HI16       00000004   g
00000054  00000c06 R_MIPS_LO16       00000004   g
--------------------------------------------------------------

Note the placement of the final .rodata.cst4 LO16.

Besides, if we do change the linker, the new behaviour won't appear in
an official release until 2.16.  (I'm assuming the change would be too
invasive for 2.15.1, if such a release is made.)  And by the time 2.16
is out, 3.4 will be fairly old.

>  I think there is a way to deal with it gracefully and still do the right 
> fix.  You worry about the case where there is an orphaned LO16 relocation 
> that happens to work currently, beacuse:
>
> 1. The offset of its corresponding HI16 relocation is zero.
>
> 2. There's another HI16 relocation preceding the relocation and its offset 
> is non-zero.
>
> Is that right?

Yup.

> How about detecting this case and warning the user upon linking?
> I think it can be detected thus:
>
> Given a LO16 relocation:
>
> 1. If the offset of the preceding HI16 relocation is zero, then proceed 
> normally.
>
> 2. If the offset of the preceding HI16 relocation is non zero and the
> symbol referred to is not the one referred to by this relocation, then the
> object was created by broken gas.  Issue a warning.  Optionally assume the
> high part of the offset is zero.
>
> 3. If the offset of the preceding HI16 relocation is non zero and the
> symbol referred to is the same, then proceed normally.  The resulting
> binary would be broken when built with current ld anyway as the
> non-orphaned LO16 relocation corresponding to the preceding HI16 (which
> may or may not be one currently being processed) would not use the high
> part of the offset.  We don't know if the object is good or bad, so no
> warning.  Alternatively, perhaps we may mark good objects somehow and
> issue a warning here for non-marked ones.

This might work, but ugh!  It doesn't appeal to me at all.  Maybe the
maintainers will feel differently though...

>  I've seen something like this:
>
> 	lui	$6,%hi($LC33)
> 	lbu	$6,%lo($LC33+1)($6)
>
> which I can't really see a reasonable explanation for -- for me it's a
> plain bug.

Depends on context really.  What's .LC33?  If it's only byte-aligned,
then yes, it's probably a bug.

Richard

Attachment: foo.s
Description: Text document