[PATCH] Substantial RuntimeDyldMachO cleanup.

Thu Jul 17 11:17:39 PDT 2014

Couple of small comments, otherwise ship it.

+    return std::unique_ptr<RuntimeDyldMachO>(new RuntimeDyldMachOARM(MM));

make_unique?

+/// can be found in ./MachOTargets.

I think you've got it as Targets at the moment?

-eric

On Wed, Jul 16, 2014 at 11:20 AM, Lang Hames <lhames at gmail.com> wrote:
> Hi Eric, Phil,
>
> Sorry about the delayed reply - I was out on holidays for a bit.
>
> Attached is an updated patch:
> - The ARM64 class has been renamed to AArch64.
> - I've fixed the weirdly named 'do' methods.
> - Everything has been run through clang format.
>
> Thanks very much for the review and feedback. If this all looks good to you
> I'll go ahead and commit.
>
> Cheers,
> Lang.
>
>
> On Tue, Jun 24, 2014 at 11:24 AM, Eric Christopher <echristo at gmail.com>
> wrote:
>>
>> 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
>> >>
>
>