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

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

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 <https://reviews.llvm.org/rG2e42cc7a50e867d939cac6ee3d375a85a30b984d> 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, 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

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



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF.cpp:30
+
+constexpr char debug_prefix[] = "jitLink_ELF: ";
+
----------------
debug_prefix is currently unused.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:71-72
+      auto Symbols = Obj.symbols(&SecRef);
+      if (errorToBool(std::move(Symbols.takeError())))
+        continue;
+
----------------
This should error out rather than continuing.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:84-86
+        uint8_t Other;
+        uint64_t Value;
+        uint64_t Size;
----------------
Other and Size are unused here.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:172-173
+      auto Symbols = Obj.symbols(&SecRef);
+      if (errorToBool(std::move(Symbols.takeError())))
+        continue;
+
----------------
Ditto here.


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

https://reviews.llvm.org/D79832