Page MenuHomePhabricator

Initial commit for the elf x86 implementation for jitlink
ClosedPublic

Authored by jaredwy on Tue, May 12, 6:14 PM.

Details

Summary

This is the first commit(and my first commit to LLVM :D) of many to come for the elf x86 jit linker. This just gets a very basic c program working with the execution engine.
e.g.

int main() return 42;

I plan to continue the rest of the work in tree. To that point there is a lot of clean up that will continue to happen as this evolves and the code state reflects that. For example there is commented out code, and some variables unused that are there for future work.

This is my first time using many of these apis so I welcome any improvements or suggestions if I am using them wrong or missed any utility methods to make life easier.
Thanks!

Diff Detail

Event Timeline

jaredwy created this revision.Tue, May 12, 6:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 12, 6:14 PM

This is great stuff -- thanks Jared!

I've included some style comments inline.

This should also have an llvm-jitlink regression test case along the lines of llvm/test/ExecutionEngine/JITLink/X86/MachO_x86-64_relocations.s. Since this initial version doesn't support relocations your test case should just be a sanity check for ELF magic recognition, and section and symbol parsing. E.g.

# RUN: rm -rf %t && mkdir -p %t
# RUN: llvm-mc -triple=x86_64-unknown-linux -filetype=obj -o %t/elf_reloc.o %s
# RUN: llvm-jitlink -noexec -slab-allocate 100Kb -slab-address 0xfff00000 \
# RUN:    -define-abs external_data=0x1 -define-abs external_func=0x2 \
# RUN:    -check=%s %t/elf_reloc.o
#
# Test standard ELF relocations.

        .text
        .file   "testcase.c"
        .globl  main
        .p2align        4, 0x90
        .type   main,@function
main:
        movl    $42, %eax
        retq
.Lfunc_end0:
        .size   main, .Lfunc_end0-main

        .ident  "clang version 10.0.0-4ubuntu1 "
        .section        ".note.GNU-stack","",@progbits
        .addrsig

(Testcase not actually tested yet: I'm still building your branch)

llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
161

MachO only has 256 sections indexes (the section index field is 8-bit), but I thought ELF had more?

245–256

This could use a clear TODO comment.

271–273

The LLVM style guide opts for no braces around single conditional statements, so this should be:

if (isRelocatable())
  return make_error<JITLinkError>("Object is not a relocatable ELF");
321–323

This should probably have a "TODO: add relocation handling" comment, even if it's obvious.

Just got a chance to look at the build messages for this. There are a few warnings (noted inline). I also noticed that this patch is against 2e42cc7a50e which is from late January. I don't think the JITLink APIs have changed too much: if you're happy to rebase against top of tree then go for it, otherwise I'm happy to do that before committing.

Finally, I got a chance to check out my suggested test line and realized that you don't need most of it for now. The following will do:

# RUN: rm -rf %t && mkdir -p %t
# RUN: llvm-mc -triple=x86_64-unknown-linux -filetype=obj -o %t/elf_reloc.o %s
# RUN: llvm-jitlink -noexec %t/elf_reloc.o
#
# Test standard ELF relocations.

        .text
        .file   "testcase.c"
        .globl  main
        .p2align        4, 0x90
        .type   main,@function
main:
        movl    $42, %eax
        retq
.Lfunc_end0:
        .size   main, .Lfunc_end0-main

        .ident  "clang version 10.0.0-4ubuntu1 "
        .section        ".note.GNU-stack","",@progbits
        .addrsig

Once those warnings are taken care of and the test case is in this looks good to me.

llvm/lib/ExecutionEngine/JITLink/ELF.cpp
31

debug_prefix is currently unused.

llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
72–73

This should error out rather than continuing.

85–87

Other and Size are unused here.

173–174

Ditto here.

jaredwy updated this revision to Diff 264378.Fri, May 15, 4:12 PM
jaredwy marked 6 inline comments as done.Fri, May 15, 8:24 PM
jaredwy added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
72–73

I have this fixed this locally. As discussed this now outputs the error to dgbs() rather than just dropping it.

161

I have removed this locally as I found out that the elf object can handle this lookup for me.

lhames accepted this revision.Sat, May 16, 8:49 PM

Ok. This looks good to me!

If you don't have commit access just let me know and I will commit on your behalf.

This revision is now accepted and ready to land.Sat, May 16, 8:49 PM

I will try and get commit status but I think to get this into tree, lets just have you commit it for now.

This revision was automatically updated to reflect the committed changes.
abhinavgaba added inline comments.
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
100

Binding and Value are read only inside this LLVM_DEBUG, making them unread if debug messages are disabled.

error: variable ‘Binding’ set but not used [-Werror=unused-but-set-variable]
dblaikie added inline comments.Tue, May 26, 12:21 PM
llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp
100

Usually easier to commit fixes for things like this directly, rather than going through a round of feedback, waiting for someone to fix, etc.

I've fixed this in fca76b79456c916fd2ce193ef76d6e795bd9c105