This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors
ClosedPublic

Authored by njames93 on Dec 2 2020, 6:45 AM.

Details

Summary

Added a trivial destructor in release mode and in debug mode a destructor that asserts RefCount is indeed zero.
This ensure people aren't manually (maybe accidentally) destroying these objects like in this contrived example.

{
  std::unique_ptr<SomethingRefCounted> Object;
  holdIntrusiveOwnership(Object.get());
  // Object Destructor called here will assert.
}

Diff Detail

Event Timeline

njames93 created this revision.Dec 2 2020, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 6:45 AM
njames93 requested review of this revision.Dec 2 2020, 6:45 AM
njames93 added inline comments.Dec 2 2020, 8:21 AM
llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
120–121

Hmmm... This assert is causing clang-tidy tests to fail??? Investigating.

njames93 updated this revision to Diff 309093.Dec 2 2020, 4:10 PM

Fix assertion due to a pointer stored in a ManagedStatic.

Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 4:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Removing myself as reviewer since I no longer work in this area. Someone who is more involved with llvm Support/ADT should review this.

njames93 added inline comments.
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
190–196

Is this a case that happens in other places, ManagedStatics holding RefCounted objects, if so this could be extracted and reused in other places.

dblaikie added inline comments.Dec 2 2020, 4:39 PM
llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
75–76

I can't quite understand this comment - perhaps you could try rephrasing it?

79–80

What does it mean to copy a ref counted object with a builtin ref count?

I would /guess/ maybe copies (or moves) of ref counted objects like this should only be valid when the ref count is 1 or maybe only 0? (since all the other users who might have an active reference certainly aren't getting a reference to the newly copied object implicitly)

Do you know which users which relied on any of the copy behavior? Maybe leave it as = default for now?

I guess it makes sense that the copy would have a zero ref count - same as if it were newly default constructed. But if no one's been using this effectively/at all already - maybe avoid creating this new behavior/changing the semantics here? What if we only allowed copies of objects with a zero ref count - is that sufficient for existing uses?

dexonsmith added inline comments.Dec 2 2020, 4:41 PM
llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
75–80

I'm not quite following why you needed to add these special members; it seems like just the destructor would be enough for the assertion; but if you do need them, can/should they be = default?

njames93 updated this revision to Diff 309120.Dec 2 2020, 6:29 PM

Added test cases demonstrating copy and move behaviour.

I've added test cases which demonstrate what these copies/moves are about.
The move constructor probably would never get used. I can't see of a reason to move the contents of a ref counted pointer. In which case there could be an argument to delete it.
The copy constructor OTOH is very likely to be used (which i think is the reason its currently defined in RefCountedBase).
Adding the CopyAssignment and MoveAssignment operators are there to make sure the compiler wont define them implicitly, copying the ref count.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
75–76

I understand it in my head, just can't think of the best way to write it down.

75–80

They can't be defaulted as we don't want to copy the RefCount.

dexonsmith added inline comments.Dec 3 2020, 11:21 AM
llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
75–76

Maybe something like this would work:

/// Do not copy or move RefCount when constructing or assigning from
/// a derived type. These operations don't imply anything about the
/// number of IntrusiveRefCntPtr instances.

WDYT?

75–80

Ah, I get it, thanks for explaining.

dblaikie added inline comments.Dec 3 2020, 1:27 PM
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
190–196

I think probably the right solution is to have TrueMatcherImpl's dtor Release the same way its ctor Retains. Symmetry is nice/helps avoid splitting this quirk into two different places.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
75–80

Do we need them at all, though?

The code prior to this patch has a copy ctor that "does the right thing" by producing a ref count of 0 for the new copy (same as a newly constructed object). No move operations will be provided - nor probably should there be any, the copy semantics are cheap/correct & not sure there's a more optimized implementation for move.

The copy assignment operaotr currently is probably broken (the implicit copy assignment is deprecated in the presence of a user-declared copy ctor - so we could probably delete that rather than adding it if it's not used (& at least, currently if it is used it'd be pretty broken).

I've committed 2e83ccc2ee333389110659f3cb313968a0c970d4 which takes parts of this patch - specifically fixing the test that was already intended to test the RefCountedBase's copy constructor, but didn't (because the test called the default constructor instead) and extending it to test/cover (& also fix) the similar behavior in ThreadSafeRefCountedBase.

That should leave this proposed patch to add the dtors and assertions.

njames93 updated this revision to Diff 309489.Dec 4 2020, 2:08 AM

Made this just focus on the destruction asserts.

njames93 retitled this revision from [llvm] Unify interface of (ThreadSafe)?RefCountedBase to [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors.Dec 4 2020, 2:22 AM
njames93 edited the summary of this revision. (Show Details)
njames93 added inline comments.
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
190–196

That method is a nasty can of worms. If TrueMatcherImpl dtor calls Release, that Release call would realise the RefCount is now zero. That in turn would then call delete on TrueMatcherImpl resulting in a double-free.
I think symmetry is possibly the right way to go though, just in using a custom creator that calls Retain itself, remove that responsibility from TrueMatcherImpl.

njames93 updated this revision to Diff 309512.Dec 4 2020, 4:47 AM

Made ManagedStatic code transparent to users for RefCounted objects.

njames93 updated this revision to Diff 309524.Dec 4 2020, 6:00 AM

Remove unnecessary comment added in ASTMatchersInternal.cpp

dblaikie added inline comments.Dec 4 2020, 12:33 PM
llvm/include/llvm/Support/ManagedStatic.h
25 ↗(On Diff #309524)

Are there many uses that rely on this? I don't think it's really worth all this infrastructure - compared to having it supported on an as-needed basis, such as directly in TrueMatcherImpl's ctor and dtor.

njames93 marked 6 inline comments as done and 2 inline comments as done.Dec 4 2020, 2:28 PM
njames93 added inline comments.
llvm/include/llvm/Support/ManagedStatic.h
25 ↗(On Diff #309524)

It doesn't work in TrueMatcherImpl ctor/dtor due to the whole double free issue(See previous comment). I can go back to just supporting it for TrueMatcherImpl by writing a single custom creator/deleter for that class.

This approach taken was a little overkill but likely a little more foolproof for someone using the library. I'll go ahead with whichever approach you would prefer.

dblaikie added inline comments.Dec 4 2020, 6:29 PM
llvm/include/llvm/Support/ManagedStatic.h
25 ↗(On Diff #309524)

Ah, right - thanks for walking me through it again, now I better understand your previous comment - sorry for that erroneous suggestion/confusion.

Fair points all.

Given all that, I'm sort of leaning towards the idea that maybe the right solution here is for the TrueMatcherInstance bear the cost of the complexity here (if it's the only one) with something like:

struct TrueMatcherImplCreator {
  static void *call() {
    return new IntrusiveRefCntPtr<TrueMatcherImpl>(new TrueMatcherImpl());  
  }
};
static llvm::ManagedStatic<IntrusiveRefCntPtr<TrueMatcherImpl>, TrueMatcherImplCreator> TrueMatcherInstance;

I worry about creating a fairly generic extension point for customizing how elements in ManagedStatic can be constructed and destroyed via specialization rather than via explicit creator/destroyer parameters.

And while the custom destroyer is a bit simpler mechanically (doesn't involve dynamically allocating an IntrusiveCntPtr, which is unintuitive to say the least) - I think sticking to the "if you ever share ownership of a RefCountedBase object, you must've allocated it with 'new' and be really sharing ownership - no lies" is probably a healthier model for RefCountedBase/IntrusiveRefCntPtr.

njames93 added inline comments.Dec 5 2020, 4:07 AM
llvm/include/llvm/Support/ManagedStatic.h
25 ↗(On Diff #309524)

Decided to take a step back. This is trying to fix a problem that's only here because we are using a ManagedStatic. when its not needed. A function scope static has nearly the same semantics of ManagedStatic and as this is only used in one function it seems a much better fit.

njames93 updated this revision to Diff 309730.Dec 5 2020, 4:07 AM

Refactored the ManagedStatic instance into a function static.

dblaikie accepted this revision.Dec 5 2020, 10:09 AM
dblaikie added inline comments.
clang/lib/ASTMatchers/ASTMatchersInternal.cpp
157

You can remove this ctor declaration, since it's the same as the implicit default.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
73

You could probably commit this separately - no need for extra review, but it's separate/independent of the rest of this change, I think?

llvm/include/llvm/Support/ManagedStatic.h
25 ↗(On Diff #309524)

Yeah, I'm good with that - I guess ManagedStatic has the extra "deallocate/destroy on llvm_shutdown" and this object doesn't seem big/significant enough that it'll matter to destroy them on shutdown. No doubt we've got lots of other function-local statics of similar weight/use that we don't worry about destroying on llvm_shutdown.

This revision is now accepted and ready to land.Dec 5 2020, 10:09 AM