[PATCH] Substantial RuntimeDyldMachO cleanup.

[Eric Christopher]

Seems good to me as well.

One small nit: Might be reasonable to migrate the ARM64 classes to
AArch64 to match the rest of llvm? (The relocation names, of course,
will have to stay the same.)

-eric

On Mon, Jun 23, 2014 at 5:09 PM, Philip Reames
<listmail at philipreames.com> wrote:
> Lang,
>
> I'm happy with all of your answers and like the general direction.  If
> others approve, go for it.
>
> Philip
>
>
> On 06/23/2014 04:47 PM, Lang Hames wrote:
>
> 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 list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>