Re: [PATCH 1/2] MIPS: Compressed PLT/stubs support

[Richard Sandiford <rdsandiford@googlemail.com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>> Maybe something like:
>> 
>>   /* If compressed PLT entries are available, make sure that we use them
>>      for MIPS16 and microMIPS calls.  */
>>   else if ((r_type == R_MIPS16_26 || r_type == R_MICROMIPS_26_S1)
>> 	   && h != NULL
>> 	   && h->use_plt
>> 	   && h->root.plt.plist->comp_offset != MINUS_ONE)
>>     {
>>       sec = htab->splt;
>>       symbol = (sec->output_section->vma
>> 		+ sec->output_offset
>> 		+ htab->plt_header_size
>> 		+ htab->plt_mips_offset
>> 		+ h->root.plt.plist->comp_offset
>> 		+ 1);
>>       target_is_16_bit_code_p = !MICROMIPS_P (abfd);
>>       target_is_micromips_code_p = MICROMIPS_P (abfd);
>>     }
>> 
>> at the end of the:
>> 
>>   /* If this is a reference to a 16-bit function with a stub, we need
>>      to redirect the relocation to the stub unless:
>> 
>> chain of ifs.
>
>  More or less, though I've decided to push it ahead of the chain so that 
> it sees the state consistent regardless of whether any PLT entry processed 
> has been duplicated for dual-mode support.  This probably does not really 
> matter right now (fn_stub/need_fn_stub cases will override any symbol 
> value to use for the relocation and likewise the mode setting anyway, 
> call_stub/call_fp_stub cases will only ever have a standard MIPS PLT entry 
> due to the arrangement in _bfd_mips_elf_adjust_dynamic_symbol and 
> la25_stub cases will never have a PLT entry as these must resolve 
> locally), but I feel a bit uneasy about this half-cooked state.  Let me 
> know if you disagree (and why).

The changes made by the block beginning:

   /* If this is a reference to a 16-bit function with a stub, we need
      to redirect the relocation to the stub unless:

are mutually-exclusive with each other and with the new PLT transformation.
We should only ever perform one.  And the transformation:

  /* If this is a MIPS16 call with a stub, that is made through the PLT or
     to a standard MIPS function, we need to redirect the call to the stub.
     Note that we specifically exclude R_MIPS16_CALL16 from this behavior;
     indirect calls should use an indirect stub instead.  */
  else if (r_type == R_MIPS16_26 && !info->relocatable
	   && ((h != NULL && (h->call_stub != NULL || h->call_fp_stub != NULL))
	       || (local_p
		   && mips_elf_tdata (input_bfd)->local_call_stubs != NULL
		   && mips_elf_tdata (input_bfd)->local_call_stubs[r_symndx] != NULL))
	   && ((h != NULL && h->use_plt_entry) || !target_is_16_bit_code_p))

logically trumps the PLT one.

That's why I think all four transformations should be in a single if chain,
and why the PLT one should come last.

You said yourself about the addition of (h != NULL && h->use_plt_entry):

  The current code flow is a bit subtle, this piece works because as a side 
  effect of the PLT being standard MIPS code the call is qualified as a 
  cross-mode jump.  However this is not really the reason the call needs to 
  be redirected for -- the redirection would have to be done regardless 
  even if we did decide to emit the PLT entry as MIPS16 code for some 
  reason.

That is, if we (redundantly) created both standard and MIPS16 PLT
entries for functions with stubs, and if the "if" statement as you had
it redirected the call from a standard PLT entry to a MIPS16 PLT entry,
this code must explicitly ignore that transformation.  So why do it?
Making a point of doing it ahead of the if-else block, only to explicitly
ignore it in the if-else block, just makes the code more confusing IMO.

Richard