[1/2] Make cached_reg_t own its data
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
fail
|
Testing failed
|
Commit Message
struct cached_reg_t own its data buffer, but currently that is managed
manually. Convert it to use a unique_xmalloc_ptr.
Change-Id: I05a107098b717299e76de76aaba00d7fbaeac77b
---
gdb/python/py-unwind.c | 13 ++++++-------
gdb/regcache.h | 5 ++++-
gdb/remote.c | 24 ++++++------------------
3 files changed, 16 insertions(+), 26 deletions(-)
Comments
On 2023-12-15 13:12, Pedro Alves wrote:
> struct cached_reg_t own its data buffer, but currently that is managed
> manually. Convert it to use a unique_xmalloc_ptr.
>
> Change-Id: I05a107098b717299e76de76aaba00d7fbaeac77b
Hi Pedro,
When building with clang 17:
CXX python/py-unwind.o
/home/smarchi/src/binutils-gdb/gdb/python/py-unwind.c:126:16: error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
126 | cached_reg_t reg[];
| ^
Simon
On 2023-12-20 23:48, Simon Marchi wrote:
>
>
> On 2023-12-15 13:12, Pedro Alves wrote:
>> struct cached_reg_t own its data buffer, but currently that is managed
>> manually. Convert it to use a unique_xmalloc_ptr.
>>
>> Change-Id: I05a107098b717299e76de76aaba00d7fbaeac77b
>
> Hi Pedro,
>
> When building with clang 17:
>
> CXX python/py-unwind.o
> /home/smarchi/src/binutils-gdb/gdb/python/py-unwind.c:126:16: error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
> 126 | cached_reg_t reg[];
> | ^
>
Bah. Really Clang? Not even a warning instead of an error?
I'm going to apply this to unbreak the build ASAP, but maybe we should really get
rid of this flexible array member and C++-fy the whole of struct cached_frame_info.
From bfcfa995f9461726d57f0d9a2879ba4352547870 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 21 Dec 2023 10:43:20 +0000
Subject: [PATCH] Fix Clang build issue with flexible array member and
non-trivial dtor
Commit d5cebea18e7a ("Make cached_reg_t own its data") added a
destructor to cached_reg_t.
That caused a build problem with Clang, which errors out like so:
> CXX python/py-unwind.o
> gdb/python/py-unwind.c:126:16: error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
> 126 | cached_reg_t reg[];
> | ^
This is is not really a problem for our code, which allocates the
whole structure with xmalloc, and then initializes the array elements
with in-place new, and then takes care to call the destructor
manually. Like, commit d5cebea18e7a did:
@@ -928,7 +927,7 @@ pyuw_dealloc_cache (frame_info *this_frame, void *cache)
cached_frame_info *cached_frame = (cached_frame_info *) cache;
for (int i = 0; i < cached_frame->reg_count; i++)
- xfree (cached_frame->reg[i].data);
+ cached_frame->reg[i].~cached_reg_t ();
Maybe we should get rid of the flexible array member and use a bog
standard std::vector. I doubt this would cause any visible
performance issue.
Meanwhile, to unbreak the build, this commit switches from C99-style
flexible array member to 0-length array. It behaves the same, and
Clang doesn't complain. I got the idea from here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70932#c11
GCC 9, our oldest support version, already supported this:
https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Zero-Length.html
but the extension is actually much older than that. Note that
C99-style flexible array members are not standard C++ either.
Change-Id: I37dda18f367e238a41d610619935b2a0f2acacce
---
gdb/python/py-unwind.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 31b74c67310..8fed55beadc 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -123,7 +123,15 @@ struct cached_frame_info
/* Length of the `reg' array below. */
int reg_count;
- cached_reg_t reg[];
+ /* Flexible array member. Note: use a zero-sized array rather than
+ an actual C99-style flexible array member (unsized array),
+ because the latter would cause an error with Clang:
+
+ error: flexible array member 'reg' of type 'cached_reg_t[]' with non-trivial destruction
+
+ Note we manually call the destructor of each array element in
+ pyuw_dealloc_cache. */
+ cached_reg_t reg[0];
};
extern PyTypeObject pending_frame_object_type
base-commit: 3a4ee6286814e850f66d84b6b8b18cd053649d35
@@ -785,7 +785,7 @@ pyuw_prev_register (frame_info_ptr this_frame, void **cache_ptr,
for (; reg_info < reg_info_end; ++reg_info)
{
if (regnum == reg_info->num)
- return frame_unwind_got_bytes (this_frame, regnum, reg_info->data);
+ return frame_unwind_got_bytes (this_frame, regnum, reg_info->data.get ());
}
return frame_unwind_got_optimized (this_frame, regnum);
@@ -903,15 +903,14 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
struct value *value = value_object_to_value (reg->value.get ());
size_t data_size = register_size (gdbarch, reg->number);
- cached_frame->reg[i].num = reg->number;
-
/* `value' validation was done before, just assert. */
gdb_assert (value != NULL);
gdb_assert (data_size == value->type ()->length ());
- cached_frame->reg[i].data = (gdb_byte *) xmalloc (data_size);
- memcpy (cached_frame->reg[i].data,
- value->contents ().data (), data_size);
+ cached_reg_t *cached = new (&cached_frame->reg[i]) cached_reg_t ();
+ cached->num = reg->number;
+ cached->data.reset ((gdb_byte *) xmalloc (data_size));
+ memcpy (cached->data.get (), value->contents ().data (), data_size);
}
}
@@ -928,7 +927,7 @@ pyuw_dealloc_cache (frame_info *this_frame, void *cache)
cached_frame_info *cached_frame = (cached_frame_info *) cache;
for (int i = 0; i < cached_frame->reg_count; i++)
- xfree (cached_frame->reg[i].data);
+ cached_frame->reg[i].~cached_reg_t ();
xfree (cache);
}
@@ -176,7 +176,10 @@ using register_read_ftype
struct cached_reg_t
{
int num;
- gdb_byte *data;
+ gdb::unique_xmalloc_ptr<gdb_byte> data;
+
+ cached_reg_t () = default;
+ cached_reg_t (cached_reg_t &&rhs) = default;
};
/* Buffer of registers. */
@@ -1353,8 +1353,6 @@ class extended_remote_target final : public remote_target
struct stop_reply : public notif_event
{
- ~stop_reply ();
-
/* The identifier of the thread about this event */
ptid_t ptid;
@@ -7604,12 +7602,6 @@ remote_notif_stop_can_get_pending_events (remote_target *remote,
return 0;
}
-stop_reply::~stop_reply ()
-{
- for (cached_reg_t ® : regcache)
- xfree (reg.data);
-}
-
static notif_event_up
remote_notif_stop_alloc_reply ()
{
@@ -8094,17 +8086,18 @@ Packet: '%s'\n"),
hex_string (pnum), p, buf);
cached_reg.num = reg->regnum;
- cached_reg.data = (gdb_byte *)
- xmalloc (register_size (event->arch, reg->regnum));
+ cached_reg.data.reset ((gdb_byte *)
+ xmalloc (register_size (event->arch,
+ reg->regnum)));
p = p1 + 1;
- fieldsize = hex2bin (p, cached_reg.data,
+ fieldsize = hex2bin (p, cached_reg.data.get (),
register_size (event->arch, reg->regnum));
p += 2 * fieldsize;
if (fieldsize < register_size (event->arch, reg->regnum))
warning (_("Remote reply is too short: %s"), buf);
- event->regcache.push_back (cached_reg);
+ event->regcache.push_back (std::move (cached_reg));
}
else
{
@@ -8436,12 +8429,7 @@ remote_target::process_stop_reply (struct stop_reply *stop_reply,
stop_reply->arch);
for (cached_reg_t ® : stop_reply->regcache)
- {
- regcache->raw_supply (reg.num, reg.data);
- xfree (reg.data);
- }
-
- stop_reply->regcache.clear ();
+ regcache->raw_supply (reg.num, reg.data.get ());
}
remote_thread_info *remote_thr = get_remote_thread_info (this, ptid);