<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 11, 2017 at 11:12 AM Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi All,<div><br></div><div>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:</div><div><br></div><div><font face="monospace, monospace">MCObjectStreamer::~MCObjectStreamer() {</font></div><div><font face="monospace, monospace"> delete &Assembler->getBackend();</font></div><div><font face="monospace, monospace"> delete &Assembler->getEmitter();</font></div><div><font face="monospace, monospace"> delete &Assembler->getWriter();</font></div><div><font face="monospace, monospace"> delete Assembler;</font></div><div><font face="monospace, monospace">}</font></div><div><br></div><div>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".</div><div><br></div><div>Target vends MC objects by calling registered ctor functions. E.g.:</div><div><br></div><div><font face="monospace, monospace">MCAsmBackend *createMCAsmBackend(const MCRegisterInfo &MRI,<br></font></div><div><div><font face="monospace, monospace"> StringRef TheTriple, StringRef CPU,</font></div><div><font face="monospace, monospace"> const MCTargetOptions &Options) </font><span style="font-family:monospace,monospace">const {</span></div><div><font face="monospace, monospace"> if (!MCAsmBackendCtorFn)</font></div><div><font face="monospace, monospace"> return nullptr;</font></div><div><font face="monospace, monospace"> return MCAsmBackendCtorFn(*this, MRI, Triple(TheTriple), CPU, Options);</font></div><div><font face="monospace, monospace">}</font></div></div><div><br></div><div>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).</div></div></blockquote><div><br>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?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>There are three approaches I can think of to improve things:<br></div><div><br></div><div>(1) Keep the raw pointers, but document *very* clearly, recommending the caller stash the result in a unique_ptr immediately.</div><div><br></div><div>(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:</div><div><br></div><div><font face="monospace, monospace">std::unique_ptr<MCAsmBackend> MAB;</font></div><div><font face="monospace, monospace">Target.createMCAsmBackend(&MAB, TheTriple, CPU, Options);</font></div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>(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:<br></div><div><br></div><div><font face="monospace, monospace">template <typename T></font></div><div><font face="monospace, monospace">class transient_unique_ptr {</font></div><div><font face="monospace, monospace">public:</font></div><div><font face="monospace, monospace"> transient_unique_ptr(std::unique_ptr<T> InVal) : Val(InVal.release()) {}</font></div><div><font face="monospace, monospace"> transient_unique_ptr(transient_unique_ptr<T> &&Other) : Val(Other.Val) { </font><span style="font-family:monospace,monospace">Other.Val = nullptr; </span><span style="font-family:monospace,monospace">}</span></div><div><font face="monospace, monospace"> transient_unique_ptr&& operator=(transient_unique_ptr<T> &&Other) { </font><span style="font-family:monospace,monospace">std::swap(Val, Other.Val); </span><span style="font-family:monospace,monospace">return *this; }</span></div><div><font face="monospace, monospace"> ~transient_unique_ptr() { assert(!Val && "value should have been handed off"); }</font></div><div><font face="monospace, monospace"> std::unique_ptr<T> take() { auto Tmp = Val; Val = nullptr; return unique_ptr<T>(Val); }</font></div><div><font face="monospace, monospace">private: </font></div><div><font face="monospace, monospace"> T *Val = nullptr;</font></div><div><font face="monospace, monospace">};</font></div><div><br></div><div>This type can also operate without needing a T definition. Usage looks like:</div><div><br></div><div><font face="monospace, monospace">auto MAB = Target.createMCAsmBackend(TheTriple, CPU, Options).take();</font></div></div></blockquote><div><br>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(...)); ))<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>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.<br></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>Who has thoughts? If we can settle on an approach I'd love to press forward with the cleanup.<br></div><div><br></div><div>Cheers,</div><div>Lang.</div></div>
</blockquote></div></div>