[MCJIT] Remove eventListeners when MCJIT is destructed

[anna]

Summary

We can register any number of listeners when MCJIT instance is created.
However, we do not remove them when destructing (and we never call the
UnregisterJITEventListener method).

This looks like a memory leak, but I'm not very clear about any other purposes
for the eventListeners.

[lhames]

Hi Anna,

Thanks for looking into this.

The ExecutionEngine::RegisterEventListener documentation specifies that the JIT does not take ownership of the event listener, so freeing the listener is the client's responsibility.

The usual idiom is to allocate the listener as a local variable (if the JIT is used in a single function) or member variable (if the JIT is a class member), then register it. That way the listener is freed with the JIT. If any clients are heap allocating listeners and not freeing them that's definitely a bug.

Cheers,
Lang.

[lhames]

Looking closer, those two lines (EventListners.clear() and Archives.clear()) are totally redundant in a destructor. We should just remove them. :)

• Lang.

[anna]

Updated based on comments above. We can avoid freeing the Archive in the MCJIT destructor.

[anna]

thanks Lang for the quick response! It makes sense now. I think there are other parts in the MCJIT code as well, which follow the same idiom (responsibility on the client). I have changed the code to avoid removing the Archives in the destructor.

[anna]

We dont need the change. The client should handle event listeners. see comment above.

lib/ExecutionEngine/MCJIT/MCJIT.cpp

MCJIT::~MCJIT() {
   MutexGuard locked(lock);
 
   Dyld.deregisterEHFrames();
 
   for (auto &Obj : LoadedObjects)
     if (Obj)
       NotifyFreeingObject(*Obj);
 
-  Archives.clear();
 }
 
 void MCJIT::addModule(std::unique_ptr<Module> M) {
   MutexGuard locked(lock);
 
   if (M->getDataLayout().isDefault())
     M->setDataLayout(getDataLayout());