[PATCH] D96627: [llvm-jitlink] Implement JITLoaderGDB ObjectLinkingLayer plugin for ELF x86-64

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

sgraenitz added a comment.

Unregistering, failure handling, tests and out-of-process support are still to do. I made some notes and highlighted current open questions inline.

Going forward, I think the most pressing question is testing. LLDB will have an end-to-end test, which checks that a JIT breakpoint in a minimal C++ file gets hit. It will cover all of the mandatory code paths. I am not sure though what's the best way to test cases with multiple MRs, splitting MRs, multi-threading, removing resources, etc. `EHFrameRegistrationPlugin` seems to have coverage for most of its code from LLJIT tests. Maybe that could be an option here too?



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:1320
+  virtual void notifyMaterializing(const jitlink::LinkGraph &G) = 0;
+
   /// Notify this context that linking failed.
----------------
New API function. The ObjectLinkingLayerJITLinkContext forwards it to the plugins. It feels quite symmetric to notifyFailed().


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h:1370
+  }
+
 private:
----------------
New API function. Doesn't give the API client access to the underlying buffer.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/ObjectLinkingLayer.h:75
+                                     const jitlink::JITLinkContext &Ctx) {}
+
     virtual void notifyLoaded(MaterializationResponsibility &MR) {}
----------------
New API function. We don't really need the LinkGraph, but only providing the context seems weird.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:338
-      }
-
       sys::Memory::ProtectionFlags Prot;
----------------
In order to make the plugin usable, we have to "ProcessAllSections". The good news is that the pruning step in linkPhase1 seems to remove all this.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-gdb-loader.cpp:207
+  return Key;
+}
+
----------------
It's quite far away from this patch, but I had a look at JITDylib::delegate(). It splits a MR in two and it looks like they both share one tracker. That might break my code currently. For now, I kept tracking debug objects by ResourceKey here, because I can get them from a MR, but not the other way around.


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-gdb-loader.cpp:290
+  {
+    std::unique_lock<std::mutex> Lock(DebugAllocLock);
+    auto It = PendingDebugAllocs.find(Key);
----------------
TODO: `lock_guard`


================
Comment at: llvm/tools/llvm-jitlink/llvm-jitlink-gdb-loader.cpp:345
+                  DstKey, SrcKey),
+          inconvertibleErrorCode()));
+    } else {
----------------
I can't reach the ExecutionSession here and there is no Error return value. Any chance we can report that?

This is due to the "never have more than one pending debug object" restriction above. Otherwise there is no way to determine the right debug object for a MR in the modifyPassConfig() and notifyLoaded() handlers. Or am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96627