[libcamera-devel] [PATCH 1/4] Documentation: coding_style: Add object ownership rules

Fri Jan 18 18:53:15 CET 2019

Hi Laurent, Jacopo,

On Sat, Jan 19, 2019 at 1:41 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> On Fri, Jan 18, 2019 at 06:00:44PM +0100, Jacopo Mondi wrote:
> >  On Fri, Jan 18, 2019 at 06:17:19PM +0200, Laurent Pinchart wrote:
> > > On Fri, Jan 18, 2019 at 03:56:08PM +0100, Jacopo Mondi wrote:
> > >> Hi Laurent,
> > >> thanks for doing this!
> > >>
> > >> I don't look picky here, but this is an important piece of
> > >> documentation, and I want to make sure I got it right first...
> > >
> > > This is exactly the kind of patch for which I expect picky reviews :-)
> > >
> >
> >  Happy to be of any help then :)
> >
> > >> On Fri, Jan 18, 2019 at 01:59:13AM +0200, Laurent Pinchart wrote:
> > >>> Object ownership is a complex topic that can lead to many issues, from
> > >>> memory leak to crashes. Document the rules that libcamera enforces to
> > >>> make object ownership tracking explicit.
> > >>>
> > >>> This is a first version of the rules and is expected to be expanded as
> > >>> the library is developed.
> > >>>
> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >>> ---
> > >>> Documentation/coding-style.rst | 62 ++++++++++++++++++++++++++++++++++
> > >>> 1 file changed, 62 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > >>> index f8d2fdfeda8e..f77325239bfa 100644
> > >>> --- a/Documentation/coding-style.rst
> > >>> +++ b/Documentation/coding-style.rst
> > >>> @@ -81,6 +81,68 @@ C++-11-specific features:
> > >>> overused.
> > >>> * Variadic class and function templates
> > >>>
> > >>> +Object Ownership
> > >>> +~~~~~~~~~~~~~~~~
> > >>> +
> > >>> +libcamera creates and destroys many objects at runtime, for both objects
> > >>> +internal to the library and objects exposed to the user. To guarantee proper
> > >>> +operation without use after free, double free or memory leaks, knowing who owns
> > >>> +each object at any time is crucial. The project has enacted a set of rules to
> > >>> +make object ownership tracking as explicit and fool-proof as possible.
> > >>> +
> > >>> +In the context of this section, the terms object and instance are used
> > >>> +interchangeably and both refer to an instance of a class. The term reference
> > >>> +refers to both C++ references and C++ pointers in their capacity to refer to an
> > >>> +object. Passing a reference means offering a way to a callee to obtain a
> > >>> +reference to an object that the caller has a valid reference to. Borrowing a
> > >>> +reference means using a reference passed by a caller without ownership transfer
> > >>> +based on the assumption that the caller guarantees the validate of the
> > >>
> > >> the validity
> > >
> > > Fixed.
> > >
> > >>> +reference for the duration of the operation that borrows the reference.
> > >>
> > >> maybe just "that borrows it."
> > >
> > > Fixed.
> > >
> > >>> +
> > >>> +#. Single Owner Objects
> > >>> +
> > >>> +   * By default an object has a single owner at any time.
> > >>
> > >> "an object that has a"
> > >
> > > No, I really meant to say that by default an object has a single owner
> > > at any time, to explain that by default objects do not use shared
> > > ownership.
> > >
> >
> >  Ah right, I thougth this was the definition of what a "single owner
> >  object" is...
> >
> >
> > >>> +   * References to a single owner object can be borrowed by passing them from
> > >>> +     the owner to the borrower, providing that
> > >>
> > >> Not sure this was what you wanted to express, but it seems to that
> > >> it would be more clear to state that "Ownership of a single owner
> > >> object can be borrowed, providing that"
> > >
> > > But then I can't use "reference" in the list below. How about
> > >
> > > "A single owner object can be borrowed by passing a reference from the
> > > owner to the borrower, providing that"
> > >
> >  I still feel like it is actually the ownership that is
> >  transferred/moved not the reference itself. This is fine anyway!
>
> When transferring ownership, you're right, but when borrowing the
> object, it's the object that you borrow, not its ownership. I'll ask you
> to borrow your pen, not the ownership of your pen :-)
>
> > >>> +
> > >>> +     * the owner guarantees the validity of the reference for the whole duration
> > >>> +       of borrowing, and
> > >>> +     * the borrower doesn't access the reference after the end of the borrowing.
> > >>> +
> > >>> +     When borrowing from caller to callee for the duration of a function call,
> > >>
> > >> "When borrowing from caller to callee"
> > >> Can you borrow from callee to caller?
> > >
> > > Yes you can. CameraManager::instance() does exactly that, it returns a
> > > reference that is still owned by CameraManager can can be borrowed for
> > > the duration of the libcamera lifetime (which should be long enough :-)).
> > >
> >
> >  Right!
> >
> > >>> +     this implies that the callee shall not keep any stored reference after it
> > >>> +     returns. These rules applies to the callee and all the functions it calls,
> > >>
> > >> rules -> apply
> > >
> > > Fixed.
> > >
> > >>> +     directly or indirectly.
> > >>
> > >> * Ownership is borrowed by accessing the unique_ptr<> owned resources
> > >> in the caller and by declaring the callee function parameter as:
> > >> Your two points below here (const &) and pointer.
> > >> * const &
> > >> * pointers.
> >
> >  Does this point not apply in your opinion?
>
> I missed it, sorry. I'll take this into account in v2.
>
> > >>> +   * When the object doesn't need to be modified and may not be null, borrowed
> > >>> +     references are passed as 'const &'.
> > >>> +   * When the object may be modified or can be null, borrowed references are
> > >>> +     passed as pointers. Unless otherwise specified, pointers passed to
> > >>> +     functions are considered as borrowed references valid for the duration of
> > >>> +     the function only.
> > >>> +   * Single ownership is emphasized as much as possible by storing the unique
> > >>> +     reference as a std::unique_ptr<>.
> > >>
> > >> Shouldn't this be (one of) the first points?
> > >>
> > >>> +   * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> > >>
> > >> s/transfered/transferred
> > >
> > > The spelling "transfered" exists:
> > > https://en.wiktionary.org/wiki/transfered. I'll still fix it :-)
> > >
> >
> >  Ah! my spell checker complained and I didn't look it up online
>
> You spellchecker rightfully complained, as "transfered" is documented as
> "Misspelling of transferred" :-)
>
> > >> * Ownership is transfered by passing the reference as a std::unique_ptr<>.
> > >> or
> > >> * Ownership is transferred by declaring the callee functions
> > >> parameter as std::unique_ptr<>
> > >> (I would use the "by declaring the callee function parameter
> > >> as" in the description of owership borrowing too)
> > >
> > > How about "Ownership transfer is expressed by passing the reference as a
> > > std::unique_ptr<>." ? I'd like to avoid referring to function calls
> > > here, as that's not the only way to transfer ownership:
> > >
> > > struct Foo
> > > {
> > >     std::unique_ptr<Bar> ptr;
> > > };
> > >
> > > Foo a;
> > > Foo b;
> > >
> > > b.ptr = std::move(a.ptr);
> > >
> >
> >  I see... I like your suggestion...
> >
> > > (This will likely not appear as-is in our code of course.)
> >
> >  Hopefully not :)
> >
> > >
> > >> Also, the use of std::unique_ptr<> as function argument implies the object
> > >> ownership gets moved to the callee so it won't be valid in the caller
> > >> afterwards (if not returned by the callee and re-assigned again in the
> > >> caller). Is this worth being described or is it clear enough in the
> > >> definition of what a unique_ptr<> is?
> > >
> > > I think it's clear from the std::unique_ptr<> definition and the concept
> > > of ownership transfer, but I can add the following sentence if you think
> > > it could be useful.
> > >
> > > "After ownership transfer the former owner has no valid reference to the
> > > object anymore and shall not access it without obtaining a valid
> > > reference."
> >
> >  I let you decide if it is worth or not
>
> I'll add it.
>
> > >> Furthermore, do you think moving ownership of a resource to a callee
> > >> function should be marked as a corner case, or indication of an issue
> > >> in the design, or do you see this happening often in practice?
> > >
> > > I think it can happen. See the setEventDispatcher() patch for instance.
> > > The function documentation states that the CameraManager takes ownership
> > > of the dispatcher, but with the patch the std::unique_ptr<> in the
> > > caller becomes null, ensuring that the caller will not erroneously try
> > > to delete the dispatcher later on.
> >
> >  and hopefully won't try to access it :)
> >
> > >>> +
> > >>> +#. Shared Objects
> > >>> +
> > >>> +   * Objects that may have multiple owners at a given time are called shared
> > >>> +     objects. They are reference-counted and live as long as any references to
> > >>> +     the object exist.
> > >>> +   * Shared objects are created with std::make_shared<> or
> > >>> +     std::allocate_shared<> an stored in an std::shared_ptr<>.
> > >>> +   * Borrowed references to shared objects are passed with the same rules as for
> > >>> +     single owner objects.
> > >>
> > >> Yes, but in this case 'borrowing' and 'transferring' of ownership have
> > >> different side effects on the object reference counting. I would
> > >> state that explicitly.
> > >
> > > Note that borrowing a reference to an object managed through a shared
> > > pointer is done by passing a reference to the object itself, not the
> > > shared pointer. I'll clarify this:
> > >
> > > * Borrowed references to shared objects are passed as references to the
> > > object itself, not to the std::shared_ptr<>, with the same rules as for
> > > single owner objects.
> >
> >  Why can't you borrow a reference as "shared_ptr<> &" ? That's
> >  effectively borrowing a shared reference without increasing the
> >  reference count, isn't it?
>
> You can do that too, but it's more complex. Functions that borrow a
> reference shouldn't be written assuming that the object uses shared_ptr,
> unique_ptr or no smart pointer, they should take a reference or pointer
> to the object directly.

I'd vote against passing shared_ptr by reference (i.e. "shared_ptr<>
&"). Semantically it's more clear if we always pass shared_ptr by
value. The caller can decide whether it wants to duplicate the shared
ownership (through copying the shared_ptr), or transferring its own
share of the ownership to the callee (through std::move the
shared_ptr).

Some reference:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions

I understand we're following Google C++ style guide, but I find the
Chromium C++ style guide useful as well. In the link scoped_refptr<T>
is the Chromium's version of std::shared_ptr<T>.

>
> > >>> +   * Ownership is shared by creating and passing copies of any valid
> > >>> +     std::shared_ptr<> reference. Ownership is released by destroying the
> > >>> +     corresponding std::shared_ptr<>.
> > >>> +
> > >>> +.. attention:: Long term borrowing of single owner objects is allowed. Example
> > >>> +   use cases are implementation of the singleton pattern (where the singleton
> > >>> +   guarantees the validity of the reference forever), or returning references
> > >>> +   to global objects whose lifetime matches the lifetime of the application. As
> > >>> +   long term borrowing isn't marked through language constructs, it shall be
> > >>> +   documented explicitly in details in the API.
> > >>
> > >> Thanks, this note is very useful imho.
> > >> Feel free to take in whatever you consider appropriate
> > >
> > > Thank you for your comments. Please let me know if you're fine with the
> > > changes I proposed above, and which ones should be included or dropped.
> > >
> >
> >  I'm fine, please go ahead merging this once you consider it ready.
> >
> > >>> +
> > >>>
> > >>> Tools
> > >>> -----
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel