This is the mail archive of the binutils@sources.redhat.com 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: [PATCH] x86-64: support newer relocation types


>> --- /home/jbeulich/src/binutils/mainline/2005-06-08/bfd/elf32-i386.c	2005-06-08 14:50:38.000000000 +0200
>> +++ 2005-06-08/bfd/elf32-i386.c	2005-06-08 15:41:20.913778616 +0200
>> @@ -95,7 +95,7 @@ static reloc_howto_type elf_howto_table[
>>    HOWTO(R_386_16, 0, 1, 16, FALSE, 0, complain_overflow_bitfield,
>>  	bfd_elf_generic_reloc, "R_386_16",
>>  	TRUE, 0xffff, 0xffff, FALSE),
>> -  HOWTO(R_386_PC16, 0, 1, 16, TRUE, 0, complain_overflow_bitfield,
>> +  HOWTO(R_386_PC16, 0, 1, 16, TRUE, 0, complain_overflow_signed,
>
>I would preffer to handle this via separate patch (even tought the
>change looks correct to me, see bellow)

This I can certainly do, though it seemed wasted effort to break this out given the identical adjustments made to the respective 64-bit code.

>> +  HOWTO(R_X86_64_PC64, 0, 4, 64, TRUE, 0, complain_overflow_bitfield,
>> +	bfd_elf_generic_reloc, "R_X86_64_PC64", FALSE, MINUS_ONE, MINUS_ONE,
>> +	TRUE),
>
>Well, mine version reads here:
>+   HOWTO(R_X86_64_PC64, 0, 4, 64, TRUE, 0, complain_overflow_signed,
>+ 	bfd_elf_generic_reloc, "R_X86_64_PC64", FALSE, MINUS_ONE, MINUS_ONE,
>+ 	TRUE),
>
>I think Jan's patch is correct in this respect (as unlike for smaller
>relocation times, the address "wrap around"), but please double check.

I'm not sure what to double check here.

>> +	  /* Note that sgot is not involved in this
>> +	     calculation.  We always want the start of .got.plt.  If we
>> +	     defined _GLOBAL_OFFSET_TABLE_ in a different way, as is
>> +	     permitted by the ABI, we might have to change this
>> +	     calculation.  */
>> +	  relocation -= htab->sgotplt->output_section->vma
>> +			+ htab->sgotplt->output_offset;
>> +	  break;
>> +
>> +	case R_X86_64_GOTPC32:
>> +	  /* Use global offset table as symbol value.  */
>> +	  relocation = htab->sgotplt->output_section->vma
>> +		       + htab->sgotplt->output_offset;
>> +	  unresolved_reloc = FALSE;
>> +	  break;
>
>There is another difference relative to my implementation:
>
>+ 	  /* Note that sgot->output_offset is not involved in this
>+ 	     calculation.  We always want the start of .got.  If we
>+ 	     defined _GLOBAL_OFFSET_TABLE in a different way, as is
>+ 	     permitted by the ABI, we might have to change this
>+ 	     calculation.  */
>+ 	  relocation -= htab->sgot->output_section->vma;
>+ 	  break;
>+ 
>+ 	case R_X86_64_GOTPC32:
>+ 	  /* Use global offset table as symbol value.  */
>+ 	  relocation = htab->sgot->output_section->vma;
>+ 	  unresolved_reloc = FALSE;
>+ 	  break;
>+ 

I'm fairly sure the code in the patch is correct: What you want here is the address of the GOT, not the address of the section the GOT is contained in. Of course, in practice GOT will probably never live in a section shared with something else, but who knows...

>I think you need to add matching of GOT relative arithmetics:
>*************** output_disp (insn_start_frag, insn_start
>*** 3336,3353 ****
>  
>  	      p = frag_more (size);
>  	      reloc_type = reloc (size, pcrel, sign, i.reloc[n]);
>! 	      if (reloc_type == BFD_RELOC_32
>! 		  && GOT_symbol
>! 		  && GOT_symbol == i.op[n].disps->X_add_symbol
>! 		  && (i.op[n].disps->X_op == O_symbol
>! 		      || (i.op[n].disps->X_op == O_add
>! 			  && ((symbol_get_value_expression
>! 			       (i.op[n].disps->X_op_symbol)->X_op)
>! 			      == O_subtract))))
>  		{
>  		  offsetT add;
>  
>! 		  if (insn_start_frag == frag_now)
>  		    add = (p - frag_now->fr_literal) - insn_start_off;
>  		  else
>  		    {
>--- 3338,3360 ----
>  
>  	      p = frag_more (size);
>  	      reloc_type = reloc (size, pcrel, sign, i.reloc[n]);
>! 	      if ((reloc_type == BFD_RELOC_32
>! 		   && GOT_symbol
>! 		   && GOT_symbol == i.op[n].disps->X_add_symbol
>! 		   && (i.op[n].disps->X_op == O_symbol
>! 		       || (i.op[n].disps->X_op == O_add
>! 			   && ((symbol_get_value_expression
>! 				(i.op[n].disps->X_op_symbol)->X_op)
>! 			       == O_subtract))))
>! 		  || (reloc_type == BFD_RELOC_32_PCREL
>! 		      && GOT_symbol
>! 		      && GOT_symbol == i.op[n].disps->X_add_symbol))
>  		{
>  		  offsetT add;
>  
>! 		  if (flag_code == CODE_64BIT)
>! 		    add = 0;
>! 		  else if (insn_start_frag == frag_now)
>  		    add = (p - frag_now->fr_literal) - insn_start_off;
>  		  else
>  		    {

If so, this would seem to be an unrelated change, as the GOTPC32 reloc in my patch is handled exactly like the i386 one (which of course is very broken, but obviously cannot be fixed). Or maybe I don't see how you would see BFD_RELOC_32_PCREL to end up here.

>And here the GOT relocation:
>*************** lex_got (reloc, adjust)
>*** 3542,3548 ****
>      { "DTPOFF",   { BFD_RELOC_386_TLS_LDO_32, 0, BFD_RELOC_X86_64_DTPOFF32 } },
>      { "GOTNTPOFF",{ BFD_RELOC_386_TLS_GOTIE,  0, 0                         } },
>      { "INDNTPOFF",{ BFD_RELOC_386_TLS_IE,     0, 0                         } },
>!     { "GOT",      { BFD_RELOC_386_GOT32,      0, BFD_RELOC_X86_64_GOT32    } }
>    };
>    char *cp;
>    unsigned int j;
>--- 3552,3558 ----
>      { "DTPOFF",   { BFD_RELOC_386_TLS_LDO_32, 0, BFD_RELOC_X86_64_DTPOFF32 } },
>      { "GOTNTPOFF",{ BFD_RELOC_386_TLS_GOTIE,  0, 0                         } },
>      { "INDNTPOFF",{ BFD_RELOC_386_TLS_IE,     0, 0                         } },
>!     { "GOT",      { BFD_RELOC_386_GOT32,      0, BFD_RELOC_X86_64_GOT32    } },
>    };
>    char *cp;
>    unsigned int j;

Hmm, this confuses me. All the difference consists in an (ill) comma as far as I can see.

>> @@ -5442,10 +5456,10 @@ tc_gen_reloc (section, fixp)
>>        && GOT_symbol
>>        && fixp->fx_addsy == GOT_symbol)
>>      {
>> -      /* We don't support GOTPC on 64bit targets.  */
>> -      if (flag_code == CODE_64BIT)
>> -	abort ();
>> -      code = BFD_RELOC_386_GOTPC;
>> +      if (flag_code != CODE_64BIT)
>> +	code = BFD_RELOC_386_GOTPC;
>> +      else
>> +	code = BFD_RELOC_X86_64_GOTPC32;
>>      }
>>  
>>    rel = (arelent *) xmalloc (sizeof (arelent));
>And GOT arithmetic here...
>*************** tc_gen_reloc (section, fixp)
>*** 5252,5265 ****
>        break;
>      }
>  
>!   if (code == BFD_RELOC_32
>        && GOT_symbol
>        && fixp->fx_addsy == GOT_symbol)
>      {
>-       /* We don't support GOTPC on 64bit targets.  */
>        if (flag_code == CODE_64BIT)
>! 	abort ();
>!       code = BFD_RELOC_386_GOTPC;
>      }
>  
>    rel = (arelent *) xmalloc (sizeof (arelent));
>--- 5275,5288 ----
>        break;
>      }
>  
>!   if ((code == BFD_RELOC_32 || code == BFD_RELOC_32_PCREL)
>        && GOT_symbol
>        && fixp->fx_addsy == GOT_symbol)
>      {
>        if (flag_code == CODE_64BIT)
>!         code = BFD_RELOC_X86_64_GOTPC32;
>!       else
>!         code = BFD_RELOC_386_GOTPC;
>      }
>  
>    rel = (arelent *) xmalloc (sizeof (arelent));

Yours is the same change as mine (except for the BFD_RELOC_32_PCREL, as above), isn't it?

>Otherwise our patches are almost equivalent (so I believe they are
>correct ;).  I would preffer the tc-i386.c changes  (even if the
>arithmetic syntax can be added incrementally) to go in and someone BFD
>aware to comment on the relocatino handling issues, but otherwise the
>patch would be OK.

Now, how do we go further with this? Who's going to finally approve it?

Jan


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