[llvm-commits] [patch] Trying to enable EH support in the MCJIT

[Kaylor, Andrew]

Hi Rafael,

This patch generally looks very good.  You did correctly interpret that I meant for only the registration itself to go to the memory manager.

Unfortunately, I don't believe there is any documentation on how the external process support should work.  If you study the 'remote' implementation in lli carefully enough you can get an idea of what it needs to be like.  There is code in LLDB which does actual remote execution of MCJIT emitted code, and it does something similar to what is in lli.

I'm afraid I don't have enough knowledge of MachO to answer your questions there in any depth.  Perhaps Jim can offer some direction.  I would say that creating a wrapper function in the support library is probably the way to go.

Do you know how this works on Windows?  Your patch leaves __register_frame as an unresolved external there.  I don't expect a working implementation there any time soon, but obviously the call to __register_frame needs to be excluded.

The declaration of getEHFrameSection() in RuntimeDyldELF.h, RecordingMemoryManger.h and JITMemoryManager.cpp should be 'virtual'.  This also needs to be added to the RecordingJITMemoryManager class in unittests/ExecutionEngine/JIT/JITTest.cpp

In RuntimeDyldImpl.h, 

+  /// sections. Since we don't run the static linker, this method check if there
+  /// is any tail data that has to appended to emulate that.

should read

+  /// sections. Since we don't run the static linker, this method checks if there
+  /// is any tail data that has to be appended to emulate that.

(check->checks and 'to appended'->'to be appended').

Otherwise, as I said, I think the patch looks great.

Thanks,
Andy

-----Original Message-----
From: Rafael Espíndola [mailto:rafael.espindola at gmail.com] 
Sent: Thursday, December 06, 2012 5:32 PM
To: Kaylor, Andrew
Cc: llvm-commits; Jim Grosbach
Subject: Re: [patch] Trying to enable EH support in the MCJIT

On 4 December 2012 15:06, Kaylor, Andrew <andrew.kaylor at intel.com> wrote:
> Hi Rafael,

Hi Andrew, sorry for taking so long to reply.

> The general problem I have with this patch is that MCJIT and RuntimeDyld need to be able to handle the case where the code being emitted will be run in an external process.  The way you've implemented this seems to be at odds with that goal.  In the 'remote' case, we need a way to call __register_frames in the target process and we shouldn't call it in the host process.  In any event, the frame registration should be deferred until MCJIT::finalizeObject() is called, as the emitted memory may move before then.  We have the same problems with the registerWithDebugger call, and that should also be moved out of MCJIT (which is on my list of things to do).
>
> There is a similar difficulty with resolving external functions.  We're currently handling that by delegating function resolution to the [RuntimeDyld|JIT]MemoryManager.  I've always thought it didn't quite belong there and that instead we need a new object to handle linking issues.  Your EH support would seem to likewise make sense in such an object.
>
> Perhaps in the short term you can add something to the RuntimeDyldMemoryManager interface to register the frames along with a note saying that it should find a new home eventually.

Is there some documentation on how the external process support works?
If I understand you correctly, only the registration itself should go to RTDyldMemoryManager, right? I ask because the memory manages doesn't know the name of the sections, so I added getEHFrameSection to RuntimeDyld.

For adding support for MachO, should I create a sys::RegisterFrame that handles the different way ELF and MachO runtimes work?

Another issue with MachO that I have found so far is that not all section relations are encoded in the relocations. For example, there is no relocation from .eh_frame to .text, we have to know the EH format and patch it. In the attached patch, should this go to the getEHFrame implementation? Should I change resolveRelocations to handle these "implicit" relocations instead?

> That brings me to my second comment, which is that I'd prefer not to have the example code use the DefaultJITMemoryManager if it's going to use MCJIT.  As you know, the plan is for the old JIT stuff to go away when it can and I'm hoping that MCJIT can be completely severed from the old JIT interfaces even sooner.  The RuntimeDyldMemoryManager doesn't currently support setPoisonMemory, but it probably should.  Otherwise, is there any reason that SectionMemoryManager wouldn't work for this ExceptionDemo code?

Looks like it was just missing a call to finalizeObject, so the page permissions were wrong.

> -Andy

Cheers,
Rafael