This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFC: Adding non-PIC executable support to MIPS


Daniel Jacobowitz <dan@debian.org> writes:
> On Sun, Jul 27, 2008 at 10:10:23AM +0100, Richard Sandiford wrote:
>> Daniel Jacobowitz <dan@debian.org> writes:
>> > All comments welcome - Richard, especially from you.  How would you
>> > like to proceed?  I think the first step should be to get your other
>> > binutils/gcc patches merged, including MIPS16 PIC; I used those as a
>> > base.  But see a few of the notes for potential problems with those
>> > patches.
>> 
>> Yeah, Nick's approved most of the remaining binutils changes (thanks).
>> I haven't applied them yet because of the doubt over whether st_size
>> should be even or odd for ISA-encoded MIPS16 symbols.  I don't really
>> have an opinion, so I'll accept a maintainerly decision...
>
> In that case, I suggest we go with even sizes.

OK.  I'll take that as maintainer approval and commit it later.

>> I think the gcc side is good to go, modulo the _mcount thing.
>
> Ok.  Want me to find a fix, or will you?

I'll have a go.

>> As far as binutils goes, I saw a couple of potential problems:
>> 
>> (1) The patch adds the following code to
>>     _bfd_mips_elf_create_dynamic_sections:
>> 
>> +      if (htab->use_plts_and_copy_relocs && htab->root.hplt == NULL)
>> +	{
>> +	  h = _bfd_elf_define_linkage_sym (abfd, info, s,
>> +					   "_PROCEDURE_LINKAGE_TABLE_");
>> +	  htab->root.hplt = h;
>> +	  if (h == NULL)
>> +	    return FALSE;
>> +	  h->type = STT_FUNC;
>> +	}
>> 
>>     But use_plts_and_copy_relocs is only set after all input bfds have
>>     been read in.
>
> Whoops.  I've had recurring problems with this - there were several
> other things our previous patch did based on the first time a pic0
> input was seen and this was one of them.  I had in my notes that
> the _P_L_T_ symbol was going to be tricky to get right and I just just
> drop it.  But obviously I didn't.
>
> I'll define the symbol later.  This is straightforward, as nothing (in
> non-VxWorks anyway) refers to the symbol.

OK.

>> @@ -7432,9 +7484,18 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
>>  		elf_hash_table (info)->dynobj = dynobj = abfd;
>>  	      break;
>>  	    }
>> -	  /* Fall through  */
>> +	  /* Fall through.  */
>>  
>>  	default:
>> +	  /* Most static relocations require pointer equality, except
>> +	     for branches.  */
>> +	  if (h)
>> +	    h->pointer_equality_needed = TRUE;
>> +	  /* Fall through.  */
>> +
>> +	case R_MIPS_26:
>> +	case R_MIPS_PC16:
>> +	case R_MIPS16_26:
>>  	  if (h)
>>  	    ((struct mips_elf_link_hash_entry *) h)->has_static_relocs = TRUE;
>>  	  break;
>>
>>     But pointer equality is needed for non-call GOT relocations too.
>>     I couldn't see anything that explicitly handled that case.
>
> I'm not sure about this.  While pointer equality, the concept, is
> needed, I don't think it mandates a valid address on the PLT entry
> (which is what this flag controls).  Won't the dynamic linker do the
> right thing?

Sigh.  You're right of course: I was simply going by the name
of the variable, without considering what it actually controlled.

("pointer_equality_needed" seems an unfortunate choice of name for
something that, with optimisation in mind, we specifically want to
be false in some cases where pointer equality is needed.  Not your
problem though. ;))

>> I think it'd be good to tighten the assert, as it would currently trigger
>> for valid X >= -0x80000000 addresses (both in 32-bit and 64-bit code).
>> I don't know of any system out there that allows dynamic executables
>> in kernel space, but I've ceased to be surprised by such things ;)
>
> OK.  Could you check that I got this right?
>
>   /* The PLT sequence is not safe for N64 if .got.plt's address can
>      not be loaded in two instructions.  */
>   BFD_ASSERT ((gotplt_value & ~(bfd_vma) 0x7fffffff) == 0
>               || ~(gotplt_value | 0x7fffffff) == 0);

Looks right if we can rely on wider-than-necessary bfd_vmas being
sign-extended, but I don't know off-hand if we can.  (If we can, then:

    BFD_ASSERT (((gotplt_value + 0x80000000) >> 31) < 2);

is a little shorter.  Someone's probably going to tell me that it's
also wrong.)

>> Please keep the "... <...>:" addresses at least.  (You did this
>> in some of the other tests, thanks.)  The non-disassembly dumps
>> rely on functions being at certain addresses, and I think it's
>> easier to follow the test if those addresses are explicit in
>> the disassembly.
>> 
>> (I've used custom scripts for this sort of test to try to
>> protect them from fluctuations in the size of the dynamic info.
>> The addresses should be pretty stable.)
>
> They're a pain to update when something does change; I spent most of
> an entire day fixing hardcoded addresses in the test scripts, and like
> Ian, I'm starting to wonder about the value.

FWIW, I think these sorts of test are useful.  When used right, they can
reduce the risk of depending on undocumented behaviour.  (Like: things
that run OK due to an accident in the ld.so implementation.)

The situation you were in was unusual: you were making incompatible
changes to the ABI under test.  The tests are really there to make
sure that we don't accidentally do that again in future. ;)
So in way, a high level of pain is reassuring.  (It's easy to say
that when you're not doing the work.)

And yeah, I've found it painful updating things that use the default
linker script, often due to trivial differences like an empty section
being removed.  But using custom scripts should avoid some of that.

E.g. one of the things I (and I think you) were trying to do was
to change "normal" objects as little as possible.  I think the
pre-existing tests helped to reduce the amount of new-PIC gunk in
old objects.  And QoI things like GOT page estimation are hard to
test without some level of hard-coding.

> I put the addresses back for symbols, which sounds like a fine
> compromise.

Thanks.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]