Re: [PATCH] MIPS/GAS: Correct the handling of %hi/%lo with subtraction

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Sun, 23 Sep 2012, Richard Sandiford wrote:

> >  Issues #1-3 are handled by the patch below.  I have removed any range 
> > checks for the LO16 relocation and added further code to handle the MIPS16 
> > case right.  I have added complementing code for HI16 relocations, that is 
> > similar to that for LO16, but I've decided not to obfuscate it too much 
> > and didn't factor it out.
> 
> I'm not buying that :-)  The relocation value we want to install
> doesn't depend on the current contents of the instruction, so I don't
> think that:
> 
>    install_reloc (...(*valP + 0x8000) >> 16...);
> 
> (for hi) and:
> 
>    install_reloc (...*valP...);
> 
> (for lo) is any less obfuscated than cut-&-pasting all the insn reading
> and writing bits.
> 
> IMO, the problem with the current code is lack of factoring.  It's no
> harder to resolve relocations at md_apply_fix time than at md_assemble
> time, but at the moment they use two separate pieces of code.
> If we use a common routine for both then...

 Fair enough, thanks for doing the grunt work!

> >  Issue #5 is fixed simply by checking for the condition and reporting an 
> > error -- GAS shouldn't silently produce wrong code, but such usage is 
> > currently unknown.  It may well be that it'll trigger somewhere and some 
> > further cases may have to be implemented.  I have deliberately disregarded 
> > the cases of HIGHER and HIGHEST relocs that are relevant to the original 
> > scenario reported in the issue where the n64 ABI is used -- for them to 
> > have any significance the distance between the symbols would have to be 
> > greater than 2GB and 128TB respectively.  I have therefore concluded that 
> > implementing any handling would be a wasted effort at this time.  Sent 
> > separately.
> 
> ...the HIGHER and HIGHEST bits come out in the wash.

 Yeah, well...

> As with the jump relocs discussed in the other thread, we could simply
> set fx_done back to 0 for other relocs on RELA targets, but REL would
> again require more care.  E.g. %got_hi would need the high part of the
> constant to be installed in-place; simply setting fx_done to 0
> and relying on the generic overflow check would produce wrong results
> for that kind of reloc.  So here too I agree it's best to report
> an error across the board.

 What would be the point -- how is a GOT entry corresponding to constant 
zero defined in the ABI?  I think this is plain broken assembly, not 
something we could handle anyhow other than issuing an error.

> I ended up adding a separate test for TLS relocs against constants,
> since at the moment they trigger a segfault on:
> 
>       S_SET_THREAD_LOCAL (fixP->fx_addsy);

 I missed that, sorry.

 We're still having an issue in some places where more obscure constant 
expressions are used as an argument to macros or real instructions (as in 
LI vs ADDIU) where no percent-op is involved, that is the result may, 
though does not have to extend beyond the 16-bit range of the respective 
instructions (i.e signed or unsigned as in ADDIU vs XORI), up to 64 bits.  

 Owing to the lack of time I've deliberately refrained from addressing 
that on this occasion, however I'll try to keep this in mind and come up 
with a proposal later on unless you beat me to it.

  Maciej