Bug 18759

The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=dbac553d28887561e3f154654ec8e70195d89943

commit dbac553d28887561e3f154654ec8e70195d89943
Author: Peter Zotov <whitequark@whitequark.org>
Date:   Tue Aug 11 17:12:21 2015 +0100

    Fix encoding or OpenRisk1000 PC relative relocations.
    
    	PR ld/18759
    	* elf32-or1k.c (R_OR1K_32_PCREL): Set pcrel_offset to TRUE.
    	(R_OR1K_16_PCREL): Likewise.
    	(R_OR1K_8_PCREL): Likewise.

Hi Peter,

  Thanks for the bug report and patch.  I have now checked your fix in.

Cheers
  Nick

This patch wasn't tested.  It causes FAIL: ld-elf/merge.  By the look of or1k gas does not producing addends suited to pcrel relocs with pcrel_offset true.

I will take a look at this soon.

I have looked at this and it is not a bug I've introduced. The test xfails or32, which is how or1k used to be named. The test was not updated, which has in fact hidden this very bug. And I think ld-elf/eh-frame-hdr currently erroneously xfails or1k, which is another reason this bug was not visible.

ld-elf/merge should be updated to xfail or1k, and ld-elf/eh-frame-hdr should be updated to not xfail or1k.

The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9b57267f4ffa4f8a168f89630a4b68fb51a419de

commit 9b57267f4ffa4f8a168f89630a4b68fb51a419de
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Aug 12 19:07:26 2015 +0930

    Revert "Fix encoding or OpenRisk1000 PC relative relocations."
    
    This reverts commit dbac553d28887561e3f154654ec8e70195d89943.
    
    	PR ld/18759
    	* elf32-or1k.c: Revert 2015-08-11 change.

Why did you revert this?

The test being disabled for or32 is not relevant.  The test can fail for all sorts of reasons, most commonly that string merge is not supported by a target.  What is relevant is that the test exercises R_OR1K_32_PCREL relocations, and produces an incorrect binary after your patch.  Clearly, your patch is responsible so I have reverted it.

Note that changing pcrel_offset is an ABI change.  It is quite possible to define pcrelative relocations such that pcrel_offset should be false (but this would be unusual for ELF).  So without looking at the ABI I can't say whether your patch is wrong or just lacks a gas change.

I have confirmed that, at least, gas emits PC-relative relocations incorrectly. I will fix that.

Speaking of the ABI: OR1K does not have a document (any document) which specifies the R_OR1K_*_PCREL relocations. In fact, I believe that binutils is the only software that implements them.

Ok, I took a shot at fixing gas today. To recap...

Here is the minimal testcase:

  .section .data1
     .long 0
  x: .long 0

  .section .data2
     .long 0
     .long 0
  y: .long (x-.)

To compile:

  $ llvm-mc -triple=or1k test.s -filetype=obj -o mc.o
  $ ./gas/as-new test.s -o as.o

We can now examine the relocations.

  $ ./binutils/objdump -r mc.o
  RELOCATION RECORDS FOR [.data2]:
  OFFSET   TYPE              VALUE 
  00000008 R_OR1K_32_PCREL   .data1+0x00000004

  $ ./binutils/objdump -r as.o 
  RELOCATION RECORDS FOR [.data2]:
  OFFSET   TYPE              VALUE 
  00000008 R_OR1K_32_PCREL   .data1-0x00000004

LLVM emits the correct value: the address of the `x` symbol. gas does not: it emits the address of the `x` symbol minus the offset of `y` in .data2. That is, it emits the relocation relative to the start of the section, as if pcrel_offset was FALSE.

We can link the files and confirm that this is inconsistent with what ld does:

  $ ./ld/ld-new -shared mc.o -o mc.so
  $ ./ld/ld-new -shared as.o -o as.so
  
  $ ./binutils/objdump -s -j .data1 -j .data2 mc.so
  Contents of section .data1:
   216c 00000000 00000000                    ........        
  Contents of section .data2:
   0000 00000000 00000000 00002168           ..........!h    
  
  $ ./binutils/objdump -s -j .data1 -j .data2 as.so
  Contents of section .data1:
   2180 00000000 00000000                    ........        
  Contents of section .data2:
   0000 00000000 00000000 00002174           ..........!t    

The place where gas decides the addend to emit is in tc_gen_reloc in tc-or1k.c. Currently, it delegates to gas_cgen_tc_gen_reloc (which is used in several other backends, none of which passes the ld-elf/merge test).

gas_cgen_tc_gen_reloc uses fixP->fx_addnumber as the addend in all cases except when emitting vtable relocations, in which case it uses fixP->fx_offset. fixP->fx_addnumber is a scratch field which in case of cgen is set to *valP in gas_cgen_md_apply_fix. *valP=-4, which comes from fixup_segment in write.c:978:

	  else if (sub_symbol_segment == this_segment
		   && !S_FORCE_RELOC (fixP->fx_subsy, 0)
		   && !TC_FORCE_RELOCATION_SUB_LOCAL (fixP, add_symbol_segment))
	    {
	      add_number -= S_GET_VALUE (fixP->fx_subsy);
	      fixP->fx_offset = (add_number + fixP->fx_dot_value
				 + fixP->fx_dot_frag->fr_address);

This code calculates add_number=-4, which is later passed as valP. It also recalculates the offset to the target symbol and assigns it fixP->fx_offset; the value it assigns is identical to the value that was there before.

After that, it seems to do something else for pc-relative relocations in this condition at write.c:1053:

      if (fixP->fx_pcrel)
	{
	  add_number -= MD_PCREL_FROM_SECTION (fixP, this_segment);

... except it doesn't; OR1K md_pcrel_from_section always returns 0 for fixP->fx_addsy which is in different section than the current one, as would always be the case when the assembler has to emit a relocation.

Anyway, we're finally at write.c:1066, and invoking md_apply_fix ~ gas_cgen_md_apply_fix in or1k:

      if (!fixP->fx_done)
	md_apply_fix (fixP, &add_number, this_segment);

After gas_cgen_md_apply_fix sets fixP->fx_addnumber to add_number, the next relevant bit of code is in gas_cgen_tc_gen_reloc is at cgen.c:1069:

  /* Use fx_offset for these cases.  */
  if (fixP->fx_r_type == BFD_RELOC_VTABLE_ENTRY
      || fixP->fx_r_type == BFD_RELOC_VTABLE_INHERIT)
    reloc->addend = fixP->fx_offset;
  else
    reloc->addend = fixP->fx_addnumber;

... which just puts the incorrect value it into the reloc->addend=-4.

It seems that somewhere in the cgen code above there is a bug. I don't really know where and there is not enough information in the comments to determine. I am including the writeup above in case someone is motivated enough to figure that out.

What I did is I ripped out the entire tc_gen_reloc in tc-or1k.c and replaced it with a sane and much simpler version from tc-aarch64.c. It just sets reloc->addend to fixp->fx_offset.

I'm attaching the complete updated patch. It passes the entire testsuite.

Created attachment 8624 [details]
Patch

(In reply to whitequark from comment #11)
> Created attachment 8624 [details]

I may be going mad here, but it looks to me like this patch has already been checked in...

Cheers
  Nick

Nick, that can't be right. I've just looked at binutils gitweb (e.g. https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf32-or1k.c;h=a1eba0956e459653a4867d60d9a262c9cd51a60e;hb=HEAD) and it's not there.

(In reply to whitequark from comment #13)
> Nick, that can't be right. I've just looked at binutils gitweb 

Ah - found it - for some reason your patch is reversed...

OK so I will reverse apply the patch and then test it.

Cheers
  Nick

Ah right, sorry. I messed up the git invocation somehow, it seems.

(In reply to whitequark from comment #11)

I still see a linker testsuite failure for eh-frame-hdr.  Looking in the log I see:

  Executing on host: sh -c {./ld-new  -Lbinutils/current/ld/testsuite/ld-elf  -e _start --eh-frame-hdr -o tmpdir/dump tmpdir/dump0.o  2>&1}  /dev/null ld.tmp (timeout = 300)
spawn [open ...]
  ./ld-new: unrecognized option '--eh-frame-hdr'
  ./ld-new: use the --help option for usage information
  failed with: <./ld-new: unrecognized option '--eh-frame-hdr'

How did you configure your ork1 toolchain ?  I am using "--target=or1k-elf" which may be why this is not working.

Cheers
  Nick

I use --target=or1k-linux, since I need to generate shared object files. I still run them on bare metal but it looks like -linux is the closest I have.

The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8a9e7a9121490a8c64d8c17f5be510e43104f6d9

commit 8a9e7a9121490a8c64d8c17f5be510e43104f6d9
Author: Peter Zotov <whitequark@whitequark.org>
Date:   Fri Sep 25 15:21:14 2015 +0100

    Correct the generation of OR1K pc-relative relocations.
    
    gas	PR ld/18759
    	* config/tc-or1k.c (tc_gen_reloc): Correct computation of PC
    	relative relocs.
    	* config/tc-or1k.h (GAS_CGEN_PRCEL_R_TYPE): Delete.
    
    bfd	* elf32-or1k.c (R_OR1K_32_PCREL): Set pcrel_offset to TRUE.
    	(R_OR1K_16_PCREL): Likewise.
    	(R_OR1K_8_PCREL): Likewise.
    
    ld/tests * ld-elf/eh-frame-hdr: Expect to pass on the or1k-linux target.

Hi Peter,

  OK - I have applied your patch, along with a small change the eh-frame-hdr.d file so that the test is only expected to pass for the or1k-linux variant, and the addition of changelog entries.

Cheers
  Nick

Thanks!