[PATCH] Substantial RuntimeDyldMachO cleanup.

Thu Jul 17 12:04:34 PDT 2014

Hi Eric.

I've changed the unique_ptr references to make_unique, fixed that comment,
and committed in r213293.

Thanks again for the review and feedback.

Cheers,
Lang.

On Thu, Jul 17, 2014 at 11:17 AM, Eric Christopher <echristo at gmail.com>
wrote:

> 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
> >> >>
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140717/f0c3542a/attachment.html>