[PATCH] D140279: Adds support for GOT relocations to i386/ELF backend

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/test/ExecutionEngine/JITLink/i386/ELF_external_to_absolute_conversion.s:27-31
+        addl    $_GLOBAL_OFFSET_TABLE_+(.Ltmp0-.L0$pb), %eax
+        movl    $0, -4(%ebp)
+        movl    a at GOTOFF(%eax), %eax
+        addl    $4, %esp
+        popl    %ebp
----------------
jain98 wrote:
> lhames wrote:
> > This sequence is trying to access the non-existent GOT-entry, which is why it crashes at runtime.
> > 
> > I think the assembly can be reduced to the i386 equivalent of the x86-64 sequence:
> > ```
> >         .text
> >         .globl  main                            
> >         .p2align        4, 0x90
> >         .type   main, at function
> > main:                    
> > .L0$pb:
> >         movl    $_GLOBAL_OFFSET_TABLE_-.L0$pb, %eax
> >         movl    $foo at GOTOFF, %eax
> >         xorl    %eax, %eax
> >         retl
> >         .size   main, .-main
> > ```
> > The '$' symbol at the start of the expression means that we're treating the result of the expression as an immediate, not an address to be loaded from. The immediate values that we move into `%eax` will be zeros, but since `%eax` isn't used anyway we can disregard that.
> > This sequence is trying to access the non-existent GOT-entry, which is why it crashes at runtime.
> 
> I didn't quite get that. I //think//, you're referring to this instruction -
> ```
> movl    a at GOTOFF(%eax), %eax
> ```
> I thought this instruction was using `a`'s offset from the GOT symbol - not accessing any GOT entry? Could you point to the specific instruction that you were referring to in your comment?
> 
> I do think I understand why the seg fault might be happening though. Essentially `a at GOTOFF(%eax)` ends up being `a at GOTOFF(0x00000000)`? Which is likely out of the JITLink process' memory. If that is indeed the case, I have another question. The assembly that I used from the test was actually generated by Clang. Is it just that Clang uses a different linker that can actually handle the code that it's producing without seg-faulting?
> I thought this instruction was using a's offset from the GOT symbol - not accessing any GOT entry? Could you point to the specific instruction that you were referring to in your comment?

Oh you're right -- `GOTOFF` references the symbol, not the GOT entry for the symbol. Sorry for the confusion!

In that case you're absolutely right -- this should work even if there's no GOT synthesized. It looks like the problem is that the `R_386_GOTPC` relocation has an addend embedded in it, but it's not being included in the Edge construction:

```
ELF_external_to_absolute_conversion.i386.o:     file format elf32-i386


Disassembly of section .text:

00000000 <main>:
   0:   55                      push   %ebp
   1:   89 e5                   mov    %esp,%ebp
   3:   50                      push   %eax
   4:   e8 00 00 00 00          call   9 <main+0x9>
   9:   58                      pop    %eax
   a:   81 c0 03 00 00 00       add    $0x3,%eax.       <-- 0x00000003 addend embedded here at fixup location.
  10:   c7 45 fc 00 00 00 00    movl   $0x0,-0x4(%ebp)
  17:   8b 80 00 00 00 00       mov    0x0(%eax),%eax
  1d:   83 c4 04                add    $0x4,%esp
  20:   5d                      pop    %ebp
  21:   c3                      ret    
```

Edge from debugging output:
```
      0xb7ab600c (block + 0x0000000c), addend = +0x00000000, kind = Delta32, target = _GLOBAL_OFFSET_TABLE_  
```

You just need to read that addend out when constructing the edge. E.g. here's the MachO_x86_64 backend doing something similar: https://github.com/llvm/llvm-project/blob/5a58b19f9c93f3ac51bcde318508131ae78aa10c/llvm/lib/ExecutionEngine/JITLink/MachO_x86_64.cpp#L342

The missing addend is causing the variable reference to point to the page _before_ the data section, which is usually outside allocated memory -- that's triggering the crash, not the missing GOT.

Once you fix that I think your original test case should just work -- sorry for the bad advice above!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140279/new/

https://reviews.llvm.org/D140279