[PATCH] D47098: [ELF] Make R_GOTONLY_PC_FROM_END R_GOTREL_FROM_END relative to _GLOBAL_OFFSET_TABLE_ instead of end of .got

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

I am not sure in this patch honestly. I'll add Peter, the author of https://reviews.llvm.org/D34355/https://reviews.llvm.org/D44259.

You wrote:

> Currently in X86 and X86_64, when _GLOBAL_OFFSET_TABLE_ is used, there can be a mismatch of its value and the end of .got

That should not be important I think. As far I understand, _GLOBAL_OFFSET_TABLE_[0] can point either to .got or to .got.plt (depending on target) and used by glibc
to find link time address of _DYNAMIC (see https://bugs.llvm.org/show_bug.cgi?id=36555). 
Does not seem we should assume that _GLOBAL_OFFSET_TABLE_ is the end of .got or not the end of .got.

It does not seem correct to me to resolve relocations relative to _GLOBAL_OFFSET_TABLE_ generally. There is an issue with resolving
relocations against particular _GLOBAL_OFFSET_TABLE_ symbol, but I am not sure that fix should affect anything else.



================
Comment at: ELF/InputSection.cpp:501
+    // If we scanned relocations earlier, GlobalOffsetTable could't be null.
+    return (GOTBase ? GOTBase->getVA()
+                    : InX::Got->getVA() + InX::Got->getSize()) +
----------------
What is your plan to fix this `FIXME`? It does not seem nice approach to add weak _GLOBAL_OFFSET_TABLE_ symbols to test cases
which affects the calculation.


================
Comment at: test/ELF/got-i386.s:8
+// CHECK:      Name: .got (
+// CHECK:      Type: SHT_PROGBITS
 // CHECK-NEXT: Flags [
----------------
Why you dropped `-NEXT`?


================
Comment at: test/ELF/got-i386.s:37
-// CHECK-NEXT:  Section: .bss
-// CHECK-NEXT: }
 
----------------
Please do minimal changes to test cases. It was no need to drop all these lines in this patch I believe.


================
Comment at: test/ELF/got-i386.s:41
-// 0x1200A - 10 = addr(.got) = 0x12000
-// 0x1200A + 5 - 15 = addr(.got) = 0x12000
 // DISASM:      Disassembly of section .text:
----------------
You should update these calculations and keep them instead of dropping.


================
Comment at: test/ELF/x86-64-reloc-shared.s:17
+R_X86_64_GOTPC64:
+  movabsq $_GLOBAL_OFFSET_TABLE_-., %r11
----------------
What I see is that llvm-mc produce following relocations here for me:

```
000000000003  000300000002 R_X86_64_PC32     0000000000000000 _GLOBAL_OFFSET_TABLE_ - 4
000000000009  00030000001d R_X86_64_GOTPC64  0000000000000000 _GLOBAL_OFFSET_TABLE_ + 2
```

So R_X86_64_GOTPC32 seems to be uncovered be this test.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47098