[PATCH] Substantial RuntimeDyldMachO cleanup.

[Philip Reames]

The overall direction seems quite reasonable.

I haven't looked at the patch in any detail under the assumption you're 
mostly just moving code around.   A couple small comments:
- Do all these functions need to be defined in headers?
- Do the functions on RuntimeDyldMach0 need to be marked virtual or 
override?
- Can we move the DEBUG_TYPE macros out of the headers and into source 
files?

Philip



On 06/20/2014 01:36 PM, Lang Hames wrote:
> 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 
> <mailto: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
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140620/516a895d/attachment.html>