[PATCH] Substantial RuntimeDyldMachO cleanup.

[Lang Hames]

CCing Nick Kledzik as suggested by Rafael.

- Lang.


On Mon, Jun 23, 2014 at 3:49 PM, Lang Hames <lhames at gmail.com> wrote:

> Hi Philip,
>
> Thanks for taking a look!
>
> Answering point-by-point:
>
> (1) These functions don't have to be defined in the headers, but that's
> probably where we want them. Part of the intent of the CRTP pattern is
> these definitions should be inlined into the parent class
> (RuntimeDyldCRTPBase), so they need to be present when the instantiations
> of the parent class are used (in RuntimeDyldMachO.cpp). That means either
> putting the code for all MachO targets in to RuntimeDyldMachO.cpp, or
> having one header per target. I think the latter is cleaner.
>
> (2) Yes for now, but my plan is to start removing some of the dynamism
> from the RuntimeDyldImpl class tree in the future. That work was omitted
> from this patch as it was already getting large, and I wanted to clear the
> general direction with the community first.
>
> (3) I can remove the DEBUG_TYPE defines from the target-specific files if
> I push the include order around in RuntimeDyldMachO.h:
>
> From what we have now (with the DEBUG_TYPE defined in the headers):
>
> #include ...
> #include "MachOTargets/..."
> #include "MachOTargets/..."
> #include ...
>
> To:
>
> #include ...
> #include ...
> // End of non-target includes.
>
> #define DEBUG_TYPE "dyld"
> #include "MachOTargets/..."
> #include "MachOTargets/..."
> // End target includes.
>
> The effect is the same either way: We can use the DEBUG macro from the
> target specific headers. I think I actually prefer the latter, and I'd be
> happy to switch to it. Neither is canonical, but that's down to the
> weirdness of the CRTP.
>
> Cheers,
> Lang.
>
>
>
> On Fri, Jun 20, 2014 at 4:36 PM, Philip Reames <listmail at philipreames.com>
> wrote:
>
>>  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> 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 listllvm-commits at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140623/2015e966/attachment.html>