[PATCH] Substantial RuntimeDyldMachO cleanup.

Wed Jul 16 11:20:43 PDT 2014

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/20140716/ba5f0730/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtdyld-macho-cleanup-v3.patch
Type: application/octet-stream
Size: 81599 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/ba5f0730/attachment.obj>