Re: [PATCH 1/3] Introduce gdb::unique_ptr

[Pedro Alves <palves@redhat.com>]

On 10/11/2016 07:47 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Monday, October 10, 2016 6:47 PM
>> To: gdb-patches@sourceware.org
>> Subject: [PATCH 1/3] Introduce gdb::unique_ptr
> 
> Hello Pedro,

Hi Markus,

> 
>> Many make_cleanup uses in the code base are best eliminated by using a
>> "owning" smart pointer to manage ownership of the resource
>> automatically.
>>
>> The question is _which_ smart pointer.
>>
>> We have std::auto_ptr in C++03, but, as is collective wisdom by now,
>> that's too easy to misuse, and has therefore been deprecated in C++11
>> and finally removed in C++17.
> 
> Would it make sense to switch to C++11, instead?  

I think it would make a lot of sense to switch to C++11.  I'd love
that.  rvalue references support, move-aware containers, "auto",
std::unique_ptr and std::shared_ptr would be all very nice to have.

The only question in my mind is -- are people OK with requiring
gcc 4.8 or later?

I think gcc 4.8 or newer were available in Fedora 20.  I believe
Ubuntu 12.04 had it available as an option.  On older RHEL systems, 
it's available in DTS.

On older systems, you'd need to compile a newer gcc or clang first
before building gdb.  Would people find that acceptable?  Or put
another way, would anyone find that unacceptable?

But meanwhile, I don't want C++ conversion to be blocked
by that.  Hence this new shim.

Even if we don't require C++11, I think it'd make sense to compile
in C++11 mode if we have a capable compiler.  I.e., if you have
gcc 4.8 or newer, pass -std=gnu++11/-std=gnu++0x if necessary.

> Or delay this work until we can make this move?

No, I don't think so.  I think we need _some_ owning smart pointer.

An alternative is some separate RAII object that destroys a pointer
when leaving a scope:

  template <class T>
  struct cleanup_pointer
  {
    explicit cleanup_pointer(T *p) : m_ptr (p) {}
    cleanup_pointer { if (m_ptr != NULL) delete m_ptr; }
    void discard () { m_ptr = NULL; }
    T *m_ptr;
  };

  // Document that this returns a new object that must
  // be released with "delete".
  struct foo *function_returning_new_ptr ();

  // Document that this uses P, ownership is still with the caller.
  // May throw.
  void foo (struct foo *p);

  // Document that P is no longer owned by the caller after this.
  // May throw.
  void bar (struct foo *p);

  void some_function ()
  {
    struct foo *ptr = function_returning_new_ptr ();
    cleanup_pointer<struct foo *> cleanup (ptr);

    ptr->foo = .....;
    bar (ptr); // whoops, bar now owns the pointer, but
               // cleanup_pointer will delete it anyway
               // once we leave this scope
  
    // would need to remember to call this.
    // cleanup.discard ();
  }

That looks very much like our current cleanups, except we
no longer need the "do_cleanups" call.  But it's far from idiomatic
C++.   And, ends up being more code to write/maintain
than using a smart pointer.  We can do better.  E.g.,:

  // (gdb::unique_ptr is like std::unique_ptr and
  // gdb::move is like std::move for gdb::unique_ptr)

  // prototype clearly tells you this returns a new object.
  gdb::unique_ptr <struct foo> function_returning_new_ptr ();

  // Uses P, ownership is still with the caller.
  void foo (struct foo *p);

  // Clearly tells you this transfers ownership.
  void bar (gdb::unique_ptr <struct foo> p);

  void some_function ()
  {
    // struct foo *ptr = function_returning_new_ptr (); // would not compile.
    gdb::unique_ptr <struct foo> ptr
       = function_returning_new_ptr (); // OK, ownership clearly transferred to caller.

    ptr->foo = .....;
    foo (ptr.get ()); // clearly foo borrows the pointer.

    bar (gdb::move (ptr)); // clearly transfers ownership.
  }

The other use case a smart pointer clarifies is making
a structure/class be the owner of some pointer.  Like:

 struct foo
 {
   gdb::unique_ptr <struct foo> ptr;
 };

When a "foo" is destroyed, then ptr is destroyed automatically
as well.  Without the smart pointer, we'd have to explicitly
write delete ptr in a destructor:

 struct foo
 {
   ~foo() { delete ptr; }
   struct foo *ptr;
 };


Most C++03 projects past a size/history will bake their own
custom smart pointers for the use cases above.  Most large projects
will have invented theirs before std::unique_ptr existed though, so
they'll differ in API details.   However in our case, std::unique_ptr is
already there, so I think it makes a lot of sense to reuse the same
API, and take advantage of newer compilers as "lints", since
that's what people will be using for development anyway.  Once we do
require C++11, then we won't have to relearn a new smart pointer API.


Even if we were to use std::auto_ptr only, I would still think that that
would be an improvement over raw pointers, since with raw pointers
it's even easier to confuse "borrowing pointers" from "owning pointers",
since you have 0 zero help from the type system.  

The potential problems with std::auto_ptr stem from its
"move semantics", and are of the form:

  std::auto_ptr<type> ptr = function_returning_type ();

  std::auto_ptr<type> ptr2 = ptr;  // this moves instead of
                                   // copying.  I.e., ptr is NULL after this.

and:

  void function (std::auto_ptr<type> ptr);
  function (ptr);  // This moves instead of copying.  ptr is NULL after this.
  ptr->foo = 1; // crash.

In the latter case, the function prototype indicates that the caller
wants to transfer ownership of the pointer to the callee.  But that's
not clear at the call site.

So in C++03 mode, gdb::unique_ptr "allows" that too.  I.e., the compiler
won't complain.  However, that _won't_ compile in C++11 mode, forcing you
to write instead:

  gdb::unique_ptr<type> ptr2 = gdb::move (ptr);  // OK, now move is clearly
                                                 // what was intended.
  function (gdb::move (ptr2));  // Likewise.
  ptr2->foo = 1; // crash, but, obviously incorrect.

And of course the C++03 gdb::unique_ptr version supports this as well.

There's also the advantage that gdb::unique_ptr works with malloced
objects, and arrays (for 'new some_type[];').  We'd need _some_ smart
pointer for those, so std::auto_ptr alone won't be sufficient anyway...

WDYT?

Thanks,
Pedro Alves