[PATCH] D28245: ADT: IntrusiveRefCntPtr: Broaden the definition of correct usage of RefCountedBase

Tue Jan 3 13:50:13 PST 2017

On Tue, Jan 3, 2017 at 1:34 PM Justin Lebar <jlebar at google.com> wrote:

> I would be happy if we wanted to change the comment from
>
> > Objects that inherit from RefCountedBase should always be allocated with
> operator new.
>
> to
>
> > You should usually allocate objects that inherit from RefCountedBase on
> the heap with operator new.  You can allocate them on the stack, but then
> you must ensure that we never will call operator delete on the object,
> which usually means you must be certain that every function which takes a
> pointer or reference to the object (including member functions) promises as
> part of its contract never to call retain() or release().
>
> If that's satisfactory to you, I guess you can stop reading here.  :)
>

It still seems like it's the opposite of what I'd expect (& the opposite of
how the STL documents std::enable_shared_from_this).

> > I feel like the default should be the opposite (which is
> std::enable_shared_from_this's position as well) - that the need for
> intrusive reference counting would /allow/ the type to be owned in this
> way, but not require it. If a type requires it, it could document that (on
> any specific operations, or universally).
>
> OK, let me try to frame this question.  Before I begin, I should point
> out that I actually have a fair bit of experience working in
> refcounted systems.  Almost every C++ object in Firefox is refcounted,
> and I spent a few years on a team tasked with tracking down memory
> leaks, which meant I became quite familiar with Firefox's object
> lifetime management (in)capabilities.  Thankfully LLVM's use of
> refcounting is far simpler than Firefox's.  I hope we can keep it that
> way.
>
> So, back to framing the question.  Let T be some type which inherits
> from RefCountedBase.  We have some function f(T* t) (maybe a member
> function on T).  There are three possibilities:
>
> a) f's contract is that it may call retain on t.
> b) f's contract is that it never calls retain on t.
> c) f's contract does not specify whether or not it (transitively)
> calls retain on t.
>
> My understanding is we're trying to decide whether, in case (c), we
> should assume that f will never be modified to call retain, or that it
> may be modified to call retain.  I'm saying we should assume it may be
> modified to call retain, and you're saying that we should assume it
> cannot be modified to call retain.  Is that right?
>

Roughly. I think there's a real wrinkle to this sort of approach, though.
In my experience (which is likely more limited than yours in this domain),
the reason to use intrusive reference counting is because some intermediate
API doesn't handle/record ownership accurately - perhaps there's some API
that traffics in unowned pointers, but on both sides of this API there's
something that knows about their ownership semantics and relies on
intrusive reference counting to re-acquire/prolong lifetime on the other
side of this opaque barrier.

If we were actually documenting the ownership contract in this API - we'd
not need intrusive ref counting & could use std::shared_ptr instead to
explicitly document the potential for ownership passing.

> Let's call these two assumptions may-retain and cannot-retain.
>
> I think may-retain is the safer assumption, for two reasons:
>
> 1) Proving that that cannot-retain is safe in today's LLVM would
> require a large effort.
>
> There are ~30 classes in llvm today that inherit from RefCountedBase.
> Most or all of them are in case (c) -- they do not specify whether or
> not their member functions may call retain.  However, we have no
> automated way to check whether any of them have member functions that
> transitively call retain(this).  Similarly, there are many free
> functions which use these 30 classes, and we have no way to check
> whether any of them call retain(this).  Verifying that each of these
> classes is cannot-retain-safe would require a whole-program call-graph
> analysis.
>

I generally wouldn't expect most APIs (member and non-member functions) to
specify such a contract - and that specific APIs coordinate this sort of
situation.

> In contrast, may-retain is obviously a safe(r) assumption, since the
> allowed set of uses under may-retain is a subset of the allowed set of
> uses under cannot-retain.  Moreover, one could easily write an
> automated check to verify compliance with may-retain -- it's just a
> matter of looking for instances of T allocated on the stack.
>
> 2) cannot-retain worsens the practical business of writing refcounted
> classes.
>
> cannot-retain says that if I write a T and I happen not to put a
> comment saying "don't allocate this on the stack", then it is my
> problem as the class author if someone uses T on the stack and then I
> make a change to T that breaks them.
>
> In contrast, may-retain treats the fact that we inherit from
> RefCountedBase as a signal to users of the class that they probably
> shouldn't allocate T on the stack without talking to the author first
> (and adding a comment indicating that it's OK).
>
> As a practical matter, the author of T probably never even thought
> about whether or not it's safe to allocate it on the stack.
>

I'd actually generally expect they should/would - simple unit tests would
likely create instances of the type on the stack.

If the type was really only ever meant to be allocated with 'new' I'd
suggest the class should have a private ctor and a factory function so that
the only way it can be obtained is through that factory that uses new (&
ideally returns an IntrusiveRefCntPtr).

> cannot-retain assumes that if things happen to work today, that's
> intentional and will remain so forever, but certainly the first part
> of that assumption is frequently incorrect.  By adding an implicit
> assumption to every T, cannot-retain complicates the practical
> business of writing refcounted classes.
>
> ---
>
> The reason I understand you've given in favor of cannot-retain above
> is that RefCountedBase should be seen a signal only that refcounting
> is *allowed* on a type, but not that it's mandatory.
>
> This is kind of begging the question; it's just another way of saying
> "cannot-retain should be the default assumption."  It sort of seems
> that you're saying that cannot-retain should be the default because
> it's more flexible?  If so, I would respond by saying, "more flexible
> for whom?"  It's more flexible for users of the class, but decreases
> the flexibility of the class author and any functions which operate on
> T.

Yes, I believe that makes code generally easier to understand because it
encourages code to isolate the places that depend on intrusive/implicit
ownership passing. It makes the usual/obvious code (not ownership passing)
the default assumption, which is generally what I think we want.

>   I've tried to argue above why I think the flexibility tradeoff
> should be resolved in favor of the class author in this case.
>
> >  (which is std::enable_shared_from_this's position as well)
>
> I can totally believe that the STL says "we never retain/release an
> object which happens to inherit from enable_shared_from_this in any
> STL call".  That seems to me to be a very sensible position for the
> STL to take.
>
> However, I would be very surprised if the STL took a position on the
> question that I am interested in here, namely whether as a convention
> we can assume that user code may be modified to call retain on t.
>

Right, I don't think it takes a position on that - and that was my original
suggestion: to remove the comment and take no position on it either.

- Dave

>
>
> On Tue, Jan 3, 2017 at 11:47 AM, David Blaikie via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> >
> >
> > On Tue, Jan 3, 2017 at 11:43 AM Justin Lebar via Phabricator
> > <reviews at reviews.llvm.org> wrote:
> >>
> >> jlebar requested changes to this revision.
> >> jlebar added a comment.
> >> This revision now requires changes to proceed.
> >>
> >> I think what I wrote on the mailing list applies here:
> >>
> >> > btw I don't think it's at all so simple.  For example, an object
> >> >  might, in one of its member functions, cause itself to be
> >> >  retained/released.  This is perfectly valid, but of course will break
> >> >  you in exciting ways if you allocate the object on the stack.
> >> >
> >> > The advice of "it's safe to allocate these objects on the stack if
> >> >  you're careful and the object doesn't ever cause itself to be
> >> >  retained/released" really means "if the object doesn't *currently*
> >> >  ever cause itself to be retained/released".  IOW allocating such an
> >> >  object on the stack constrains future changes to the object's
> >> >  implementation.  If I had to do a cleanup of code I didn't care about
> >> >  that was allocating such an object on the stack before I could land a
> >> >  change I cared about, I'd be pretty annoyed, and rightfully so, I
> >> >  think.
> >> >
> >> > That is to say, unless it's part of an object's contract that it never
> >> >  causes itself to be retained/released, I think you're in for a bad
> >> >  time.  Indeed, the same applies for every function you pass that
> >> >  object to: Unless it is part of that function's contract that it
> never
> >> >  causes the object to be retained or released, passing a
> >> >  stack-allocated object to such a function is going to constrain
> future
> >> >  changes to the implementation of that function and all transitive
> >> >  callees.
> >> >
> >> > This seems like an extremely unfriendly thing to do.
> >>
> >> That is, as written this comment seems to say that it's OK for me to
> take
> >> a
> >> class that inherits from RefCountedBase and allocate it on the stack if
> I
> >> am
> >> careful about it.  But doing so constrains the author of the class from
> >> ever
> >> calling Retain/Release on itself.  I think our default should be that
> this
> >> is
> >> allowed, which implies that our default should be that allocating such
> >> objects
> >> on the stack is disallowed.
> >
> >
> > I feel like the default should be the opposite (which is
> > std::enable_shared_from_this's position as well) - that the need for
> > intrusive reference counting would /allow/ the type to be owned in this
> way,
> > but not require it. If a type requires it, it could document that (on any
> > specific operations, or universally).
> >
> >>
> >> If a class explicitly promises never to Retain/Release itself, then
> >> allocating
> >> it on the stack is fine.  However, for the same reason, you should only
> >> pass it
> >> to functions that promise never to Retain/Release the object.
> >>
> >> I'm fine if you want to write this up in a comment, although I am
> curious
> >> what
> >> problem you're trying to solve.
> >
> >
> > Broadly: That ownership semantics are kept orthogonal to types as much as
> > possible. That some users of a type may need to leverage intrusive
> reference
> > counting (to get through some interface that can't accurately model the
> > ownership in the interim, for example) shouldn't so-constrain all users
> to
> > use the type that way.
> >
> > - Dave
> >
> >>
> >>
> >>
> >> https://reviews.llvm.org/D28245
> >>
> >>
> >>
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170103/cd568929/attachment.html>