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.

I am pretty convinced that bitfield is correct here now.
> 
> >> +	  /* 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 guess that explains why both versions are working and why some other
backends are using my version of code too.
> 
> 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.

i386 GOT relocation is different from GOTPC32 in a way that one is IP
relative and other not.
I386 way of encoding GOT relocation is:
        addl    $_GLOBAL_OFFSET_TABLE_+[.-.L2], %ebx
While for x86-64 we want something like:
	leaq    _GLOBAL_OFFSET_TABLE_(%rip), %rbx
So the expression to match needs tobe slightly different.
> 
> >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.

I used to have there GOTOFF64 keyword but then replaced it by GOTOFF,
but forgot the comma in, sorry.
> 
> >> @@ -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?

Yes, but I think we need BFD_RELOC_32_PCREL for the PCrelative encoding
above.
> 
> Now, how do we go further with this? Who's going to finally approve it?

While in theory I can approve x86-64 patches, in BFD area I don't think
I am competent to, so I would preffer if some other maintainer looked
into it or at least didn't complained for some time.

Honza
> 
> Jan


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