[PATCH] D79832: Initial commit for the elf x86 implementation for jitlink

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

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, at function
  main:
          movl    $42, %eax
          retq
  .Lfunc_end0:
          .size   main, .Lfunc_end0-main
  
          .ident  "clang version 10.0.0-4ubuntu1 "
          .section        ".note.GNU-stack","", at progbits
          .addrsig

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



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:160
+
+    /// We only have 256 section indexes: Use a vector rather than a map.
+    std::vector<std::vector<object::ELFFile<object::ELF64LE>::Elf_Shdr_Range *>>
----------------
MachO only has 256 sections indexes (the section index field is 8-bit), but I thought ELF had more?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:244-255
+        /*
+          G->addAbsoluteSymbol(*Name, SymRef.getValue(), SymRef.st_size,
+          Linkage::Strong, Scope::Default, false);
+
+          if(SymRef.isCommon()) {
+            G->addCommonSymbol(*Name, Scope::Default, getCommonSection(), 0, 0,
+          SymRef.getValue(), false);
----------------
This could use a clear TODO comment.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:270-272
+    if (!isRelocatable()) {
+      return make_error<JITLinkError>("Object is not a relocatable ELF");
+    }
----------------
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");


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:320-322
+  Error applyFixup(Block &B, const Edge &E, char *BlockWorkingMem) const {
+    return Error::success();
+  }
----------------
This should probably have a "TODO: add relocation handling" comment, even if it's obvious.


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

https://reviews.llvm.org/D79832