[PATCH] D80613: Getting R_X86_64_PC32 working as our first relocation

Wed May 27 15:16:32 PDT 2020

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

This looks great!

Style issues: There are a number of single line statements with braces around them -- those braces should be moved to match the style guide. You should also clang-format the patch if you haven't already. Otherwise this is good to land.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:196-200
+      // TODO can the elf obj file do this for me?
+      if (SecRef.sh_type == ELF::SHT_REL) {
+        return make_error<llvm::StringError>("Shouldn't have REL in x64",
+                                             llvm::inconvertibleErrorCode());
+      }
----------------
No braces for single-line statements. :)

Side note regarding the TODO: Long term I think it would make sense to do some sort of up-front validation of the input file, but for now this is the right place to check this.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:237
+        }
+        // TODO error handling?
+        auto BlockToFix = *JITSection->blocks().begin();
----------------
For what error condition? Empty sections? Does your parser ever produce them?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80613