[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 inline comments.


================
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()) +
----------------
MaskRay wrote:
> grimar wrote:
> > 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.
> Changed the description. I think the usage of `InX::Got->getVA() + InX::Got->getSize()` is incorrect. We should use either:
> 
> * `ElfSym::GlobalOffsetTable`. but it can be null when `_GLOBAL_OFFSET_TABLE_` is not explicitly referenced.
> * If `GlobalOffsetTable` is null, use `InX::Got` or `InX::GotPlt` depending on `GotBaseSymInGotPlt`. We can avoid this if we scan relocations earlier and decide to emit `GlobalOffsetTable`.
> 
> To properly address this I also need to study the TLS stuff. This revision is already sort of complicated, I want to keep the changes in a future revision
> If GlobalOffsetTable is null, use InX::Got or InX::GotPlt depending on GotBaseSymInGotPlt. We can avoid this if we scan relocations earlier and decide to emit GlobalOffsetTable.

I think `GlobalOffsetTable` should never be null here and scanning earlier seems to be the correct way to me. I am not sure if there will be problems or it is easy to do.
Because of that, I think it would be better to see the full patch. It should reveal the whole picture and get rid of the need to 
add symbols to the existent test cases.




================
Comment at: test/ELF/got-i386.s:41
-// 0x1200A - 10 = addr(.got) = 0x12000
-// 0x1200A + 5 - 15 = addr(.got) = 0x12000
 // DISASM:      Disassembly of section .text:
----------------
MaskRay wrote:
> grimar wrote:
> > You should update these calculations and keep them instead of dropping.
> I kept `// 0x13000 (bar) - 0x12000 (_GLOBAL_OFFSET_TABLE_) = 4096`.
The original code contains 3 code lines and 3 comments, one for each:

```
  movl $1, bar at GOTOFF(%ecx)
  movl $2, obj at GOTOFF(%ecx)
  movl $3, obj+5 at GOTOFF(%ecx)

// 0x12000 - 0 = addr(.got) = 0x12000
// 0x1200A - 10 = addr(.got) = 0x12000
// 0x1200A + 5 - 15 = addr(.got) = 0x12000

```

You dropped 2/3 of comments. Please don't do that.


================
Comment at: test/ELF/i386-gotoff-shared.s:24
 
-// 0x1000 - (0x2050 + 4) = -4180
+// 0x2000 (_GLOBAL_OFFSET_TABLE_) - 0x1000 = -4096
 
----------------
0x2000 - 0x1000 != -4096


================
Comment at: test/ELF/relocation-i686.s:3
 // RUN: llvm-mc -filetype=obj -triple=i686-unknown-linux %p/Inputs/shared.s -o %t2.o
-// RUN: ld.lld -shared %t2.o -o %t2.so
+// RUN: ld.lld -shared %t2.o -soname t2.so -o %t2.so
 // RUN: ld.lld --hash-style=sysv %t %t2.so -o %t2
----------------
MaskRay wrote:
> grimar wrote:
> > Why you had to do that?
> In case the pathname of %t2.o is too long in some environment and makes `DT_NEEDED` of `%t2` longer and changes addresses of other fields.
This sounds like an independent change for me.


================
Comment at: test/ELF/x86-64-reloc-shared.s:17
+R_X86_64_GOTPC64:
+  movabsq $_GLOBAL_OFFSET_TABLE_-., %r11
----------------
MaskRay wrote:
> grimar wrote:
> > 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.
> `R_X86_64_GOTPC32` is a special case of `R_X86_64_PC32` where the symbol is `_GLOBAL_OFFSET_TABLE_`. GNU as emits `R_X86_64_GOTPC32` but llvm-mc emits `R_X86_64_PC32`. I have also tested GNU as 
If llvm-mc is unable to generate inputs for the test, you can use yaml2obj to craft objects.
(For example, see `ELF\invalid\invalid-relocation-x64.test`)

But currently the R_X86_64_GOTPC32 is untested by your test case and that is not OK.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47098