llvm dev - Oct 2017 - TargetRegistry and MC object ownership.

Hi All,

While trying to put together an MC-based assembler the other day, I again
encountered MC's non-obvious memory ownership rules. The most egregious
example of these is MCObjectStreamer's destructor:

MCObjectStreamer::~MCObjectStreamer() {
  delete &Assembler->getBackend();
  delete &Assembler->getEmitter();
  delete &Assembler->getWriter();
  delete Assembler;
}

In the depths of a fever from a head-cold, I snapped. I've been hacking MC
to convert these raw pointers (and worse: references!) to unique_ptrs
(apologies to people whose backbends I've broken), but I hit a big blocker
when I get to Target in "llvm/Support/TargetRegistry.h".

Target vends MC objects by calling registered ctor functions. E.g.:

MCAsmBackend *createMCAsmBackend(const MCRegisterInfo &MRI,
                                 StringRef TheTriple, StringRef CPU,
                                 const MCTargetOptions &Options) const {
  if (!MCAsmBackendCtorFn)
    return nullptr;
  return MCAsmBackendCtorFn(*this, MRI, Triple(TheTriple), CPU, Options);
}

The callee owns the resulting object so ideally we would return a
unique_ptr<MCAsmBackend>, but to do this we'd need access to the
definition
of MCAsmBackend which we can't get without violating layering. (We need the
definition because unique_ptr<MCAsmBackend> can notionally call
MCAsmBackend's destructor, though we know it won't here as the whole
point
is to hand ownership back to the caller).

There are three approaches I can think of to improve things:

(1) Keep the raw pointers, but document *very* clearly, recommending the
caller stash the result in a unique_ptr immediately.

(2) Have the ctor functions take a unique_ptr<T>* as their first argument
and store the result there (thanks to Dave Blaikie for this suggestion).
Passing the unique_ptrs by pointer means we don't need a definition for T.
Usage looks like:

std::unique_ptr<MCAsmBackend> MAB;
Target.createMCAsmBackend(&MAB, TheTriple, CPU, Options);


(3) Introduce a 'transient_unique_ptr<T>' type that never calls
T's
destructor, and whose purpose is to pass off ownership to a real unique_ptr:

template <typename T>
class transient_unique_ptr {
public:
  transient_unique_ptr(std::unique_ptr<T> InVal) : Val(InVal.release()) {}
  transient_unique_ptr(transient_unique_ptr<T> &&Other) :
Val(Other.Val) { Other.Val
= nullptr; }
  transient_unique_ptr&& operator=(transient_unique_ptr<T>
&&Other) {
std::swap(Val,
Other.Val); return *this; }
  ~transient_unique_ptr() { assert(!Val && "value should have been
handed
off"); }
  std::unique_ptr<T> take() { auto Tmp = Val; Val = nullptr; return
unique_ptr<T>(Val); }
private:
  T *Val = nullptr;
};

This type can also operate without needing a T definition. Usage looks like:

auto MAB = Target.createMCAsmBackend(TheTriple, CPU, Options).take();

Option (1) is a minimum. Option (2) and (3) are both compromises to deal
with layering rules, but of those I think (2) probably comes closest to
adhering to the principle of least surprise.

Who has thoughts? If we can settle on an approach I'd love to press forward
with the cleanup.

Cheers,
Lang.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171011/6af587e5/attachment.html>

On Wed, Oct 11, 2017 at 11:12 AM Lang Hames <lhames at gmail.com> wrote:
> Hi All,
>
> While trying to put together an MC-based assembler the other day, I again
> encountered MC's non-obvious memory ownership rules. The most egregious
> example of these is MCObjectStreamer's destructor:
>
> MCObjectStreamer::~MCObjectStreamer() {
>   delete &Assembler->getBackend();
>   delete &Assembler->getEmitter();
>   delete &Assembler->getWriter();
>   delete Assembler;
> }
>
> In the depths of a fever from a head-cold, I snapped. I've been hacking
MC
> to convert these raw pointers (and worse: references!) to unique_ptrs
> (apologies to people whose backbends I've broken), but I hit a big
blocker
> when I get to Target in "llvm/Support/TargetRegistry.h".
>
> Target vends MC objects by calling registered ctor functions. E.g.:
>
> MCAsmBackend *createMCAsmBackend(const MCRegisterInfo &MRI,
>                                  StringRef TheTriple, StringRef CPU,
>                                  const MCTargetOptions &Options) const
{
>   if (!MCAsmBackendCtorFn)
>     return nullptr;
>   return MCAsmBackendCtorFn(*this, MRI, Triple(TheTriple), CPU, Options);
> }
>
> The callee owns the resulting object so ideally we would return a
> unique_ptr<MCAsmBackend>, but to do this we'd need access to the
definition
> of MCAsmBackend which we can't get without violating layering. (We need
the
> definition because unique_ptr<MCAsmBackend> can notionally call
> MCAsmBackend's destructor, though we know it won't here as the
whole point
> is to hand ownership back to the caller).
>
I realize your goals are somewhat more immediate, but I wouldn't mind
better understanding the layering here and what might be a good end goal to
know how much work it is, where people should spend it if they want to move
towards that goal, etc. Any ideas how this should be designed to have good
layering?

> There are three approaches I can think of to improve things:
>
> (1) Keep the raw pointers, but document *very* clearly, recommending the
> caller stash the result in a unique_ptr immediately.
>
> (2) Have the ctor functions take a unique_ptr<T>* as their first
argument
> and store the result there (thanks to Dave Blaikie for this suggestion).
> Passing the unique_ptrs by pointer means we don't need a definition for
T.
> Usage looks like:
>
> std::unique_ptr<MCAsmBackend> MAB;
> Target.createMCAsmBackend(&MAB, TheTriple, CPU, Options);
>
This could be a reference rather than a pointer, FWIW - and while the
callsite might look a bit wonky, no one could readily misuse it and anyone
who went looking to fix it would quickly find the comments explaining why,
or if they ignored it, would hit the layering issue we know about already.

> (3) Introduce a 'transient_unique_ptr<T>' type that never
calls T's
> destructor, and whose purpose is to pass off ownership to a real
unique_ptr:
>
> template <typename T>
> class transient_unique_ptr {
> public:
>   transient_unique_ptr(std::unique_ptr<T> InVal) :
Val(InVal.release()) {}
>   transient_unique_ptr(transient_unique_ptr<T> &&Other) :
Val(Other.Val) { Other.Val
> = nullptr; }
>   transient_unique_ptr&& operator=(transient_unique_ptr<T>
&&Other) { std::swap(Val,
> Other.Val); return *this; }
>   ~transient_unique_ptr() { assert(!Val && "value should have
been handed
> off"); }
>   std::unique_ptr<T> take() { auto Tmp = Val; Val = nullptr; return
> unique_ptr<T>(Val); }
> private:
>   T *Val = nullptr;
> };
>
> This type can also operate without needing a T definition. Usage looks
> like:
>
> auto MAB = Target.createMCAsmBackend(TheTriple, CPU, Options).take();
>
The risk here is that it would be easy to miss the 'take()' call (&
auto
especially makes that risky - if it weren't for auto we could use
unnameable type tricks to make it obvious that someone shouldn't make a
local variable (that wouldn't help if template argument deduction were used
like: func(createMCAsmBackend(...)); ))

> Option (1) is a minimum. Option (2) and (3) are both compromises to deal
> with layering rules, but of those I think (2) probably comes closest to
> adhering to the principle of least surprise.
>
Yeah - I think people will find (2) quirky but doesn't mean building whole
new constructs (that might end up with more usage than we'd like) for these
kinds of layering problems. It's where I'd lean.

> Who has thoughts? If we can settle on an approach I'd love to press
> forward with the cleanup.
>
> Cheers,
> Lang.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171011/b97fd56a/attachment.html>

Lang Hames via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Hi All,
>
> While trying to put together an MC-based assembler the other day, I again
> encountered MC's non-obvious memory ownership rules. The most egregious
> example of these is MCObjectStreamer's destructor:
>
> MCObjectStreamer::~MCObjectStreamer() {
>   delete &Assembler->getBackend();
>   delete &Assembler->getEmitter();
>   delete &Assembler->getWriter();
>   delete Assembler;
> }
>
> In the depths of a fever from a head-cold, I snapped. I've been hacking
MC
> to convert these raw pointers (and worse: references!) to unique_ptrs
> (apologies to people whose backbends I've broken), but I hit a big
blocker
> when I get to Target in "llvm/Support/TargetRegistry.h".
>
> Target vends MC objects by calling registered ctor functions. E.g.:
>
> MCAsmBackend *createMCAsmBackend(const MCRegisterInfo &MRI,
>                                  StringRef TheTriple, StringRef CPU,
>                                  const MCTargetOptions &Options) const
{
>   if (!MCAsmBackendCtorFn)
>     return nullptr;
>   return MCAsmBackendCtorFn(*this, MRI, Triple(TheTriple), CPU, Options);
> }
>
> The callee owns the resulting object so ideally we would return a
> unique_ptr<MCAsmBackend>, but to do this we'd need access to the
definition
> of MCAsmBackend which we can't get without violating layering. (We need
the
> definition because unique_ptr<MCAsmBackend> can notionally call
> MCAsmBackend's destructor, though we know it won't here as the
whole point
> is to hand ownership back to the caller).
>
> There are three approaches I can think of to improve things:
>
> (1) Keep the raw pointers, but document *very* clearly, recommending the
> caller stash the result in a unique_ptr immediately.
>
> (2) Have the ctor functions take a unique_ptr<T>* as their first
argument
> and store the result there (thanks to Dave Blaikie for this suggestion).
> Passing the unique_ptrs by pointer means we don't need a definition for
T.
> Usage looks like:
>
> std::unique_ptr<MCAsmBackend> MAB;
> Target.createMCAsmBackend(&MAB, TheTriple, CPU, Options);
>
>
> (3) Introduce a 'transient_unique_ptr<T>' type that never
calls T's
> destructor, and whose purpose is to pass off ownership to a real
unique_ptr:
>
> template <typename T>
> class transient_unique_ptr {
> public:
>   transient_unique_ptr(std::unique_ptr<T> InVal) :
Val(InVal.release()) {}
>   transient_unique_ptr(transient_unique_ptr<T> &&Other) :
> Val(Other.Val) { Other.Val
> = nullptr; }
>   transient_unique_ptr&& operator=(transient_unique_ptr<T>
&&Other) {
> std::swap(Val,
> Other.Val); return *this; }
>   ~transient_unique_ptr() { assert(!Val && "value should have
been handed
> off"); }
>   std::unique_ptr<T> take() { auto Tmp = Val; Val = nullptr; return
> unique_ptr<T>(Val); }
> private:
>   T *Val = nullptr;
> };
(4) Move target registration out of Support so that it can include the
required definitions from MC.

Cheers,
Rafael

>
> (4) Move target registration out of Support so that it can include the
> required definitions from MC.

Yep -- left that one out. Chalk that up to the fever. This seems like an
ideal solution if we can do it.

-- Lang.

On Wed, Oct 11, 2017 at 11:25 AM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:
> Lang Hames via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
> > Hi All,
> >
> > While trying to put together an MC-based assembler the other day, I
again
> > encountered MC's non-obvious memory ownership rules. The most
egregious
> > example of these is MCObjectStreamer's destructor:
> >
> > MCObjectStreamer::~MCObjectStreamer() {
> >   delete &Assembler->getBackend();
> >   delete &Assembler->getEmitter();
> >   delete &Assembler->getWriter();
> >   delete Assembler;
> > }
> >
> > In the depths of a fever from a head-cold, I snapped. I've been
hacking
> MC
> > to convert these raw pointers (and worse: references!) to unique_ptrs
> > (apologies to people whose backbends I've broken), but I hit a big
> blocker
> > when I get to Target in "llvm/Support/TargetRegistry.h".
> >
> > Target vends MC objects by calling registered ctor functions. E.g.:
> >
> > MCAsmBackend *createMCAsmBackend(const MCRegisterInfo &MRI,
> >                                  StringRef TheTriple, StringRef CPU,
> >                                  const MCTargetOptions &Options)
const {
> >   if (!MCAsmBackendCtorFn)
> >     return nullptr;
> >   return MCAsmBackendCtorFn(*this, MRI, Triple(TheTriple), CPU,
Options);
> > }
> >
> > The callee owns the resulting object so ideally we would return a
> > unique_ptr<MCAsmBackend>, but to do this we'd need access to
the
> definition
> > of MCAsmBackend which we can't get without violating layering. (We
need
> the
> > definition because unique_ptr<MCAsmBackend> can notionally call
> > MCAsmBackend's destructor, though we know it won't here as the
whole
> point
> > is to hand ownership back to the caller).
> >
> > There are three approaches I can think of to improve things:
> >
> > (1) Keep the raw pointers, but document *very* clearly, recommending
the
> > caller stash the result in a unique_ptr immediately.
> >
> > (2) Have the ctor functions take a unique_ptr<T>* as their first
argument
> > and store the result there (thanks to Dave Blaikie for this
suggestion).
> > Passing the unique_ptrs by pointer means we don't need a
definition for
> T.
> > Usage looks like:
> >
> > std::unique_ptr<MCAsmBackend> MAB;
> > Target.createMCAsmBackend(&MAB, TheTriple, CPU, Options);
> >
> >
> > (3) Introduce a 'transient_unique_ptr<T>' type that
never calls T's
> > destructor, and whose purpose is to pass off ownership to a real
> unique_ptr:
> >
> > template <typename T>
> > class transient_unique_ptr {
> > public:
> >   transient_unique_ptr(std::unique_ptr<T> InVal) :
Val(InVal.release())
> {}
> >   transient_unique_ptr(transient_unique_ptr<T> &&Other)
:
> > Val(Other.Val) { Other.Val
> > = nullptr; }
> >   transient_unique_ptr&&
operator=(transient_unique_ptr<T> &&Other) {
> > std::swap(Val,
> > Other.Val); return *this; }
> >   ~transient_unique_ptr() { assert(!Val && "value should
have been handed
> > off"); }
> >   std::unique_ptr<T> take() { auto Tmp = Val; Val = nullptr;
return
> > unique_ptr<T>(Val); }
> > private:
> >   T *Val = nullptr;
> > };
>
> (4) Move target registration out of Support so that it can include the
> required definitions from MC.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20171011/4ab38a1c/attachment.html>