[MCJIT] Add a FindGlobalVariableNamed utility

[loladiro]

Summary

This adds FindGlobalVariableNamed to ExecutionEngine (plus implementation in MCJIT), which is an analog of FindFunctionNamed for GlobalVariables.

[lhames]

Hi Keno,

The FindGlobalVariableNamed part looks good. Could you add a unit test?

Normally the loaded object info should be captured by adding an EventListener. Would it be possible for you to use that? (Or is that mechanism broken in some way?). As much as reasonably possible I want to keep the MCJIT interface in check.

[loladiro]

Sure, I'll add a test. The use case for the LoadObjectFile method was to hand off a single object file that I generated and be able to look at it. Adding an event listener didn't really work because I don't know which of the hundreds of events corresponded to the object file I want. I suppose I could after the fact query the execution engine for the address of one of the symbols in the module and look through a map of addresses to object files I create in an event listener - but that just seemed like too much effort for information which is actually readily available, but just gets discarded.

[lhames]

When you call addObjectFile, MCJIT will call NotifyObjectEmitted immediately on each registered event listener, passing a reference to the original ObjectFile, and to the LoadedObjectInfo. If you specifically want to inspect the object file currently being loaded you could raise a flag on your event listener:

MyEventListener.NextObjectIsSpecial = true;
EE->addObjectFile(...);

I'm not a fan of callbacks in general, and I agree that this is a bit ugly. However, I prefer callbacks for handling loaded objects in MCJIT because they encourage you to treat JIT'd objects and precompiled ones uniformly, which is usually what you want.

If you feel strongly about this I don't mind the method being added, as long as it's not exposed via the C API.

Shameless plug: In Orc you could add trivial custom layer for this. ;)

[loladiro]

Wouldn't that fail if the loaded object contained a dependency on a symbol in another loaded, but not-yet-codegened module?

[lhames]

No - Loaded doesn't imply finalized, and you don't have to resolve dependencies until finalization.

[loladiro]

Right, ok. That does seem like a viable strategy then. I'll add the unit test and drop the LoadObjectFile functions.

[loladiro]

Updated as discussed.

[lhames]

Thanks Keno. Looks good to me!

include/llvm/ExecutionEngine/ExecutionEngine.h

public:
   //===--------------------------------------------------------------------===//
 
   const DataLayout *getDataLayout() const { return DL; }
 
   /// removeModule - Remove a Module from the list of modules.  Returns true if
   /// M is found.
   virtual bool removeModule(Module *M);
 
-  /// FindFunctionNamed - Search all of the active modules to find the one that
+  /// FindFunctionNamed - Search all of the active modules to find the function that
   /// defines FnName.  This is very slow operation and shouldn't be used for
   /// general code.
   virtual Function *FindFunctionNamed(const char *FnName);
 
+  /// FindGlobalVariableNamed - Search all of the active modules to find the global variable
+  /// that defines Name.  This is very slow operation and shouldn't be used for
+  /// general code.
+  virtual GlobalVariable *FindGlobalVariableNamed(const char *Name, bool AllowInternal = false);
+
   /// runFunction - Execute the specified function with the specified arguments,
   /// and return the result.
   virtual GenericValue runFunction(Function *F,
                                    ArrayRef<GenericValue> ArgValues) = 0;
 
   /// getPointerToNamedFunction - This method returns the address of the
   /// specified function by using the dlsym function call.  As such it is only
   /// useful for resolving library symbols, not code generated symbols.

lib/ExecutionEngine/ExecutionEngine.cpp

Function *ExecutionEngine::FindFunctionNamed(const char *FnName) {
   for (unsigned i = 0, e = Modules.size(); i != e; ++i) {
     Function *F = Modules[i]->getFunction(FnName);
     if (F && !F->isDeclaration())
       return F;
   }
   return nullptr;
 }
 
+GlobalVariable *ExecutionEngine::FindGlobalVariableNamed(const char *Name, bool AllowInternal) {
+  for (unsigned i = 0, e = Modules.size(); i != e; ++i) {
+    GlobalVariable *GV = Modules[i]->getGlobalVariable(Name,AllowInternal);
+    if (GV && !GV->isDeclaration())
+      return GV;
+  }
+  return nullptr;
+}
 
 uint64_t ExecutionEngineState::RemoveMapping(StringRef Name) {
   GlobalAddressMapTy::iterator I = GlobalAddressMap.find(Name);
   uint64_t OldVal;
 
   // FIXME: This is silly, we shouldn't end up with a mapping -> 0 in the
   // GlobalAddressMap.
   if (I == GlobalAddressMap.end())

lib/ExecutionEngine/MCJIT/MCJIT.h

class MCJIT : public ExecutionEngine {
   // 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(const char *FnName,
                                             ModulePtrSet::iterator I,
                                             ModulePtrSet::iterator E);
 
+  GlobalVariable *FindGlobalVariableNamedInModulePtrSet(const char *Name,
+                                                        bool AllowInternal,
+                                                        ModulePtrSet::iterator I,
+                                                        ModulePtrSet::iterator E);
+
   void runStaticConstructorsDestructorsInModulePtrSet(bool isDtors,
                                                       ModulePtrSet::iterator I,
                                                       ModulePtrSet::iterator E);
 
 public:
   ~MCJIT() override;
 
   /// @name ExecutionEngine interface implementation
   /// @{
   void addModule(std::unique_ptr<Module> M) override;
   void addObjectFile(std::unique_ptr<object::ObjectFile> O) override;
   void addObjectFile(object::OwningBinary<object::ObjectFile> O) override;
   void addArchive(object::OwningBinary<object::Archive> O) override;
   bool removeModule(Module *M) override;
 
-  /// FindFunctionNamed - Search all of the active modules to find the one that
+  /// FindFunctionNamed - Search all of the active modules to find the function that
   /// defines FnName.  This is very slow operation and shouldn't be used for
   /// general code.
-  Function *FindFunctionNamed(const char *FnName) override;
+  virtual Function *FindFunctionNamed(const char *FnName) override;
+
+  /// FindGlobalVariableNamed - Search all of the active modules to find the global variable
+  /// that defines Name.  This is very slow operation and shouldn't be used for
+  /// general code.
+  virtual GlobalVariable *FindGlobalVariableNamed(const char *Name, bool AllowInternal = false) override;
 
   /// Sets the object manager that MCJIT should use to avoid compilation.
   void setObjectCache(ObjectCache *manager) override;
 
   void setProcessAllSections(bool ProcessAllSections) override {
     Dyld.setProcessAllSections(ProcessAllSections);
   }
 

lib/ExecutionEngine/MCJIT/MCJIT.cpp

Function *MCJIT::FindFunctionNamedInModulePtrSet(const char *FnName,
   for (; I != E; ++I) {
     Function *F = (*I)->getFunction(FnName);
     if (F && !F->isDeclaration())
       return F;
   }
   return nullptr;
 }
 
+GlobalVariable *MCJIT::FindGlobalVariableNamedInModulePtrSet(const char *Name,
+                                                             bool AllowInternal,
+                                                             ModulePtrSet::iterator I,
+                                                             ModulePtrSet::iterator E) {
+  for (; I != E; ++I) {
+    GlobalVariable *GV = (*I)->getGlobalVariable(Name, AllowInternal);
+    if (GV && !GV->isDeclaration())
+      return GV;
+  }
+  return nullptr;
+}
+
+
 Function *MCJIT::FindFunctionNamed(const char *FnName) {
   Function *F = FindFunctionNamedInModulePtrSet(
       FnName, OwnedModules.begin_added(), OwnedModules.end_added());
   if (!F)
     F = FindFunctionNamedInModulePtrSet(FnName, OwnedModules.begin_loaded(),
                                         OwnedModules.end_loaded());
   if (!F)
     F = FindFunctionNamedInModulePtrSet(FnName, OwnedModules.begin_finalized(),
                                         OwnedModules.end_finalized());
   return F;
 }
 
+GlobalVariable *MCJIT::FindGlobalVariableNamed(const char *Name, bool AllowInternal) {
+  GlobalVariable *GV = FindGlobalVariableNamedInModulePtrSet(
+      Name, AllowInternal, OwnedModules.begin_added(), OwnedModules.end_added());
+  if (!GV)
+    GV = FindGlobalVariableNamedInModulePtrSet(Name, AllowInternal, OwnedModules.begin_loaded(),
+                                        OwnedModules.end_loaded());
+  if (!GV)
+    GV = FindGlobalVariableNamedInModulePtrSet(Name, AllowInternal, OwnedModules.begin_finalized(),
+                                        OwnedModules.end_finalized());
+  return GV;
+}
+
 GenericValue MCJIT::runFunction(Function *F, ArrayRef<GenericValue> ArgValues) {
   assert(F && "Function *F was null at entry to run()");
 
   void *FPtr = getPointerToFunction(F);
   assert(FPtr && "Pointer to fn's code was null after getPointerToFunction");
   FunctionType *FTy = F->getFunctionType();
   Type *RetTy = FTy->getReturnType();
 

unittests/ExecutionEngine/MCJIT/MCJITMultipleModuleTest.cpp

 }
 
 // TODO:
 // Module A { Extern Global GVB, Global Variable GVA, Function FA loads GVB },
 // Module B { Extern Global GVA, Global Variable GVB, Function FB loads GVA },
 
 
 // Module A { Global Variable GVA, Function FA loads GVA },
-// Module B { Global Variable GVB, Function FB loads GVB },
-// execute FB then FA
+// Module B { Global Variable GVB, Internal Global GVC, Function FB loads GVB },
+// execute FB then FA, also check that the global variables are properly accesible
+// through the ExecutionEngine APIs
 TEST_F(MCJITMultipleModuleTest, two_module_global_variables_case) {
   SKIP_UNSUPPORTED_PLATFORM;
 
   std::unique_ptr<Module> A, B;
   Function *FA, *FB;
-  GlobalVariable *GVA, *GVB;
+  GlobalVariable *GVA, *GVB, *GVC;
   A.reset(createEmptyModule("A"));
   B.reset(createEmptyModule("B"));
 
   int32_t initialNum = 7;
   GVA = insertGlobalInt32(A.get(), "GVA", initialNum);
   GVB = insertGlobalInt32(B.get(), "GVB", initialNum);
   FA = startFunction<int32_t(void)>(A.get(), "FA");
   endFunctionWithRet(FA, Builder.CreateLoad(GVA));
   FB = startFunction<int32_t(void)>(B.get(), "FB");
   endFunctionWithRet(FB, Builder.CreateLoad(GVB));
 
+  GVC = insertGlobalInt32(B.get(), "GVC", initialNum);
+  GVC->setLinkage(GlobalValue::InternalLinkage);
+
   createJIT(std::move(A));
   TheJIT->addModule(std::move(B));
 
+  EXPECT_EQ(GVA, TheJIT->FindGlobalVariableNamed("GVA"));
+  EXPECT_EQ(GVB, TheJIT->FindGlobalVariableNamed("GVB"));
+  EXPECT_EQ(GVC, TheJIT->FindGlobalVariableNamed("GVC",true));
+  EXPECT_EQ(NULL, TheJIT->FindGlobalVariableNamed("GVC"));
+
   uint64_t FBPtr = TheJIT->getFunctionAddress(FB->getName().str());
   TheJIT->finalizeObject();
   EXPECT_TRUE(0 != FBPtr);
   int32_t(*FuncPtr)(void) = (int32_t(*)(void))FBPtr;
   EXPECT_EQ(initialNum, FuncPtr())
     << "Invalid value for global returned from JITted function in module B";
 
   uint64_t FAPtr = TheJIT->getFunctionAddress(FA->getName().str());