Page MenuHomePhabricator

[ELF] Error on relocations to local undefined symbols
ClosedPublic

Authored by MaskRay on May 4 2019, 11:45 PM.

Details

Summary

For a reference to a local symbol, ld.bfd and gold error if the symbol
is defined in a discarded section but accept it if the symbol is
undefined. This inconsistent behavior seems unnecessary for us (it
probably makes sense for them as they differentiate local/global
symbols, the error would mean more code).

Weaken the condition to getSymbol(Config->IsMips64EL) == 0 to catch such
errors. The symbol index can be 0 (e.g. R_*_NONE) and we shouldn't error on them.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

MaskRay created this revision.May 4 2019, 11:45 PM
MaskRay updated this revision to Diff 198221.May 5 2019, 10:45 PM
MaskRay edited the summary of this revision. (Show Details)

Update description

This patch works on our internal huge code base.

grimar added inline comments.May 6 2019, 5:41 AM
test/ELF/amdgpu-relocs.s
121 ↗(On Diff #198221)

Do you know what was the point to check '.rodata'/'nonalloc' content initially?

grimar added inline comments.May 6 2019, 5:53 AM
ELF/Relocations.cpp
678

I am also not sure it is a good idea to template this function.
Perhaps would be better to check this condition outside/or to pass the getSymbol value?

MaskRay updated this revision to Diff 198260.May 6 2019, 6:23 AM

Delete template parameter RelTy

MaskRay updated this revision to Diff 198266.May 6 2019, 6:40 AM

Move the amdgpu test change to D61594

MaskRay marked 3 inline comments as done.May 6 2019, 6:44 AM

This + D61583 will give an error on the case provided in PR41693, but the error message isn't so good, so we also need D59649 :)

test/ELF/amdgpu-relocs.s
121 ↗(On Diff #198221)

I've moved the amdgpu-relocs.s change to D61594

ruiu added a comment.May 6 2019, 11:51 PM

I may be missing something, but is an undefined local symbol a real thing? It can never be resolved, and thus no one would produce such symbol, no?

MaskRay marked an inline comment as done.May 7 2019, 12:03 AM

I think compilers don't produce local undefined symbol. However, users can write such invalid assembly files (see our amdgpu-relocs.s test)

Also, the "undefined" semantic can be brought up by a symbol defined in a discarded section (COMDAT or SHF_EXCLUDE). The latest report can be found at https://bugs.llvm.org/show_bug.cgi?id=41693 Such (relocations to STT_SECTION symbols whose sections are discarded) errors occur occasionally.

grimar added a comment.May 7 2019, 1:38 AM

I also do not know if undefined local symbol can be a real thing.
So my comment is about implementation.

ELF/Relocations.cpp
1034

Should we just never report error for R_*_NONE maybe? I.e use Type == Target->NoneRel here?
(should look a bit better than comparing symbol index it seems).

Do we have a test for that btw?

MaskRay added a comment.EditedMay 7 2019, 1:56 AM

I also do not know if undefined local symbol can be a real thing.
So my comment is about implementation.

See my previous comment. It is for the error when a symbol (defined in discarded section) is used in a relocation expression.

If I remove SymIndex != 0 the following will break.

arm-v4bx.test
icf10.test
icf11.test
mips-abs-got.s
relocation-dtrace.test
relocation-none-aarch64.test
relocation-none-i686.test

The relocation-dtrace.test looks quite reasonable. R_ARM_NONE (and R_X86_64_NONE etc) can be used to create references among sections to prevent garbage collection (https://lwn.net/Articles/741494/)
mips-abs-got.s .reloc 0, R_MIPS_GOT_PAGE, 0 seems a reasonable relocation type other than R_*_NONE (hope @atanasyan can confirm)

So I have to keep SymIndex != 0 (I did try to delete the check all together before I realized some are legitimate)

MaskRay marked an inline comment as done.May 7 2019, 10:06 PM
grimar added inline comments.May 15 2019, 12:52 AM
ELF/Relocations.cpp
1042

I tried to reorder/simplify the code slightly to avoid symbol index check:

const uint8_t *RelocatedAddr = Sec.data().begin() + Rel.r_offset;
RelExpr Expr = Target->getRelExpr(Type, Sym, RelocatedAddr);

// Ignore "hint" relocations because they are only markers for relaxation.
if (oneof<R_HINT, R_NONE>(Expr))
  return;

// Skip if the target symbol is an erroneous undefined symbol.
if (maybeReportUndefined(Sym, Sec, Rel.r_offset))
  return;

With that the only test case that fails is mips-abs-got.s.
I am not sure if we can do something with it, I am not a MIPS expert.

MaskRay marked an inline comment as done.May 15 2019, 1:02 AM
MaskRay added inline comments.
ELF/Relocations.cpp
1042

@atanasyan Can R_MIPS_GOT_PAGE be (reasonably) used with symbol index 0? If it is theoretically possible but unlikely emitted by compiles/humans in reality, mips-abs-got.s should probably be updated.

I do not remember a reason of adding the .reloc 0, R_MIPS_GOT_PAGE, 0 statement to the test. I think now we can change mips-abs-got.s like that:

--- a/lld/test/ELF/mips-abs-got.s
+++ b/lld/test/ELF/mips-abs-got.s
@@ -4,7 +4,7 @@
 
 # RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux -o %t.o %s
 # RUN: echo "SECTIONS { \
-# RUN:          zero = 0; foo = 0x11004; bar = 0x22000; }" > %t.script
+# RUN:          zero1 = 0; zero2 = 0; foo = 0x11004; bar = 0x22000; }" > %t.script
 # RUN: ld.lld --script %t.script -o %t.exe %t.o
 # RUN: llvm-readobj --mips-plt-got %t.exe | FileCheck %s
 
@@ -30,7 +30,7 @@
 
   .text
   nop
-  .reloc 0, R_MIPS_GOT_PAGE, 0
-  ld      $v0, %got_page(zero)($gp)
+  ld      $v0, %got_page(zero1)($gp)
+  ld      $v0, %got_page(zero2)($gp)
   ld      $v0, %got_page(foo)($gp)
   ld      $v0, %got_page(bar+0x10008)($gp)
ruiu accepted this revision.Mon, May 20, 3:38 AM

LGTM

This revision is now accepted and ready to land.Mon, May 20, 3:38 AM
MaskRay updated this revision to Diff 200246.Mon, May 20, 4:16 AM

R_ARM_V4BX uses symbol index 0. Clarify why we check SymIndex != 0 instead of Type != Target->NoneRel

MaskRay marked an inline comment as done.Mon, May 20, 4:16 AM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Mon, May 20, 4:23 AM
ELF/Relocations.cpp
1035

I.e. this is only for ARM? Should we use a helper there then maybe, like:

(pseudocode)

static bool isMarkerSym(Type) {
 if (!Arm)
  return Type == Target->NoneRel;
return SymIndex != 0;
}