[PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack

[PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack

[Simon Marchi]

Building with clang++-14, I see:

      CXX    dwarf2/cooked-index.o
    In file included from /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:21:
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:172:12: error: explicitly defaulted move constructor is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
      explicit cooked_index (cooked_index &&other) = default;
               ^
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:225:16: note: move constructor of 'cooked_index' is implicitly deleted because field 'm_storage' has a deleted move constructor
      auto_obstack m_storage;
                   ^
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb_obstack.h:128:28: note: 'auto_obstack' has been explicitly marked deleted here
      DISABLE_COPY_AND_ASSIGN (auto_obstack);
                               ^
    In file included from /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:21:
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:174:17: error: explicitly defaulted move assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
      cooked_index &operator= (cooked_index &&other) = default;
                    ^
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:225:16: note: move assignment operator of 'cooked_index' is implicitly deleted because field 'm_storage' has a deleted move assignment operator
      auto_obstack m_storage;
                   ^
    /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb_obstack.h:128:3: note: 'operator=' has been explicitly marked deleted here
      DISABLE_COPY_AND_ASSIGN (auto_obstack);
      ^
    /home/smarchi/src/binutils-gdb/gdb/../include/ansidecl.h:425:8: note: expanded from macro 'DISABLE_COPY_AND_ASSIGN'
      void operator= (const TYPE &) = delete
           ^

We explicitly make cooked_index have a default move constructor and
move assignment operator.  But it doesn't actually happen because
cooked_index has a field of type auto_obstack, which isn't movable.  The
solution, if we want to make cooked_index movable, is to also make
auto_obstack movable.

This patch does it by adding a flag in auto_obstack to denote whether
this auto_obstack instance still owns the obstack.  When moving from
an auto_obstack to another, the source auto_obstack stops being an
owner.  The destination auto_obstack destroys its current obstack if it
owns it.

Change-Id: Ifc1fe3d7d67e3ae1a14363d6c1869936fe80b0a2
---
 gdbsupport/gdb_obstack.h | 41 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/gdbsupport/gdb_obstack.h b/gdbsupport/gdb_obstack.h
index 5e870cb79810..cb915b20994d 100644
--- a/gdbsupport/gdb_obstack.h
+++ b/gdbsupport/gdb_obstack.h
@@ -122,8 +122,21 @@ struct auto_obstack : obstack
   auto_obstack ()
   { obstack_init (this); }
 
+  auto_obstack (auto_obstack &&other)
+  {
+    move_from_other (std::move (other));
+  }
+
+  auto_obstack &operator= (auto_obstack &&other)
+  {
+    maybe_free ();
+    move_from_other (std::move (other));
+
+    return *this;
+  }
+
   ~auto_obstack ()
-  { obstack_free (this, NULL); }
+  { maybe_free (); }
 
   DISABLE_COPY_AND_ASSIGN (auto_obstack);
 
@@ -131,6 +144,32 @@ struct auto_obstack : obstack
      allocation.  */
   void clear ()
   { obstack_free (this, obstack_base (this)); }
+
+private:
+  void move_from_other (auto_obstack &&other)
+  {
+    /* Copy the base fields from the other obstack.  */
+    obstack *this_obstack = this;
+    obstack *other_obstack = &other;
+    *this_obstack = *other_obstack;
+
+    /* Clear the base fields of the other obstack to make sure something does
+       not use it by mistake.  */
+    memset (other_obstack, '\0', sizeof (*other_obstack));
+
+    m_must_free = other.m_must_free;
+    other.m_must_free = false;
+  }
+
+  void maybe_free ()
+  {
+    if (!m_must_free)
+      return;
+
+    obstack_free (this, nullptr);
+  }
+
+  bool m_must_free = true;
 };
 
 /* Objects are allocated on obstack instead of heap.  */
-- 
2.26.2

Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack

[Pedro Alves]

On 2022-04-13 19:59, Simon Marchi via Gdb-patches wrote:
> Building with clang++-14, I see:
> 
>       CXX    dwarf2/cooked-index.o
>     In file included from /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:21:
>     /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:172:12: error: explicitly defaulted move constructor is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
>       explicit cooked_index (cooked_index &&other) = default;
>                ^
>     /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:225:16: note: move constructor of 'cooked_index' is implicitly deleted because field 'm_storage' has a deleted move constructor
>       auto_obstack m_storage;
>                    ^
>     /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb_obstack.h:128:28: note: 'auto_obstack' has been explicitly marked deleted here
>       DISABLE_COPY_AND_ASSIGN (auto_obstack);
>                                ^
>     In file included from /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:21:
>     /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:174:17: error: explicitly defaulted move assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
>       cooked_index &operator= (cooked_index &&other) = default;
>                     ^
>     /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.h:225:16: note: move assignment operator of 'cooked_index' is implicitly deleted because field 'm_storage' has a deleted move assignment operator
>       auto_obstack m_storage;
>                    ^
>     /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/gdb_obstack.h:128:3: note: 'operator=' has been explicitly marked deleted here
>       DISABLE_COPY_AND_ASSIGN (auto_obstack);
>       ^
>     /home/smarchi/src/binutils-gdb/gdb/../include/ansidecl.h:425:8: note: expanded from macro 'DISABLE_COPY_AND_ASSIGN'
>       void operator= (const TYPE &) = delete
>            ^
> 
> We explicitly make cooked_index have a default move constructor and
> move assignment operator.  But it doesn't actually happen because
> cooked_index has a field of type auto_obstack, which isn't movable.  The
> solution, if we want to make cooked_index movable, is to also make
> auto_obstack movable.
> 
> This patch does it by adding a flag in auto_obstack to denote whether
> this auto_obstack instance still owns the obstack.  When moving from
> an auto_obstack to another, the source auto_obstack stops being an
> owner.  The destination auto_obstack destroys its current obstack if it
> owns it.

The patch looks fine if we really want to make auto_obstack movable, but,
stepping back a bit, does cooked_index really need or want to be movable?
I did this, here:

diff --git c/gdb/dwarf2/cooked-index.h w/gdb/dwarf2/cooked-index.h
index 661664d9f84..e3d8b5a59b7 100644
--- c/gdb/dwarf2/cooked-index.h
+++ w/gdb/dwarf2/cooked-index.h
@@ -169,9 +169,9 @@ class cooked_index
 {
 public:
   cooked_index () = default;
-  explicit cooked_index (cooked_index &&other) = default;
+  explicit cooked_index (cooked_index &&other) = delete;
   DISABLE_COPY_AND_ASSIGN (cooked_index);
-  cooked_index &operator= (cooked_index &&other) = default;
+  cooked_index &operator= (cooked_index &&other) = delete;

and gdb still built.

Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack

[Simon Marchi]

> The patch looks fine if we really want to make auto_obstack movable, but,
> stepping back a bit, does cooked_index really need or want to be movable?
> I did this, here:
> 
> diff --git c/gdb/dwarf2/cooked-index.h w/gdb/dwarf2/cooked-index.h
> index 661664d9f84..e3d8b5a59b7 100644
> --- c/gdb/dwarf2/cooked-index.h
> +++ w/gdb/dwarf2/cooked-index.h
> @@ -169,9 +169,9 @@ class cooked_index
>  {
>  public:
>    cooked_index () = default;
> -  explicit cooked_index (cooked_index &&other) = default;
> +  explicit cooked_index (cooked_index &&other) = delete;
>    DISABLE_COPY_AND_ASSIGN (cooked_index);
> -  cooked_index &operator= (cooked_index &&other) = default;
> +  cooked_index &operator= (cooked_index &&other) = delete;
> 
> and gdb still built.

Hmm, I saw that do an std::move of a cooked_index_vector, so I thought
we did move cooked_indexes.  But actually it is a vector of
unique_ptr<cooked_index>.  And I guess that since auto_obstack is
non-copyable and non-movable, it wouldn't build today if we really tried
to copy or move a cooked_index.

We can just delete those lines then (instead of making them explicitly
deleted), just having DISABLE_COPY_AND_ASSIGN is enough.

Simon

Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack

[Pedro Alves]

On 2022-04-14 16:12, Simon Marchi wrote:
>> The patch looks fine if we really want to make auto_obstack movable, but,
>> stepping back a bit, does cooked_index really need or want to be movable?
>> I did this, here:
>>
>> diff --git c/gdb/dwarf2/cooked-index.h w/gdb/dwarf2/cooked-index.h
>> index 661664d9f84..e3d8b5a59b7 100644
>> --- c/gdb/dwarf2/cooked-index.h
>> +++ w/gdb/dwarf2/cooked-index.h
>> @@ -169,9 +169,9 @@ class cooked_index
>>  {
>>  public:
>>    cooked_index () = default;
>> -  explicit cooked_index (cooked_index &&other) = default;
>> +  explicit cooked_index (cooked_index &&other) = delete;
>>    DISABLE_COPY_AND_ASSIGN (cooked_index);
>> -  cooked_index &operator= (cooked_index &&other) = default;
>> +  cooked_index &operator= (cooked_index &&other) = delete;
>>
>> and gdb still built.
> 
> Hmm, I saw that do an std::move of a cooked_index_vector, so I thought
> we did move cooked_indexes.  But actually it is a vector of
> unique_ptr<cooked_index>.  And I guess that since auto_obstack is
> non-copyable and non-movable, it wouldn't build today if we really tried
> to copy or move a cooked_index.
> 
> We can just delete those lines then (instead of making them explicitly
> deleted), just having DISABLE_COPY_AND_ASSIGN is enough.

Please go ahead.

Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack

[Simon Marchi]

On 2022-04-14 11:22, Pedro Alves wrote:
> On 2022-04-14 16:12, Simon Marchi wrote:
>>> The patch looks fine if we really want to make auto_obstack movable, but,
>>> stepping back a bit, does cooked_index really need or want to be movable?
>>> I did this, here:
>>>
>>> diff --git c/gdb/dwarf2/cooked-index.h w/gdb/dwarf2/cooked-index.h
>>> index 661664d9f84..e3d8b5a59b7 100644
>>> --- c/gdb/dwarf2/cooked-index.h
>>> +++ w/gdb/dwarf2/cooked-index.h
>>> @@ -169,9 +169,9 @@ class cooked_index
>>>  {
>>>  public:
>>>    cooked_index () = default;
>>> -  explicit cooked_index (cooked_index &&other) = default;
>>> +  explicit cooked_index (cooked_index &&other) = delete;
>>>    DISABLE_COPY_AND_ASSIGN (cooked_index);
>>> -  cooked_index &operator= (cooked_index &&other) = default;
>>> +  cooked_index &operator= (cooked_index &&other) = delete;
>>>
>>> and gdb still built.
>>
>> Hmm, I saw that do an std::move of a cooked_index_vector, so I thought
>> we did move cooked_indexes.  But actually it is a vector of
>> unique_ptr<cooked_index>.  And I guess that since auto_obstack is
>> non-copyable and non-movable, it wouldn't build today if we really tried
>> to copy or move a cooked_index.
>>
>> We can just delete those lines then (instead of making them explicitly
>> deleted), just having DISABLE_COPY_AND_ASSIGN is enough.
> 
> Please go ahead.

Done, thanks.

simon