[Bug binutils/18759] R_OR1K_*_PCREL should have pcrel_offset=TRUE

[whitequark at whitequark dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18759

--- Comment #10 from whitequark at whitequark dot org ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.