[ExecutionEngine] Track objects using an abstract ObjectKey in JITEventListeners.

[lhames]

Summary

The aim of this patch is to enable JITEventListeners to work with ORC without
forcing us to keep object files alive longer than necessary.

Currently JITEventListener::NotifyObjectEmitted and NotifyObjectFreed take a
reference to the object file being emitted, but in NotifyObjectFreed this is
only ever used as a key to deallocate resources that were allocated in
NotifyObjectEmitted. Switching to an abstract key allows the JIT to deallocate
object file buffers after NotifyObjectEmitted has been called.

No guarantee is made about the key value, except that it will be distinct for
each object that has been allocated. Keys may be re-used after they are freed.

[lhames]

• [ExecutionEngine] Rename/format JITEventListener methods.

[anarazel]

This looks like a sensible improvement. There's probably not many implementors of the interface, so the renaming shouldn't cause much pain.

I noticed however that you didn't update ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp?

lib/ExecutionEngine/MCJIT/MCJIT.cpp

653

Perhaps this should rather live inside ObjectFile? No strong opinion, just pondering. Seems a bit prettier than having the ugly multiple cast in multiple places.

[lhames]

In D53773#1280967, @anarazel wrote:

This looks like a sensible improvement. There's probably not many implementors of the interface, so the renaming shouldn't cause much pain.

I noticed however that you didn't update ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp?

Oops -- I totally missed that. I will fix it and post an updated patch tomorrow.

lib/ExecutionEngine/MCJIT/MCJIT.cpp

653

Unfortunately this can not be moved to ObjectFile: For MCJIT it makes sense to continue using the original ObjectFile address as the key, but for ORC the aim is to decouple the key from the ObjectFile so that the latter can be deallocated.

[andrew.w.kaylor]

include/llvm/ExecutionEngine/JITEventListener.h

52

The comment is still using the old name here.

[lhames]

Updates the PerfJITEventListener API and fixes a comment that referred to the old API.

[lhames]

Gentle ping.

This is not high priority yet, but if there are no further issues with it I would like to land it as I intend to build on it in the future.

[anarazel]

Gentle ping.

This is not high priority yet, but if there are no further issues with it I would like to land it as I intend to build on it in the future.

Sorry, missed the updated version. I don't know if my ack is sufficient to move this forward, but the update certainly fixes the complaint I had. And the rest looks reasonable.

[andrew.w.kaylor]

Looks good to me too.

[lhames]

Committed in r348223.

include/llvm/ExecutionEngine/JITEventListener.h

 /// The default implementation of each method does nothing.
 class JITEventListener {
 public:
+  using ObjectKey = uint64_t;
+
   JITEventListener() = default;
   virtual ~JITEventListener() = default;
 
-  /// NotifyObjectEmitted - Called after an object has been successfully
-  /// emitted to memory.  NotifyFunctionEmitted will not be called for
+  /// notifyObjectLoaded - Called after an object has had its sections allocated
+  /// and addresses assigned to all symbols. Note: Section memory will not have
+  /// been relocated yet. notifyFunctionLoaded will not be called for
   /// individual functions in the object.
   ///
   /// ELF-specific information
   /// The ObjectImage contains the generated object image
   /// with section headers updated to reflect the address at which sections
   /// were loaded and with relocations performed in-place on debug sections.
-  virtual void NotifyObjectEmitted(const object::ObjectFile &Obj,
-                                   const RuntimeDyld::LoadedObjectInfo &L) {}
+  virtual void notifyObjectLoaded(ObjectKey K, const object::ObjectFile &Obj,
 
-  /// NotifyFreeingObject - Called just before the memory associated with
+  /// notifyFreeingObject - Called just before the memory associated with
   /// a previously emitted object is released.
-  virtual void NotifyFreeingObject(const object::ObjectFile &Obj) {}
+  virtual void notifyFreeingObject(ObjectKey K) {}
 
   // Get a pointe to the GDB debugger registration listener.
   static JITEventListener *createGDBRegistrationListener();

lib/ExecutionEngine/GDBRegistrationListener.cpp

 };
 
 // Buffer for an in-memory object file in executable memory
-typedef llvm::DenseMap< const char*, RegisteredObjectInfo>
+typedef llvm::DenseMap<JITEventListener::ObjectKey, RegisteredObjectInfo>
     RegisteredObjectBufferMap;
 
 /// Global access point for the JIT debugging interface designed for use with a
 /// singleton toolbox. Handles thread-safe registration and deregistration of

   /// Creates an entry in the JIT registry for the buffer @p Object,
   /// which must contain an object file in executable memory with any
   /// debug information for the debugger.
-  void NotifyObjectEmitted(const ObjectFile &Object,
-                           const RuntimeDyld::LoadedObjectInfo &L) override;
+  void notifyObjectLoaded(ObjectKey K, const ObjectFile &Obj,
 
   /// Removes the internal registration of @p Object, and
   /// frees associated resources.
   /// Returns true if @p Object was found in ObjectBufferMap.
-  void NotifyFreeingObject(const ObjectFile &Object) override;
+  void notifyFreeingObject(ObjectKey K) override;
 
 private:
   /// Deregister the debug info for the given object file from the debugger

   ObjectBufferMap.clear();
 }
 
-void GDBJITRegistrationListener::NotifyObjectEmitted(
-                                       const ObjectFile &Object,
-                                       const RuntimeDyld::LoadedObjectInfo &L) {
+void GDBJITRegistrationListener::notifyObjectLoaded(
+    ObjectKey K, const ObjectFile &Obj,
 
-  OwningBinary<ObjectFile> DebugObj = L.getObjectForDebug(Object);
+  OwningBinary<ObjectFile> DebugObj = L.getObjectForDebug(Obj);
 
   // Bail out if debug objects aren't supported.
   if (!DebugObj.getBinary())

   const char *Buffer = DebugObj.getBinary()->getMemoryBufferRef().getBufferStart();
   size_t      Size = DebugObj.getBinary()->getMemoryBufferRef().getBufferSize();
 
-  const char *Key = Object.getMemoryBufferRef().getBufferStart();
-
-  assert(Key && "Attempt to register a null object with a debugger.");
   llvm::MutexGuard locked(*JITDebugLock);
-  assert(ObjectBufferMap.find(Key) == ObjectBufferMap.end() &&
+  assert(ObjectBufferMap.find(K) == ObjectBufferMap.end() &&
          "Second attempt to perform debug registration.");
   jit_code_entry* JITCodeEntry = new jit_code_entry();
 

     JITCodeEntry->symfile_addr = Buffer;
     JITCodeEntry->symfile_size = Size;
 
-    ObjectBufferMap[Key] = RegisteredObjectInfo(Size, JITCodeEntry,
-                                                std::move(DebugObj));
+    ObjectBufferMap[K] =
+        RegisteredObjectInfo(Size, JITCodeEntry, std::move(DebugObj));
     NotifyDebugger(JITCodeEntry);
   }
 }
 
-void GDBJITRegistrationListener::NotifyFreeingObject(const ObjectFile& Object) {
-  const char *Key = Object.getMemoryBufferRef().getBufferStart();
+void GDBJITRegistrationListener::notifyFreeingObject(ObjectKey K) {
   llvm::MutexGuard locked(*JITDebugLock);
-  RegisteredObjectBufferMap::iterator I = ObjectBufferMap.find(Key);
+  RegisteredObjectBufferMap::iterator I = ObjectBufferMap.find(K);
 
   if (I != ObjectBufferMap.end()) {
     deregisterObjectInternal(I);

lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp

   typedef DenseMap<const void *, MethodAddressVector>  ObjectMap;
 
   ObjectMap  LoadedObjectMap;
-  std::map<const char*, OwningBinary<ObjectFile>> DebugObjects;
+  std::map<ObjectKey, OwningBinary<ObjectFile>> DebugObjects;
 
 public:
   IntelJITEventListener(IntelJITEventsWrapper* libraryWrapper) {

   ~IntelJITEventListener() {
   }
 
-  void NotifyObjectEmitted(const ObjectFile &Obj,
-                           const RuntimeDyld::LoadedObjectInfo &L) override;
+  void notifyObjectLoaded(ObjectKey Key, const ObjectFile &Obj,
 
-  void NotifyFreeingObject(const ObjectFile &Obj) override;
+  void notifyFreeingObject(ObjectKey Key) override;
 };
 
 static LineNumberInfo DILineInfoToIntelJITFormat(uintptr_t StartAddress,

   return Result;
 }
 
-void IntelJITEventListener::NotifyObjectEmitted(
-                                       const ObjectFile &Obj,
-                                       const RuntimeDyld::LoadedObjectInfo &L) {
+void IntelJITEventListener::notifyObjectLoaded(
+    ObjectKey Key, const ObjectFile &Obj,
 
   OwningBinary<ObjectFile> DebugObjOwner = L.getObjectForDebug(Obj);
   const ObjectFile *DebugObj = DebugObjOwner.getBinary();

   // registered function addresses for each loaded object.  We will
   // use the MethodIDs map to get the registered ID for each function.
   LoadedObjectMap[ObjData] = Functions;
-  DebugObjects[Obj.getData().data()] = std::move(DebugObjOwner);
+  DebugObjects[Key] = std::move(DebugObjOwner);
 }
 
-void IntelJITEventListener::NotifyFreeingObject(const ObjectFile &Obj) {
+void IntelJITEventListener::notifyFreeingObject(ObjectKey Key) {
   // This object may not have been registered with the listener. If it wasn't,
   // bail out.
-  if (DebugObjects.find(Obj.getData().data()) == DebugObjects.end())
+  if (DebugObjects.find(Key) == DebugObjects.end())
     return;
 
   // Get the address of the object image for use as a unique identifier
-  const ObjectFile &DebugObj = *DebugObjects[Obj.getData().data()].getBinary();
+  const ObjectFile &DebugObj = *DebugObjects[Key].getBinary();
   const void* ObjData = DebugObj.getData().data();
 
   // Get the object's function list from LoadedObjectMap

 
   // Erase the object from LoadedObjectMap
   LoadedObjectMap.erase(OI);
-  DebugObjects.erase(Obj.getData().data());
+  DebugObjects.erase(Key);
 }
 
 }  // anonymous namespace.

lib/ExecutionEngine/MCJIT/MCJIT.h

   /// the future.
   std::unique_ptr<MemoryBuffer> emitObject(Module *M);
 
-  void NotifyObjectEmitted(const object::ObjectFile& Obj,
-                           const RuntimeDyld::LoadedObjectInfo &L);
-  void NotifyFreeingObject(const object::ObjectFile& Obj);
+  void notifyObjectLoaded(const object::ObjectFile &Obj,
+  void notifyFreeingObject(const object::ObjectFile &Obj);
 
   JITSymbol findExistingSymbol(const std::string &Name);
   Module *findModuleForSymbol(const std::string &Name, bool CheckFunctionsOnly);

lib/ExecutionEngine/MCJIT/MCJIT.cpp

 
   for (auto &Obj : LoadedObjects)
     if (Obj)
-      NotifyFreeingObject(*Obj);
+      notifyFreeingObject(*Obj);
 
   Archives.clear();
 }

   if (Dyld.hasError())
     report_fatal_error(Dyld.getErrorString());
 
-  NotifyObjectEmitted(*Obj, *L);
+  notifyObjectLoaded(*Obj, *L);
 
   LoadedObjects.push_back(std::move(Obj));
 }

   if (Dyld.hasError())
     report_fatal_error(Dyld.getErrorString());
 
-  NotifyObjectEmitted(*LoadedObject.get(), *L);
+  notifyObjectLoaded(*LoadedObject.get(), *L);
 
   Buffers.push_back(std::move(ObjectToLoad));
   LoadedObjects.push_back(std::move(*LoadedObject));

   }
 }
 
-void MCJIT::NotifyObjectEmitted(const object::ObjectFile& Obj,
-                                const RuntimeDyld::LoadedObjectInfo &L) {
+void MCJIT::notifyObjectLoaded(const object::ObjectFile &Obj,
+  uint64_t Key =
+      static_cast<uint64_t>(reinterpret_cast<uintptr_t>(Obj.getData().data()));
   MutexGuard locked(lock);
   MemMgr->notifyObjectLoaded(this, Obj);
   for (unsigned I = 0, S = EventListeners.size(); I < S; ++I) {
-    EventListeners[I]->NotifyObjectEmitted(Obj, L);
+    EventListeners[I]->notifyObjectLoaded(Key, Obj, L);
   }
 }
 
-void MCJIT::NotifyFreeingObject(const object::ObjectFile& Obj) {
+void MCJIT::notifyFreeingObject(const object::ObjectFile &Obj) {
+  uint64_t Key =
+      static_cast<uint64_t>(reinterpret_cast<uintptr_t>(Obj.getData().data()));
   MutexGuard locked(lock);
   for (JITEventListener *L : EventListeners)
-    L->NotifyFreeingObject(Obj);
+    L->notifyFreeingObject(Key);
 }
 
 JITSymbol

lib/ExecutionEngine/OProfileJIT/OProfileJITEventListener.cpp

   std::unique_ptr<OProfileWrapper> Wrapper;
 
   void initialize();
-  std::map<const char*, OwningBinary<ObjectFile>> DebugObjects;
+  std::map<ObjectKey, OwningBinary<ObjectFile>> DebugObjects;
 
 public:
   OProfileJITEventListener(std::unique_ptr<OProfileWrapper> LibraryWrapper)

 
   ~OProfileJITEventListener();
 
-  void NotifyObjectEmitted(const ObjectFile &Obj,
-                           const RuntimeDyld::LoadedObjectInfo &L) override;
+  void notifyObjectLoaded(ObjectKey Key, const ObjectFile &Obj,
 
-  void NotifyFreeingObject(const ObjectFile &Obj) override;
+  void notifyFreeingObject(ObjectKey Key) override;
 };
 
 void OProfileJITEventListener::initialize() {

   }
 }
 
-void OProfileJITEventListener::NotifyObjectEmitted(
-                                       const ObjectFile &Obj,
-                                       const RuntimeDyld::LoadedObjectInfo &L) {
+void OProfileJITEventListener::notifyObjectLoaded(
+    ObjectKey Key, const ObjectFile &Obj,
   if (!Wrapper->isAgentAvailable()) {
     return;
   }

     }
   }
 
-  DebugObjects[Obj.getData().data()] = std::move(DebugObjOwner);
+  DebugObjects[Key] = std::move(DebugObjOwner);
 }
 
-void OProfileJITEventListener::NotifyFreeingObject(const ObjectFile &Obj) {
+void OProfileJITEventListener::notifyFreeingObject(ObjectKey Key) {
   if (Wrapper->isAgentAvailable()) {
 
     // If there was no agent registered when the original object was loaded then
     // we won't have created a debug object for it, so bail out.
-    if (DebugObjects.find(Obj.getData().data()) == DebugObjects.end())
+    if (DebugObjects.find(Key) == DebugObjects.end())
       return;
 
-    const ObjectFile &DebugObj = *DebugObjects[Obj.getData().data()].getBinary();
+    const ObjectFile &DebugObj = *DebugObjects[Key].getBinary();
 
     // Use symbol info to iterate functions in the object.
     for (symbol_iterator I = DebugObj.symbol_begin(),

     }
   }
 
-  DebugObjects.erase(Obj.getData().data());
+  DebugObjects.erase(Key);
 }
 
 }  // anonymous namespace.

lib/ExecutionEngine/Orc/OrcCBindingsStack.h

   void notifyFinalized(orc::VModuleKey K,
            const object::ObjectFile &Obj,
            const RuntimeDyld::LoadedObjectInfo &LoadedObjInfo) {
+    uint64_t Key = static_cast<uint64_t>(
+        reinterpret_cast<uintptr_t>(Obj.getData().data()));
     for (auto &Listener : EventListeners)
-      Listener->NotifyObjectEmitted(Obj, LoadedObjInfo);
+      Listener->notifyObjectLoaded(Key, Obj, LoadedObjInfo);
   }
 
   void notifyFreed(orc::VModuleKey K, const object::ObjectFile &Obj) {
+    uint64_t Key = static_cast<uint64_t>(
+        reinterpret_cast<uintptr_t>(Obj.getData().data()));
     for (auto &Listener : EventListeners)
-      Listener->NotifyFreeingObject(Obj);
+      Listener->notifyFreeingObject(Key);
   }
 
   orc::ExecutionSession ES;

lib/ExecutionEngine/PerfJITEvents/PerfJITEventListener.cpp

       CloseMarker();
   }
 
-  void NotifyObjectEmitted(const ObjectFile &Obj,
-                           const RuntimeDyld::LoadedObjectInfo &L) override;
-  void NotifyFreeingObject(const ObjectFile &Obj) override;
+  void notifyObjectLoaded(ObjectKey K, const ObjectFile &Obj,
+  void notifyFreeingObject(ObjectKey K) override;
 
 private:
   bool InitDebuggingDir();

     SuccessfullyInitialized = true;
 }
 
-void PerfJITEventListener::NotifyObjectEmitted(
-    const ObjectFile &Obj, const RuntimeDyld::LoadedObjectInfo &L) {
+void PerfJITEventListener::notifyObjectLoaded(
+    ObjectKey K, const ObjectFile &Obj,
+    const RuntimeDyld::LoadedObjectInfo &L) {
 
   if (!SuccessfullyInitialized)
     return;

   Dumpstream->flush();
 }
 
-void PerfJITEventListener::NotifyFreeingObject(const ObjectFile &Obj) {
+void PerfJITEventListener::notifyFreeingObject(ObjectKey K) {
   // perf currently doesn't have an interface for unloading. But munmap()ing the
   // code section does, so that's ok.
 }