[PATCH] Substantial RuntimeDyldMachO cleanup.

[Lang Hames]

Ping?

I know Andy is out on vacation, but do any other MCJIT people have any
thoughts on this?

Cheers,
Lang.



On Thu, Jun 12, 2014 at 1:34 PM, Lang Hames <lhames at gmail.com> wrote:

> Hi All,
>
> Attached is a patch that I think substantially cleans up RuntimeDyldMachO
> and, if the general direction is agreed with, could be used as a template
> for cleaning up RuntimeDyldELF.
>
> At the moment all MCJIT relocation handling logic for MachO is contained
> in the RuntimeDyldMachO class (see
> lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.{h,cpp}). Mixing the logic
> for all different MachO targets in a single class creates two problems:
> (1) Poor readability/maintainability: Any change to the relocation logic
> for a particular target has to be inspected carefully to make sure it won't
> alter the behavior for any other target. Some of my recent patches for X86
> have exacerbated this problem, so I'm very happy to be able to offer this
> patch as penance.
> (2) Poor performance: we're querying the target architecture in multiple
> places for every relocation, despite it being a known quantity as soon as
> we load an object file.
>
> To fix these issues, this patch splits RuntimeDyldMachO's functionality
> across the following class hierarchy:
>
> RuntimeDyldMachO
> Implemented in RuntimeDyldMachO.{h,cpp}
> Contains logic that is completely independent of the target. This consists
> mostly of MachO helper utilities which the derived classes use to get their
> work done.
>
> template <typename Impl>
> RuntimeDyldMachOCRTPBase<Impl> : public RuntimeDyldMachO
> Implemented in RuntimeDyldMachO.h
> Contains generic MachO algorithms/data structures that defer to the Impl
> class for target-specific behaviors. For the curious, and as the name
> suggests, this acts as the base of a CRTP[1] instance.
>
> RuntimeDyldMachOARM : public RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM>
> RuntimeDyldMachOARM64 : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOARM64>
> RuntimeDyldMachOI386 : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOI386>
> RuntimeDyldMachOX86_64 : public
> RuntimeDyldMachOCRTPBase<RuntimeDyldMachOX86_64>
> Implemented in their respective *.h files in
> lib/ExecutionEngine/RuntimeDyld/MachOTargets
> Each of these contains the relocation logic specific to their target
> architecture.
>
> These target specific classes keep the target specific craziness
> mercifully well contained, compared to what we have today. Each target can
> be modified independently, without concern for breaking other targets. The
> 'switch (Arch) ...' anti-pattern is gone, presumably with a small
> performance benefit. There's still lots of work to do, but I think this is
> a solid first step.
>
> Please let me know what you think.
>
> Cheers,
> Lang.
>
>
> [1] http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140620/01d501c6/attachment.html>