[PATCH] D29490: [ELF] - Use SignExtend when reading R_386_PC8 addend.

[Rafael Avila de Espindola via llvm-commits]

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:
> Index: test/ELF/i386-pc8-addend.s
> ===================================================================
> --- test/ELF/i386-pc8-addend.s
> +++ test/ELF/i386-pc8-addend.s
> @@ -0,0 +1,42 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=i386-pc-linux-gnu %s -o %t1.o
> +# RUN: llvm-objdump -d %t1.o | FileCheck %s --check-prefix=OBJDISASM
> +# RUN: llvm-readobj -r %t1.o | FileCheck %s --check-prefix=OBJRELOC
> +
> +## Show that we have R_386_PC8 relocation with addend = 0xFF(-1).
> +# OBJDISASM:      Disassembly of section .text:
> +# OBJDISASM-NEXT: .text:
> +# OBJDISASM-NEXT:  0: eb ff jmp -1 <.text+0x1>
> +# OBJRELOC:       Relocations [
> +# OBJRELOC-NEXT:   Section {{.*}} .rel.text {
> +# OBJRELOC-NEXT:     0x1 R_386_PC8 .foo 0x0
> +# OBJRELOC-NEXT:   }
> +# OBJRELOC-NEXT: ]

You probably don't need to test llvm-mc here. You can add that to llvm
if we are missing it.

> +## Check that there is no link error and relocation applied correctly.
> +# RUN: ld.lld %t1.o -o %t.out
> +# RUN: llvm-objdump -d %t.out | FileCheck %s --check-prefix=DISASM
> +# DISASM:      Disassembly of section .text:
> +# DISASM-NEXT: .text:
> +# DISASM-NEXT:    11000: eb 04 jmp 4 <foo>
> +# DISASM:      Disassembly of section .foo:
> +# DISASM-NEXT: foo:
> +# DISASM-NEXT:    11006: 90 nop
> +
> +# CHECK:      Contents of section .text:
> +# CHECK-NEXT:  0000 15253748

You don't have any FileCheck for this.

> +.code16
> +
> +.byte  0xeb   # short (2-byte) jump
> +.byte  foo-1f
> +
> +1:
> +nop
> +nop
> +nop
> +nop

Why do you need the nops after the target?

> +.section ".foo", "ax"
> +foo:
> + nop
> Index: ELF/Target.cpp
> ===================================================================
> --- ELF/Target.cpp
> +++ ELF/Target.cpp
> @@ -492,8 +492,9 @@
>    default:
>      return 0;
>    case R_386_8:
> -  case R_386_PC8:
>      return *Buf;
> +  case R_386_PC8:
> +    return SignExtend64<8>(*Buf);
>    case R_386_16:
>    case R_386_PC16:
>      return read16le(Buf);

We should probably fix PC16 too in a followup patch.

Cheers,
Rafael