[MCJIT] Call JIT notifiers only after code sections are ready.

[anarazel]

Summary

Extracted from https://reviews.llvm.org/D44892

Previously JIT notifiers were called before relocations were
performed (leading to ominious looking function calls of "0" being
shown in debugger / profiles), and before memory marked
executable (confusing some profilers).

Move notifications to finalizeLoadedModules().

[lhames]

This patch doesn't affect any of my use-cases, but it's possible that some clients are relying on the fact that memory will still be writable at this point.

As an alternative: ORC's RTDyldObjectLinkingLayer has a NotifyFinalized callback that is called in the equivalent location (i.e. after finalizeMemory has been called). If we want to maintain behavior for existing MCJIT clients it seems reasonable to add a matching "NotifyFinalized" callback to MCJIT.

[anarazel]

This patch doesn't affect any of my use-cases, but it's possible that some clients are relying on the fact that memory will still be writable at this point.

Would that ever be a safe thing to do? Even if, I've a hard time seeing when that'd be a reasonable solution. Not that hacks necessarily have to be reasonable. The other in-tree users of event listeners at least don't care...

As an alternative: ORC's RTDyldObjectLinkingLayer has a NotifyFinalized callback that is called in the equivalent location (i.e. after finalizeMemory has been called). If we want to maintain behavior for existing MCJIT clients it seems reasonable to add a matching "NotifyFinalized" callback to MCJIT.

That'd make sense. OTOH, afaict it'd require changing all the existing notifier users, because they all would want the new thing.

Now that https://reviews.llvm.org/D44890 has landed, I care a bit less. I was mostly bothered because without either D44890 or this, I was unable to test D44892.

[lhames]

In D47343#1111668, @anarazel wrote:

This patch doesn't affect any of my use-cases, but it's possible that some clients are relying on the fact that memory will still be writable at this point.

Would that ever be a safe thing to do? Even if, I've a hard time seeing when that'd be a reasonable solution. Not that hacks necessarily have to be reasonable. The other in-tree users of event listeners at least don't care...

I have backwards compatibility with unreasonable hacks in mind here. :)

As an alternative: ORC's RTDyldObjectLinkingLayer has a NotifyFinalized callback that is called in the equivalent location (i.e. after finalizeMemory has been called). If we want to maintain behavior for existing MCJIT clients it seems reasonable to add a matching "NotifyFinalized" callback to MCJIT.

That'd make sense. OTOH, afaict it'd require changing all the existing notifier users, because they all would want the new thing.

Now that https://reviews.llvm.org/D44890 has landed, I care a bit less. I was mostly bothered because without either D44890 or this, I was unable to test D44892.

Ok. If this is no longer urgent I'll close this revision and revisit the alternative (adding NotifyFinalized) in the future. Does that sound ok to you?

lib/ExecutionEngine/MCJIT/MCJIT.h

class MCJIT : public ExecutionEngine {
   std::vector<JITEventListener*> EventListeners;
 
   OwningModuleContainer OwnedModules;
 
   SmallVector<object::OwningBinary<object::Archive>, 2> Archives;
   SmallVector<std::unique_ptr<MemoryBuffer>, 2> Buffers;
 
   SmallVector<std::unique_ptr<object::ObjectFile>, 2> LoadedObjects;
+  SmallVector<object::ObjectFile*, 2> PendingLoadedObjects;
+  SmallVector<std::unique_ptr<RuntimeDyld::LoadedObjectInfo>, 2> PendingLoadedObjectInfos;
 
   // An optional ObjectCache to be notified of compiled objects and used to
   // perform lookup of pre-compiled code to avoid re-compilation.
   ObjectCache *ObjCache;
 
   Function *FindFunctionNamedInModulePtrSet(StringRef FnName,
                                             ModulePtrSet::iterator I,
                                             ModulePtrSet::iterator E);

lib/ExecutionEngine/MCJIT/MCJIT.cpp

if (!LoadedObject) {
     report_fatal_error(Buf);
   }
   std::unique_ptr<RuntimeDyld::LoadedObjectInfo> L =
     Dyld.loadObject(*LoadedObject.get());
 
   if (Dyld.hasError())
     report_fatal_error(Dyld.getErrorString());
 
-  NotifyObjectEmitted(*LoadedObject.get(), *L);
-
+  // Can't call notifiers yet, as relocations have not yet been
+  // performed, and memory hasn't been marked executable.
+  PendingLoadedObjects.push_back(LoadedObject->get());
+  PendingLoadedObjectInfos.push_back(std::move(L));
   Buffers.push_back(std::move(ObjectToLoad));
   LoadedObjects.push_back(std::move(*LoadedObject));
 
   OwnedModules.markModuleAsLoaded(M);
 }
 
 void MCJIT::finalizeLoadedModules() {
   MutexGuard locked(lock);
 
   // Resolve any outstanding relocations.
   Dyld.resolveRelocations();
 
   OwnedModules.markAllLoadedModulesAsFinalized();
 
   // Register EH frame data for any module we own which has been loaded
   Dyld.registerEHFrames();
 
   // Set page permissions.
   MemMgr->finalizeMemory();
+
+  // Notify listeners about loaded objects, now that memory is marked
+  // executable and relocations have been performed.
+  for (size_t i = 0; i < PendingLoadedObjects.size(); i++) {
+    auto &Obj = PendingLoadedObjects[i];
+    auto &Info = PendingLoadedObjectInfos[i];
+    NotifyObjectEmitted(*Obj, *Info);
+  }
+  PendingLoadedObjects.clear();
+  PendingLoadedObjectInfos.clear();
 }
 
 // FIXME: Rename this.
 void MCJIT::finalizeObject() {
   MutexGuard locked(lock);
 
   // Generate code for module is going to move objects out of the 'added' list,
   // so we need to copy that out before using it: