[llvm-commits] [PATCH] Add basic ELF Dyld loader (on behalf of Andy Kaylor)

Jim Grosbach grosbach at apple.com
Thu Jan 12 12:20:09 PST 2012 

Hi Daniel,

This is really cool. Thanks for working on this! In general, this looks really good. 

A few comments, mostly style, generally only mentioning a single instance of each type for brevity.

Fair bit of trailing whitespace in the patch. Please remove. Similarly, at least one instance of a hard tab in a .cpp/.h file. Use spaces only.

+void getSymbolInfo(symbol_iterator Sym, uint64_t& Addr, StringRef& Name) {

Space before the &, not after. e.g., "uint64_t &Addr". Same for pointers.

+  if (!isCompatibleFormat(InputBuffer)) {
+    return true;
+  }

No compound statement braces needed for single-line statements for 'if'.

+  // Note that storing this in the RuntimeDyld::SymbolTable would have
+  //   unintended side-effects.

Might be worth elaborating a bit on what sort of side effects.

+  symbol_iterator  SymEnd = Obj->end_symbols();

Only one space after the type.

+  switch (RE.Type) {
+    case ELF::R_X86_64_64:
+      {
+        uint8_t **Target = reinterpret_cast<uint8_t**>(TargetAddr);
+        *Target = Addr + RE.Addend;
+      }
+      break;

Case labels shouldn't indent under the switch. Open brace goes on the same line as the label. E.g.,
  switch (RE.Type) {
  case ELF::R_X86_64_64: {
      uint8_t **Target = reinterpret_cast<uint8_t**>(TargetAddr);
      *Target = Addr + RE.Addend;
    }
    break;

+    default:
+      assert(0 && ("Relocation type not implemented yet!"));
+      break;
+  }

Default cases with an assert() like this generally go at the top of the switch statement. We're not always terribly consistent about that in the rest of LLVM, to be fair, though.

+    case ELF::R_386_32:
+      {
+        uint8_t **Target = reinterpret_cast<uint8_t**>(TargetAddr);
+        *Target = Addr + RE.Addend;
+      }

It's intended that the address space where JITed code may not be the address space where the object has been loaded (e.g., remote debugging). Is this safe in that scenario? That said, the memory manager and other stuff doesn't completely handle that cleanly yet, so this is more a "keep this in mind and perhaps add FIXME comments" type thing than anything else for now.

-bool RuntimeDyldMachO::isKnownFormat(const MemoryBuffer *InputBuffer) {
-  StringRef Magic = InputBuffer->getBuffer().slice(0, 4);
-  if (Magic == "\xFE\xED\xFA\xCE") return true;
-  if (Magic == "\xCE\xFA\xED\xFE") return true;
-  if (Magic == "\xFE\xED\xFA\xCF") return true;
-  if (Magic == "\xCF\xFA\xED\xFE") return true;
-  return false;
-}
-

Why did this get changed? It looks like it just inlined the logic. That seems totally orthogonal to adding ELF support.



Index: lib/ExecutionEngine/MCJIT/MCJITMemoryManager.h
===================================================================
--- lib/ExecutionEngine/MCJIT/MCJITMemoryManager.h	(revision 146896)
+++ lib/ExecutionEngine/MCJIT/MCJITMemoryManager.h	(working copy)
@@ -34,15 +34,8 @@
   // a pointer to the allocated memory and update Size to reflect how much
   // memory was acutally allocated.
   uint8_t *startFunctionBody(const char *Name, uintptr_t &Size) {
-    // FIXME: This should really reference the MCAsmInfo to get the global
-    //        prefix.
-    if (Name[0] == '_') ++Name;
     Function *F = M->getFunction(Name);
-    // Some ObjC names have a prefixed \01 in the IR. If we failed to find
-    // the symbol and it's of the ObjC conventions (starts with "-" or 
-    // "+"), try prepending a \01 and see if we can find it that way.
-    if (!F && (Name[0] == '-' || Name[0] == '+'))
-      F = M->getFunction((Twine("\1") + Name).str());
+
     assert(F && "No matching function in JIT IR Module!");
     return JMM->startFunctionBody(F, Size);
   }
@@ -51,15 +44,8 @@
   // memory was actually used.
   void endFunctionBody(const char *Name, uint8_t *FunctionStart,
                        uint8_t *FunctionEnd) {
-    // FIXME: This should really reference the MCAsmInfo to get the global
-    //        prefix.
-    if (Name[0] == '_') ++Name;
     Function *F = M->getFunction(Name);
-    // Some ObjC names have a prefixed \01 in the IR. If we failed to find
-    // the symbol and it's of the ObjC conventions (starts with "-" or
-    // "+"), try prepending a \01 and see if we can find it that way.
-    if (!F && (Name[0] == '-' || Name[0] == '+'))
-      F = M->getFunction((Twine("\1") + Name).str());
+
     assert(F && "No matching function in JIT IR Module!");
     JMM->endFunctionBody(F, FunctionStart, FunctionEnd);
   }

Why is this code being removed? Doing so will break LLDB debugging of ObjC.

Thanks again. OK to commit with the above changes.

-Jim

On Dec 19, 2011, at 12:16 PM, Malea, Daniel wrote:

> Hi all,
> 
> Thanks everyone for your input; here's an updated patch with passing tests! In addition to running the existing ExecutionEngine tests against MCJIT, we added a few new test cases. To run them against the MCJIT implementation, invoke lit with the "--param jit_impl=mcjit" option, which then causes lli to be invoked with "-use-mcjit".
> 
> Compared to the last iteration of the patch, here’s exactly what has changed:
> -	Removed "default:" case in switch statement as per Rafael’s suggestion
> -	Moved RuntimeDyldMachO::isKnownFormat() into isCompatibleFormat() of the same class
> -	Added 6 new test cases in test/ExecutionEngine
> -	Added “XFAIL: mcjit” to tests we expect not to pass 
> -	Updated lit.cfg and existing ExecutionEngine tests to handle the jit_impl parameter
> -	Updated lli to use the default memory manager (to enable MCJIT to even run)
> -	Removed Objective-C specific name handling behavior from the MCJIT memory manager
> 
> Regarding the last point, I'm not really confident that just removing the objective-c specific behavior is correct, but it seems like language specific things should probably not go in something called "MCJITMemoryManager". Perhaps Jim Grosbach can comment as he is on the commit logs for the code being touched. Maybe we should rename the existing implementation to something like ObjCMCJITMemoryManager?
> 
> Regarding the tests, the code isn't quite robust enough to enable all the existing ExecutionEngine tests to pass due to known issues with external function calls and globals; but nonetheless the following existing tests do pass, at least on (Ubuntu) Linux ia64:
> 
> (existing tests)
> test/ExecutionEngine/2003-01-04-PhiTest.ll
> test/ExecutionEngine/2003-01-09-SARTest.ll
> test/ExecutionEngine/2003-01-10-FUCOM.ll
> test/ExecutionEngine/2003-01-15-AlignmentTest.ll
> test/ExecutionEngine/2003-05-11-PHIRegAllocBug.ll
> test/ExecutionEngine/2003-06-04-bip2-bug.ll
> test/ExecutionEngine/2003-06-05-PHIBug.ll
> test/ExecutionEngine/2003-08-15-AllocaAssertion.ll
> test/ExecutionEngine/2003-08-23-RegisterAllocatePhysReg.ll
> test/ExecutionEngine/2003-10-18-PHINode-ConstantExpr-CondCode-Failure.ll
> test/ExecutionEngine/2005-12-02-TailCallBug.ll
> test/ExecutionEngine/simplesttest.ll
> test/ExecutionEngine/simpletest.ll
> test/ExecutionEngine/test-arith.ll
> test/ExecutionEngine/test-branch.ll
> test/ExecutionEngine/test-cast.ll
> test/ExecutionEngine/test-constantexpr.ll
> test/ExecutionEngine/test-loadstore.ll
> test/ExecutionEngine/test-logical.ll
> test/ExecutionEngine/test-loop.ll
> test/ExecutionEngine/test-phi.ll
> test/ExecutionEngine/test-ret.ll
> test/ExecutionEngine/test-setcond-fp.ll
> test/ExecutionEngine/test-setcond-int.ll
> test/ExecutionEngine/test-shift.ll
> test/ExecutionEngine/test-setcond-fp.ll
> 
> (new tests)
> test/ExecutionEngine/test-call-no-external-funcs.ll
> test/ExecutionEngine/test-local.ll
> test/ExecutionEngine/test-return.ll
> 
> Thanks,
> Dan
> 
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Rafael Ávila de Espíndola
> Sent: Saturday, December 17, 2011 12:32 PM
> To: Kaylor, Andrew
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Add basic ELF Dyld loader (on behalf of Andy Kaylor)
> 
> On 14/12/11 04:28 PM, Kaylor, Andrew wrote:
>> Hi Rafael,
>> 
>> I'll remove the 'default' (per Eric's preference).
>> 
>> This will run a basic program.  I believe this version can even call 
>> functions JITted in the same module.  We have a couple of
>> lit+lli-based tests, which I believe will be submitted soon.
> 
> Cool, can you send an updated patch? If you can include at least one basic test in the patch, that would be perfect.
> 
>> Thanks, Andy
>> 
> 
> Thanks,
> Rafael
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> <0001-basic-runtimedyld-elf-loader_v3.patch>

More information about the llvm-commits mailing list