[PATCH] D97335: [Orc] Add JITLink debug support plugin for ELF x86-64

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

This looks excellent.

I still think that we want to approach this using debug object construction in the long run, but this is too useful to hold up on that point. One thing that I would like to see (and that I think is doable with this scheme as-is) is to move the changes out of JITLink and entirely into Orc / ObjectLinkingLayer. Keeping them out of the deeper levels of the stack (i.e. JITLink) should make them easier to re-do later when we switch to the "right" long term solution.

So, I would be inclined to move `DebugObject` into ORC, and `void notifyMaterializing(jitlink::LinkGraph &G)` and `Expected<std::unique_ptr<DebugObject>> createDebugObject(LinkGraph &G)` from `JITLinkContext` to ObjectLinkingLayerJITLinkContext (the decl for which may need to be hoisted into the ObjectLinkingLayer.h header for now). The new methods should carry comments saying that they're deprecated -- we can leave them in for the purposes of DebugObjectManagerPlugin for now, but we don't want other people to start using them.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/DebugSupport.cpp:60
+    // TODO: Could we use the given buffer as working memory directly
+    // and avoid the unnecessary copy below?
+    Expected<std::unique_ptr<Allocation>> AllocOrErr =
----------------
sgraenitz wrote:
> Is it possible to set an allocation's working memory to an existing buffer? Is it worth adding?
Good question. I ran across another limitation of the existing memory management API today: If you want to mutate a block in a way that changes its size (e.g. prepending or appending) then you need to allocate memory for that in the graph and copy the content into that, only to have to copy all of that back out into working memory later. What you really wanted was a way to say
"reserve me X bytes of additional memory for this block and *I'll* copy the content".

Let's stick to a copy for now, but I think we want to revisit the memory manager API in the not-too-distant-future.


================
Comment at: llvm/lib/ExecutionEngine/Orc/DebugObjectManagerPlugin.cpp:34
+
+  // We should never have more than one pending debug object.
+  ResourceKey Key = getResourceKey(MR);
----------------
sgraenitz wrote:
> This still holds. Can we have more than one resource per MR?
In this context a "resource" is some abstract thing that you manage. You can certainly manage more than one kind of resource on behalf of an MR (e.g. for an ObjC++ object you have allocated memory tracked by ObjectLinkingLayer, eh-frames tracked by the EHFrameRegistrationPlugin, and runtime data tracked by the platform plugin).

The usual idiom is to hold a `DenseMap<ResourceKey, std::vector<ResourceT>>`, where ResourceT is whatever resource type(s) the plugin is managing, which I think is what you're doing.

> // We should never have more than one pending debug object.
Is this true? JITLink supports running multiple concurrent instances -- If the plugin is to support concurrency it should be possible to have more than one debug object in-flight at a time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97335