* [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack
@ 2022-04-13 18:59 Simon Marchi
2022-04-14 14:52 ` Pedro Alves
0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2022-04-13 18:59 UTC (permalink / raw)
To: gdb-patches; +Cc: 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack
2022-04-13 18:59 [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack Simon Marchi
@ 2022-04-14 14:52 ` Pedro Alves
2022-04-14 15:12 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Pedro Alves @ 2022-04-14 14:52 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack
2022-04-14 14:52 ` Pedro Alves
@ 2022-04-14 15:12 ` Simon Marchi
2022-04-14 15:22 ` Pedro Alves
0 siblings, 1 reply; 5+ messages in thread
From: Simon Marchi @ 2022-04-14 15:12 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack
2022-04-14 15:12 ` Simon Marchi
@ 2022-04-14 15:22 ` Pedro Alves
2022-04-14 15:32 ` Simon Marchi
0 siblings, 1 reply; 5+ messages in thread
From: Pedro Alves @ 2022-04-14 15:22 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi, gdb-patches
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack
2022-04-14 15:22 ` Pedro Alves
@ 2022-04-14 15:32 ` Simon Marchi
0 siblings, 0 replies; 5+ messages in thread
From: Simon Marchi @ 2022-04-14 15:32 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-04-14 15:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 18:59 [PATCH] gdbsupport: implement move constructor and assignment operator in auto_obstack Simon Marchi
2022-04-14 14:52 ` Pedro Alves
2022-04-14 15:12 ` Simon Marchi
2022-04-14 15:22 ` Pedro Alves
2022-04-14 15:32 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).