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

Tue Jan 3 14:24:16 PST 2017

>> > 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).

Again, I don't think the STL is relevant to this question.  The STL
rightly does not take a position on the question we are interested in,
which is one of coding conventions.  However, in LLVM, it makes sense
for us to take a position on coding conventions.

>> 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.

FWIW I've looked through many classes in llvm that use intrusive
refcounting, and none of them match this idiom insofar as I can tell.
Although where it does appear, intrusive refcounting seems like a
reasonable thing to do.

Another reason to use intrusive refcounting as opposed to
std::shared_ptr is that shared_ptr is always thread-safe, which is
expensive.  Another reason is that intrusive refcounting lets you
override retain/release.

>> 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.

I don't think this is responsive to my point?  My point is that,
*because* most APIs do not specify this contract, we cannot tell
whether they today ever call retain/release.

>> 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.

Well this is a testable hypothesis, we don't need to speculate.  Maybe
you could see how many classes in LLVM that inherit from
RefCountedBase have unit tests that allocate instances of the type on
the stack?  Your guess is that most of them will either have such
tests, or have a comment indicating "don't allocate me on the stack",
correct?

> 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).

Sure, I agree this would be a good thing to do in many cases.  We can
definitely suggest it in the comments here.  But I am not volunteering
to clean up all of our existing code.  Note that this becomes somewhat
more complicated and verbose when (multiple) inheritance is involved.
Many of the classes in llvm that inherit from RefCountedBase are
relatively small helper classes, where the machinery to do this would
significantly increase the length of the class relative to its current
size.

Note also that even if a class is itself fine with being allocated on
the stack (and makes it explicit in comments or whatever), we have the
problem of functions calling retain/release.

>> 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 agree that in general allocating things on the stack is clearer than
allocating on the heap.  However, usually types inherit from
RefCountedBase because they participate in complicated ownership
hierarchies.  For such types, my prior is that it's *not* clearer to
allocate them on the stack, because of this complex ownership and also
because you can never tell which, if any, functions it's safe to call.

Perhaps you could argue with some specific code examples from llvm
that you think would be better written as a result of this idiom.

>> 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.

If you just want to remove the sentence that says

/// Objects that inherit from RefCountedBase should always be allocated with
/// operator new.

I'm fine with that.

>> 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
>> >