[PATCH] Substantial RuntimeDyldMachO cleanup.

Tue Jun 24 11:24:56 PDT 2014

One other small nit:

The naming etc of process and do is really weird. Renaming those for
clarity (or great justice, your choice) might be nice.

-eric

On Tue, Jun 24, 2014 at 11:24 AM, Eric Christopher <echristo at gmail.com> wrote:
> 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
>>