[PATCH] D129936: [JITLink][COFF][x86_64] Reimplement ADDR32NB/REL32.

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added a comment.

That's a really creative solution for a tricky issue and it looks pretty good already! Kudos! Here's first set of questions and remarks.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFF_x86_64.cpp:344
+               if (!LR) {
+                 Err = std::move(LR.takeError());
+                 return;
----------------
Detail: We shouldn't use `std::move` here explicitly. Might prevent copy-elision.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFF_x86_64.cpp:351
+  if (Err)
+    return Err;
+  return ImageBase;
----------------
Detail: I think we have to `std::move(Err)` here into the implicit ctor of `Expected<JITTargetAddress>`


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFF_x86_64.cpp:357
+  LLVM_DEBUG(dbgs() << "Optimizing and lowering COFF x86_64 edges:\n");
+  auto ImageBase = getImageBaseAddress(*Ctx);
+
----------------
This function appears to be the trickiest part of the patch. It's breaking out back into ORC to query the image base address. That's wild! And it might actually work!

The only thing that seems odd is that all COFF tests (and in general every user of the backend?) will need to define the absolute `__ImageBase` symbol now -- even though it's only used in two cases. Instead of calling it upfront here in the optimize function, could we call it on-demand in the specific cases?

I guess you could turn the variable here into a lambda, which would call `getImageBaseAddress()` only once and capture a stack variable to cache the result.. Does that make sense?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFF_x86_64.cpp:403
+  auto TT = (*COFFObj)->makeTriple();
+  TT.setObjectFormat(Triple::ObjectFormatType::COFF);
+  return COFFLinkGraphBuilder_x86_64(**COFFObj, TT).buildGraph();
----------------
What else could it be?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFF_x86_64.cpp:422
+    // Add Stubs optimizer / COFF edge lowering passes.
+    auto CtxPtr = Ctx.get();
+    Config.PreFixupPasses.push_back([CtxPtr](LinkGraph &G) {
----------------
Detail: I am a fan of writing out non-obvious types, but if not let's at least add the `*` please


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

https://reviews.llvm.org/D129936