[llvm-commits] [PATCH] Add basic ELF Dyld loader (on behalf of Andy Kaylor)
Jim Grosbach
grosbach at apple.com
Mon Jan 16 10:52:45 PST 2012
On Jan 13, 2012, at 2:25 PM, Malea, Daniel wrote:
> Hi Jim, thanks a bunch for your review!
>
> If anyone is curious, the attached patch against the latest SVN implements all of Jim's and Rafael's review comments. There are 31 passes and 16 expected failures in the ExecutionEngine tests when lit is run with "--param jit_impl=mcjit".
>
> Regarding the address/endianness issues you've raised, my colleagues are working on a proper patch to ELFObjectFile that takes that into account. We should have it ready for review soon.
>
> However, I thought I would continue the discussion regarding your last point Jim:
>
> | @@ -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.
>
> I was removing the Objective-C mangled name logic because it interferes with the execution of clang-emitted C/C++ bitcode through lli; basically local function calls will not work (i.e. test-call-no-external-funcs.ll). I hadn't realized LLDB depends on MCJIT as well... Nonetheless, we do need to figure out a way to handle this situation in the future. Here's what I propose:
Can you elaborate? I agree the code there is (more than) a bit jacky; however, it shouldn't interfere with anything else. It tries an unmodified symbol name first, which is what the changed code does exclusively.
That said, I'm planning to get rid of that code path entirely in the next day or two, so it probably doesn't matter that much.
>
> 1. Rename the existing "MCJITMemoryManager" to "ObjCMCJITMemoryManager" and add a new class (with the Objective-C logic removed) called "MCJITMemoryManager".
> 2. Add logic to EngineBuilder and MCJIT as required to allow toggling between the two. LLDB would use this toggle to switch to the ObjC version and lli could use the default.
>
> I thought I would float this very rudimentary approach on the mailing list to see if anyone has any strong feelings before I jump into implementing it though...
I'd suggest holding off for a bit. As I mentioned, the start/endFunction path is going away soon. It doesn't work well for the MCJIT path.
-j
>
> Dan
>
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Jim Grosbach
> Sent: Thursday, January 12, 2012 22:20
> To: Malea, Daniel
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Add basic ELF Dyld loader (on behalf of Andy Kaylor)
>
> 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>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> <0001-basic-runtimedyld-elf-loader_v4.patch>
More information about the llvm-commits
mailing list