This is an archive of the discontinued LLVM Phabricator instance.

Differential D137384

[MC][LoongArch] Fix needsRelocateWithSymbol() implementation
ClosedPublic

Authored by tangyouling on Nov 3 2022, 7:48 PM.

Details

Reviewers
SixWeining
xen0n
xry111
MaskRay
XiaodongLoong
wangleiat
Commits
rGb18cdeac79d0: [MC][LoongArch] Fix needsRelocateWithSymbol() implementation
Summary

LoongArch should be section plus offset, so use the generic implementation
of llvm/lib/MC/MCELFObjectTargetWriter.cpp to return false directly, like
x86 and aarch64.

$ cat test.c
static int __attribute__((section(".text.another"))) test(int a, int b)
{
	return a + b;
}
static int a = 1, b = 2;

int foo()
{
	test(a, b);
	return 0;
}

$ gcc -c test.c
$ readelf -Wr test.o

Relocation section '.rela.text' at offset 0x2a0 contains 5 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000010  0000000300000047 R_LARCH_PCALA_HI20     0000000000000000 .data + 0
0000000000000014  0000000300000048 R_LARCH_PCALA_LO12     0000000000000000 .data + 0
0000000000000018  0000000300000047 R_LARCH_PCALA_HI20     0000000000000000 .data + 4
000000000000001c  0000000300000048 R_LARCH_PCALA_LO12     0000000000000000 .data + 4
0000000000000028  0000000500000042 R_LARCH_B26            0000000000000000 .text.another + 0

Relocation section '.rela.eh_frame' at offset 0x318 contains 2 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
000000000000001c  0000000500000063 R_LARCH_32_PCREL       0000000000000000 .text.another + 0
000000000000003c  0000000200000063 R_LARCH_32_PCREL       0000000000000000 .text + 0

Diff Detail

Event Timeline

tangyouling created this revision.Nov 3 2022, 7:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 7:48 PM
Herald added subscribers: StephenFan, pengfei, hiraditya, kristof.beyls. · View Herald Transcript
tangyouling requested review of this revision.Nov 3 2022, 7:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 7:48 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
tangyouling added a comment.Nov 3 2022, 7:50 PM

in LoongArch:

$ readelf -Wr test.o 

Relocation section '.rela.text' at offset 0x2a0 contains 5 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000010  0000000300000047 R_LARCH_PCALA_HI20     0000000000000000 .data + 0
0000000000000014  0000000300000048 R_LARCH_PCALA_LO12     0000000000000000 .data + 0
0000000000000018  0000000300000047 R_LARCH_PCALA_HI20     0000000000000000 .data + 4
000000000000001c  0000000300000048 R_LARCH_PCALA_LO12     0000000000000000 .data + 4
0000000000000028  0000000500000042 R_LARCH_B26            0000000000000000 .text.another + 0

Relocation section '.rela.eh_frame' at offset 0x318 contains 2 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
000000000000001c  0000000500000063 R_LARCH_32_PCREL       0000000000000000 .text.another + 0
000000000000003c  0000000200000063 R_LARCH_32_PCREL       0000000000000000 .text + 0

in x86_64:

$ readelf -Wr test_x86.o 

Relocation section '.rela.text' at offset 0x1e8 contains 3 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000006  0000000300000002 R_X86_64_PC32          0000000000000000 .data + 0
000000000000000c  0000000300000002 R_X86_64_PC32          0000000000000000 .data - 4
0000000000000015  0000000400000002 R_X86_64_PC32          0000000000000000 .text.another - 4

Relocation section '.rela.eh_frame' at offset 0x230 contains 2 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000020  0000000400000002 R_X86_64_PC32          0000000000000000 .text.another + 0
0000000000000040  0000000200000002 R_X86_64_PC32          0000000000000000 .text + 0

in aarch64:

$ aarch64-linux-gnu-readelf -Wr test_aarch64.o 

Relocation section '.rela.text' at offset 0x2c8 contains 5 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000008  0000000300000113 R_AARCH64_ADR_PREL_PG_HI21 0000000000000000 .data + 0
000000000000000c  0000000300000115 R_AARCH64_ADD_ABS_LO12_NC 0000000000000000 .data + 0
0000000000000014  0000000300000113 R_AARCH64_ADR_PREL_PG_HI21 0000000000000000 .data + 4
0000000000000018  0000000300000115 R_AARCH64_ADD_ABS_LO12_NC 0000000000000000 .data + 4
0000000000000028  000000050000011b R_AARCH64_CALL26       0000000000000000 .text.another + 0

Relocation section '.rela.eh_frame' at offset 0x340 contains 2 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
000000000000001c  0000000500000105 R_AARCH64_PREL32       0000000000000000 .text.another + 0
0000000000000034  0000000200000105 R_AARCH64_PREL32       0000000000000000 .text + 0

in RISCV:

$ riscv64-linux-gnu-readelf -Wr test_riscv.o 

Relocation section '.rela.text' at offset 0x2b0 contains 10 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000008  0000000800000017 R_RISCV_PCREL_HI20     0000000000000000 a + 0
0000000000000008  0000000000000033 R_RISCV_RELAX                             0
000000000000000c  0000000c00000018 R_RISCV_PCREL_LO12_I   0000000000000008 .L0  + 0
000000000000000c  0000000000000033 R_RISCV_RELAX                             0
0000000000000012  0000000900000017 R_RISCV_PCREL_HI20     0000000000000004 b + 0
0000000000000012  0000000000000033 R_RISCV_RELAX                             0
0000000000000016  0000000d00000018 R_RISCV_PCREL_LO12_I   0000000000000012 .L0  + 0
0000000000000016  0000000000000033 R_RISCV_RELAX                             0
0000000000000020  0000000600000012 R_RISCV_CALL           0000000000000000 test + 0
0000000000000020  0000000000000033 R_RISCV_RELAX                             0
                           0
tangyouling added a comment.Nov 3 2022, 8:15 PM

This patch fixes some of the CallTrace issues like the following:

Before applying the patch.

$ ./test/asan/LOONGARCH64LinuxConfig/TestCases/Posix/Output/shared-lib-test.cpp.tmp
opening ./test/asan/LOONGARCH64LinuxConfig/TestCases/Posix/Output/shared-lib-test.cpp.tmp-so.so ... 
ok
=================================================================
==621522==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ffff150c1bc at pc 0x7ffff1504cd8 bp 0x7ffffb8e7770 sp 0x7ffffb8e7758
READ of size 4 at 0x7ffff150c1bc thread T0
    #0 0x7ffff1504cd4 in inc /home/loongson/llvm-work/llvm-project-test2/llvm-project/compiler-rt/test/asan/TestCases/Posix/shared-lib-test.cpp:49:14
    #1 0x555555db014c  (/home/loongson/llvm-work/llvm-project-test2/llvm-project/build_crt/test/asan/LOONGARCH64LinuxConfig/TestCases/Posix/Output/shared-lib-test.cpp.tmp+0x10c14c)
    #2 0x7ffff1921678  (/usr/lib64/libc.so.6+0x25678)
    #3 0x7ffff1921764 in __libc_start_main (/usr/lib64/libc.so.6+0x25764)
    #4 0x555555ccafc4 in _start /opt/mylaos/build/glibc-2.36/csu/../sysdeps/loongarch/start.S:61

After applying the patch.

$ ./test/asan/LOONGARCH64LinuxConfig/TestCases/Posix/Output/shared-lib-test.cpp.tmp
opening ./test/asan/LOONGARCH64LinuxConfig/TestCases/Posix/Output/shared-lib-test.cpp.tmp-so.so ... 
ok
=================================================================
==586862==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ffff258c1bc at pc 0x7ffff2584cd8 bp 0x7ffffba7bfb0 sp 0x7ffffba7bf98
READ of size 4 at 0x7ffff258c1bc thread T0
    #0 0x7ffff2584cd4 in inc /home/loongson/llvm-work/llvm-project-test2/llvm-project/compiler-rt/test/asan/TestCases/Posix/shared-lib-test.cpp:49:14
    #1 0x555558838150 in main /home/loongson/llvm-work/llvm-project-test2/llvm-project/compiler-rt/test/asan/TestCases/Posix/shared-lib-test.cpp:33:3
    #2 0x7ffff29a1678  (/usr/lib64/libc.so.6+0x25678)
    #3 0x7ffff29a1764 in __libc_start_main (/usr/lib64/libc.so.6+0x25764)
    #4 0x555558752fc4 in _start /opt/mylaos/build/glibc-2.36/csu/../sysdeps/loongarch/start.S:61

We can see that #1 can find the location of the file where the function is located.

MaskRay accepted this revision.Nov 3 2022, 8:24 PM

LGTM. Copy-paste mistake from RISC-V (which has linker relaxation) ;-)

This revision is now accepted and ready to land.Nov 3 2022, 8:24 PM
Harbormaster completed remote builds in B196053: Diff 473111.Nov 3 2022, 8:26 PM
MaskRay added a comment.Nov 3 2022, 8:28 PM

LoongArch should be section plus offset

I think you mean most relocations can use the section symbol plus offset representation.
Some relocations need to use the original symbol anyway, e.g. ifunc symbols.

xry111 added a comment.Nov 3 2022, 8:28 PM

Hmm, BFD linker does not support R_LARCH_GOT_PC_{HI12,LO20} with a non-zero addend:

case R_LARCH_GOT_PC_HI20:
case R_LARCH_GOT_HI20:
  /* Calc got offset.  */
    {
      unresolved_reloc = false;
      BFD_ASSERT (rel->r_addend == 0);

Do we need to make a custom implementation which returns false for R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B*, true for others?

tangyouling edited the summary of this revision. (Show Details)Nov 3 2022, 9:09 PM
tangyouling added a comment.Nov 3 2022, 9:16 PM

I think you mean most relocations can use the section symbol plus offset representation.
Some relocations need to use the original symbol anyway, e.g. ifunc symbols.

Hmm, it should be.

Do we need to make a custom implementation which returns false for R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B*, true for others?

It feels better to add a custom implementation. But I'm not quite sure which relocation types need to return true, and which return false ? (it's difficult for me to distinguish these correctly :( )

tangyouling added a comment.Nov 3 2022, 9:19 PM

Do x86 and aarch64 need to distinguish between relocation types (here we might be more like x86 and )?

SixWeining added subscribers: lixing-star, MQ-mengqing.Nov 5 2022, 11:44 PM
MQ-mengqing added a comment.Nov 6 2022, 5:10 PM
In D137384#3907086, @xry111 wrote:

Hmm, BFD linker does not support R_LARCH_GOT_PC_{HI12,LO20} with a non-zero addend:

case R_LARCH_GOT_PC_HI20:
case R_LARCH_GOT_HI20:
  /* Calc got offset.  */
    {
      unresolved_reloc = false;
      BFD_ASSERT (rel->r_addend == 0);

Do we need to make a custom implementation which returns false for R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B*, true for others?

The procedure happens before linking. AFAIK, the way of using sec+offset is like RELA while the way of using sym is like REL (, of course it seems the REL is a subset of the RELA).
Relocation symbols that can replace sym with sec+offset are generally local and its offset from sec can be determined at assembly procedure.
If relax which may decrease instructions is not used, the offset would not be changed later. So it is different from the Relocation of RISCV. Replacing the sym with sec+offset effectively reduces the number of syms.
The GOT method is used to access global symbols mostly, can they enter this function?

tangyouling added a comment.Nov 8 2022, 10:55 PM
In D137384#3910952, @MQ-mengqing wrote:
In D137384#3907086, @xry111 wrote:

Hmm, BFD linker does not support R_LARCH_GOT_PC_{HI12,LO20} with a non-zero addend:

case R_LARCH_GOT_PC_HI20:
case R_LARCH_GOT_HI20:
  /* Calc got offset.  */
    {
      unresolved_reloc = false;
      BFD_ASSERT (rel->r_addend == 0);

Do we need to make a custom implementation which returns false for R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B*, true for others?

The procedure happens before linking. AFAIK, the way of using sec+offset is like RELA while the way of using sym is like REL (, of course it seems the REL is a subset of the RELA).
Relocation symbols that can replace sym with sec+offset are generally local and its offset from sec can be determined at assembly procedure.
If relax which may decrease instructions is not used, the offset would not be changed later. So it is different from the Relocation of RISCV. Replacing the sym with sec+offset effectively reduces the number of syms.
The GOT method is used to access global symbols mostly, can they enter this function?

Yes, it happens before linking.

I found that only R_LARCH_32, R_LARCH_64, R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B* enter the function at compile time (some others like GOT don't enter it).
Modified it to the following, which also passed the test (when testing asan),

bool needsRelocateWithSymbol(const MCSymbol &Sym,
                             unsigned Type) const override {
  switch (Type) {
  default:
    return true;

  case ELF::R_LARCH_32:
  case ELF::R_LARCH_64:
  case ELF::R_LARCH_B16:
  case ELF::R_LARCH_B21:
  case ELF::R_LARCH_B26:
  case ELF::R_LARCH_PCALA_HI20:
  case ELF::R_LARCH_PCALA_LO12:
  case ELF::R_LARCH_PCALA64_LO20:
  case ELF::R_LARCH_PCALA64_HI12:
  case ELF::R_LARCH_32_PCREL:
    return false;
  }
}

Which do you think is more acceptable, using a generic implementation or using a custom implementation?

MQ-mengqing added a comment.Nov 8 2022, 11:31 PM
In D137384#3916546, @tangyouling wrote:
In D137384#3910952, @MQ-mengqing wrote:
In D137384#3907086, @xry111 wrote:

Hmm, BFD linker does not support R_LARCH_GOT_PC_{HI12,LO20} with a non-zero addend:

case R_LARCH_GOT_PC_HI20:
case R_LARCH_GOT_HI20:
  /* Calc got offset.  */
    {
      unresolved_reloc = false;
      BFD_ASSERT (rel->r_addend == 0);

Do we need to make a custom implementation which returns false for R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B*, true for others?

The procedure happens before linking. AFAIK, the way of using sec+offset is like RELA while the way of using sym is like REL (, of course it seems the REL is a subset of the RELA).
Relocation symbols that can replace sym with sec+offset are generally local and its offset from sec can be determined at assembly procedure.
If relax which may decrease instructions is not used, the offset would not be changed later. So it is different from the Relocation of RISCV. Replacing the sym with sec+offset effectively reduces the number of syms.
The GOT method is used to access global symbols mostly, can they enter this function?

Yes, it happens before linking.

I found that only R_LARCH_32, R_LARCH_64, R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B* enter the function at compile time (some others like GOT don't enter it).
Modified it to the following, which also passed the test (when testing asan),

bool needsRelocateWithSymbol(const MCSymbol &Sym,
                             unsigned Type) const override {
  switch (Type) {
  default:
    return true;

  case ELF::R_LARCH_32:
  case ELF::R_LARCH_64:
  case ELF::R_LARCH_B16:
  case ELF::R_LARCH_B21:
  case ELF::R_LARCH_B26:
  case ELF::R_LARCH_PCALA_HI20:
  case ELF::R_LARCH_PCALA_LO12:
  case ELF::R_LARCH_PCALA64_LO20:
  case ELF::R_LARCH_PCALA64_HI12:
  case ELF::R_LARCH_32_PCREL:
    return false;
  }
}

Which do you think is more acceptable, using a generic implementation or using a custom implementation?

In D137384#3916546, @tangyouling wrote:
In D137384#3910952, @MQ-mengqing wrote:
In D137384#3907086, @xry111 wrote:

Hmm, BFD linker does not support R_LARCH_GOT_PC_{HI12,LO20} with a non-zero addend:

case R_LARCH_GOT_PC_HI20:
case R_LARCH_GOT_HI20:
  /* Calc got offset.  */
    {
      unresolved_reloc = false;
      BFD_ASSERT (rel->r_addend == 0);

Do we need to make a custom implementation which returns false for R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B*, true for others?

The procedure happens before linking. AFAIK, the way of using sec+offset is like RELA while the way of using sym is like REL (, of course it seems the REL is a subset of the RELA).
Relocation symbols that can replace sym with sec+offset are generally local and its offset from sec can be determined at assembly procedure.
If relax which may decrease instructions is not used, the offset would not be changed later. So it is different from the Relocation of RISCV. Replacing the sym with sec+offset effectively reduces the number of syms.
The GOT method is used to access global symbols mostly, can they enter this function?

Yes, it happens before linking.

I found that only R_LARCH_32, R_LARCH_64, R_LARCH_32_PCREL, R_LARCH_PCALA*, and R_LARCH_B* enter the function at compile time (some others like GOT don't enter it).
Modified it to the following, which also passed the test (when testing asan),

bool needsRelocateWithSymbol(const MCSymbol &Sym,
                             unsigned Type) const override {
  switch (Type) {
  default:
    return true;

  case ELF::R_LARCH_32:
  case ELF::R_LARCH_64:
  case ELF::R_LARCH_B16:
  case ELF::R_LARCH_B21:
  case ELF::R_LARCH_B26:
  case ELF::R_LARCH_PCALA_HI20:
  case ELF::R_LARCH_PCALA_LO12:
  case ELF::R_LARCH_PCALA64_LO20:
  case ELF::R_LARCH_PCALA64_HI12:
  case ELF::R_LARCH_32_PCREL:
    return false;
  }
}

Which do you think is more acceptable, using a generic implementation or using a custom implementation?

Hi, All,

Frankly the custom implementation is more appreciated at present. Although I want to use the generic implementation but binutils cannot guarantee the user action as follows,

Code:
    la.got $r0, sym
    sym:
Reloc:
    0000000000000000  000000010000004b R_LARCH_GOT_PC_HI20    0000000000000000 .text + 8
    0000000000000004  000000010000004c R_LARCH_GOT_PC_LO12    0000000000000000 .text + 8

When sym is local, binutils-ld failed here. As relax may be added in the future, thus, I think it's appropriate to use custom implementation.

Other ideas?

tangyouling mentioned this in D137228: [sanitizer] Add ptrace interceptor support for LoongArch.Nov 11 2022, 5:05 PM
SixWeining accepted this revision.Nov 15 2022, 11:51 PM
Closed by commit rGb18cdeac79d0: [MC][LoongArch] Fix needsRelocateWithSymbol() implementation (authored by tangyouling, committed by SixWeining). · Explain WhyNov 15 2022, 11:53 PM
This revision was automatically updated to reflect the committed changes.
SixWeining added a commit: rGb18cdeac79d0: [MC][LoongArch] Fix needsRelocateWithSymbol() implementation.

Revision Contents

PathSize
llvm/
lib/
Target/
LoongArch/
MCTargetDesc/
7 lines
test/
MC/
LoongArch/
Relocations/
2 lines

Diff 475701

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchELFObjectWriter.cpp

Show All 18 Lines
namespace { namespace {
class LoongArchELFObjectWriter : public MCELFObjectTargetWriter { class LoongArchELFObjectWriter : public MCELFObjectTargetWriter {
public: public:
LoongArchELFObjectWriter(uint8_t OSABI, bool Is64Bit); LoongArchELFObjectWriter(uint8_t OSABI, bool Is64Bit);
~LoongArchELFObjectWriter() override; ~LoongArchELFObjectWriter() override;
// Return true if the given relocation must be with a symbol rather than
// section plus offset.
bool needsRelocateWithSymbol(const MCSymbol &Sym,
unsigned Type) const override {
return true;
}
protected: protected:
unsigned getRelocType(MCContext &Ctx, const MCValue &Target, unsigned getRelocType(MCContext &Ctx, const MCValue &Target,
const MCFixup &Fixup, bool IsPCRel) const override; const MCFixup &Fixup, bool IsPCRel) const override;
}; };
} // end namespace } // end namespace
LoongArchELFObjectWriter::LoongArchELFObjectWriter(uint8_t OSABI, bool Is64Bit) LoongArchELFObjectWriter::LoongArchELFObjectWriter(uint8_t OSABI, bool Is64Bit)
: MCELFObjectTargetWriter(Is64Bit, OSABI, ELF::EM_LOONGARCH, : MCELFObjectTargetWriter(Is64Bit, OSABI, ELF::EM_LOONGARCH,
▲ Show 20 Lines • Show All 58 Lines • Show Last 20 Lines

llvm/test/MC/LoongArch/Relocations/fde-reloc.s

# RUN: llvm-mc --filetype=obj --triple=loongarch64 < %s \ # RUN: llvm-mc --filetype=obj --triple=loongarch64 < %s \
# RUN: | llvm-readobj -r - | FileCheck %s # RUN: | llvm-readobj -r - | FileCheck %s
## Ensure that the eh_frame records the symbolic difference with ## Ensure that the eh_frame records the symbolic difference with
## the R_LARCH_32_PCREL relocation. ## the R_LARCH_32_PCREL relocation.
func: func:
.cfi_startproc .cfi_startproc
ret ret
.cfi_endproc .cfi_endproc
# CHECK: Section (4) .rela.eh_frame { # CHECK: Section (4) .rela.eh_frame {
# CHECK-NEXT: 0x1C R_LARCH_32_PCREL - 0x0 # CHECK-NEXT: 0x1C R_LARCH_32_PCREL .text 0x0
# CHECK-NEXT: } # CHECK-NEXT: }