[Bug analyzer/106390] New: Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[Bug analyzer/106390] New: Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

            Bug ID: 106390
           Summary: Support gsl::owner<T> and/or [[gnu::owner]] attribute
                    in -fanalyzer
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: redi at gcc dot gnu.org
            Blocks: 97110
  Target Milestone: ---

Check that "owned" resources are freed:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c31-all-resources-acquired-by-a-class-must-be-released-by-the-classs-destructor

This would require the analyzer to recognize the gsl::owner alias and treat it
specially. The definition of gsl::owner is simply a typedef for a raw pointer:

template <class T, class = std::enable_if_t<std::is_pointer<T>::value>>
using owner = T;

(Reference impl at https://github.com/microsoft/GSL but I plan to add a <gsl>
header to libstdc++ too, making use of GCC extensions.)

The point is to permit static analysis to treat that pointer differently to a
non-owning pointer (which just aliases some other object that isn't owned). If
the code just uses T* it's unclear what the semantics of that member are. If it
uses gsl::owner<T*> it's explicit that the class "owns" that pointer and is
directly responsible for deallocating it.

Pointers stored as a gsl::owner must be freed in a destructor, unless ownership
has been transferred to another object via move semantics.

class S {
public:
  S();
  S(S&&);
  ~S() { } // bug! owned resource not freed
private:
  struct Impl;
  gsl::owner<Impl*> m_pimpl;
};

A more general solution would be a new [[gnu::owner]] attribute that can be
added to any data member to say it owns a resource. So the following would be
equivalent to the example above:

class S {
public:
  S();
  S(S&&);
  ~S() { } // bug! owned resource not freed
private:
  struct Impl;
  [[gnu::owner]] Impl* m_pimpl;
};

This attribute would be extensible to non-pointer types such as file
descriptors and other resources where ownership is transferred by move
constructors and freed by destructors etc.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97110
[Bug 97110] [meta-bug] tracker bug for supporting C++ in -fanalyzer

[Bug analyzer/106390] Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
For more details see 
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#gslview-views

[Bug analyzer/106390] Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
As well as checking that destructors free an "owned" member, check that move
constructors and move assignments transfer ownership correctly, i.e. leave the
previous owner empty, and for move assignment, ensure any previous value in the
LHS is freed before being replaced i.e. given class Foo with member
gsl::owner<T*> m_ptr:

Foo&
Foo::operator=(Foo& rhs)
{
  m_ptr = rhs.m_ptr;
  rhs.m_ptr = nullptr;
  return *this;
}

flag a bug, because the previous value of m_ptr is overwritten without freeing
it.

[Bug analyzer/106390] Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[vultkayn at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

Benjamin Priour <vultkayn at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vultkayn at gcc dot gnu.org

--- Comment #3 from Benjamin Priour <vultkayn at gcc dot gnu.org> ---
I think gsl::owner might be insufficient and we should rather introduce
[[gnu::owner(unique)]] and [[gnu::owner(shared)]] 

Let's say we only had [[gnu::owner]] for ownership, whether unique or shared.
If so, annotating [[gnu::owner]] would mean "I am becoming A (not THE) owner of
the given resource", i.e. it would always mean "shared" ownership.

Yet doing so would make the attribute only useful to check spurious
deallocations of non-owned resource, as well as detect the resource has been
released in the destructor, but otherwise useless to check move operations, as
we cannot require a move upon acquiring a shared resource.

Thus an additional attribute will be necessary anyway, either to differentiate
"shared" and "unique" ownership, or to annotate a move operation. 

I believe [[gnu::owner(unique|shared)]] to be preferable, as we can use it to
deduce a move operation, whereas a flagged move does not induce the quality of
ownership.

[Bug analyzer/106390] Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I'm not sure that's worthwhile. Unique ownership is orders of magnitude more
common, and lifetime is managed in smaller scopes. Tracking unique ownership
within a single function is tractable, and useful. The lifetime can be analyzed
for each function in isolation, without interprocedural analysis. Separate
functions that correctly manage unique lifetimes compose into larger components
which also correctly manage those lifetimes.

Analyzing a shared lifetime pointer means also tracking reference counts, and
the lifetime typically extends across functions, maybe even across shared
library boundaries. Checking for correct code cannot be done for a single
function in isolation. If std::shared_ptr is the main example of shared
ownership, we don't need to support that in the analyzer, users should not be
worrying about its correctness.

But users frequently *do* write their own types that manage pointers directly
with unique ownership. That's what the analyzer can help with.

(In reply to Benjamin Priour from comment #3)
> Let's say we only had [[gnu::owner]] for ownership, whether unique or shared.
> If so, annotating [[gnu::owner]] would mean "I am becoming A (not THE) owner
> of the given resource", i.e. it would always mean "shared" ownership.

I agree that this would be a problem, but instead of implying that we need two
attributes, I think this implies that we should not try to use [[gsl::owner]]
for shared ownership. If you don't try to use it for both, the problem doesn't
exist.

I would caution against inventing new attributes here. The reason I suggested
[[gsl::owner]] is because it is already supported by other analysers and so can
be used in portable code.

[Bug analyzer/106390] Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[vultkayn at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

--- Comment #5 from Benjamin Priour <vultkayn at gcc dot gnu.org> ---

> I agree that this would be a problem, but instead of implying that we need
> two attributes, I think this implies that we should not try to use
> [[gsl::owner]] for shared ownership. If you don't try to use it for both,
> the problem doesn't exist.

If we discard tracking shared ownership, it indeed removes a lot of complexity,
and I agree we can stick to [[gsl::owner]], and tracking moves of resources
becomes much simpler.

I'll go with that for now, see what tests I can do, dig a little bit.

[Bug analyzer/106390] Support gsl::owner<T> and/or [[gnu::owner]] attribute in -fanalyzer

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106390

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Related work: http://thradams.com/cake/ownership.html