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

Tue Jan 3 13:33:55 PST 2017

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

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

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.

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

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
>