Re: [PATCH 1/2] Make cached_reg_t own its data

[Pedro Alves <pedro@palves.net>]

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
-- 
2.43.0