[PATCH] Substantial RuntimeDyldMachO cleanup.

[Lang Hames]

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/20140612/86cdfa3d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtdyld_macho_cleanup.patch
Type: application/octet-stream
Size: 70823 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140612/86cdfa3d/attachment.obj>