Make dwarf_expr_piece::pieces an std::vector
Commit Message
Change the manually managed array dwarf_expr_piece::piece with an
std::vector. After passing the pieces array to allocate_piece_closure,
dwarf2_evaluate_loc_desc_full doesn't need that data anymore. We can
therefore move the content of the vector to avoid copying it.
Reg-tested on the buildbot.
gdb/ChangeLog:
* dwarf2expr.h (struct dwarf_expr_piece): Move up.
(struct dwarf_expr_context) <n_pieces>: Remove.
<pieces>: Change type to std::vector.
* dwarf2expr.c (dwarf_expr_context::dwarf_expr_context): Adjust.
(dwarf_expr_context::~dwarf_expr_context): Don't manually free
pieces.
(dwarf_expr_context::add_piece): Adjust.
* dwarf2loc.c (struct piece_closure): Initialize fields.
<n_pieces>: Remove.
<pieces>: Change type to std::vector.
(allocate_piece_closure): Adjust, change parameter to
std::vector rvalue and std::move it to piece_closure.
(rw_pieced_value): Adjust.
(check_pieced_synthetic_pointer): Adjust.
(indirect_synthetic_pointer): Adjust.
(coerce_pieced_ref): Adjust.
(free_pieced_value_closure): Adjust. Use delete to free
piece_closure.
(dwarf2_evaluate_loc_desc_full): Adjust. std::move ctx.pieces
to allocate_piece_closure.
(dwarf2_loc_desc_get_symbol_read_needs): Adjust.
---
gdb/dwarf2expr.c | 14 ++------
gdb/dwarf2expr.h | 101 +++++++++++++++++++++++++++----------------------------
gdb/dwarf2loc.c | 77 ++++++++++++++++++------------------------
3 files changed, 84 insertions(+), 108 deletions(-)
Comments
On 09/14/2017 09:15 AM, Simon Marchi wrote:
> static struct piece_closure *
> allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
> - int n_pieces, struct dwarf_expr_piece *pieces,
> + const std::vector<dwarf_expr_piece> &&pieces,
const rval reference looks odd -- you can't really move
the internal elements out of a const vector. I think ...
> struct frame_info *frame)
> {
> - struct piece_closure *c = XCNEW (struct piece_closure);
> + struct piece_closure *c = new piece_closure;
> int i;
>
> c->refc = 1;
> c->per_cu = per_cu;
> - c->n_pieces = n_pieces;
> - c->pieces = XCNEWVEC (struct dwarf_expr_piece, n_pieces);
> + c->pieces = std::move (pieces);
... this isn't really moving, but actually copying, because
you'll end up calling 'std::vector(const std::vector &)', not
'std::vector(std::vector &&)'.
I wonder if there's ever a case for 'const T &&' parameters,
and if GCC could warn about them. At least in non-template
functions.
Thanks,
Pedro Alves
On 2017-09-14 11:21, Pedro Alves wrote:
> On 09/14/2017 09:15 AM, Simon Marchi wrote:
>
>> static struct piece_closure *
>> allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
>> - int n_pieces, struct dwarf_expr_piece *pieces,
>> + const std::vector<dwarf_expr_piece> &&pieces,
>
> const rval reference looks odd -- you can't really move
> the internal elements out of a const vector. I think ...
>
>> struct frame_info *frame)
>> {
>> - struct piece_closure *c = XCNEW (struct piece_closure);
>> + struct piece_closure *c = new piece_closure;
>> int i;
>>
>> c->refc = 1;
>> c->per_cu = per_cu;
>> - c->n_pieces = n_pieces;
>> - c->pieces = XCNEWVEC (struct dwarf_expr_piece, n_pieces);
>> + c->pieces = std::move (pieces);
>
> ... this isn't really moving, but actually copying, because
> you'll end up calling 'std::vector(const std::vector &)', not
> 'std::vector(std::vector &&)'.
>
> I wonder if there's ever a case for 'const T &&' parameters,
> and if GCC could warn about them. At least in non-template
> functions.
Oops, I started with a const reference, before changing it to rvalue
reference (and forgot to remove the const). Does the patch look good to
you otherwise?
Simon
On 09/14/2017 10:26 AM, Simon Marchi wrote:
> On 2017-09-14 11:21, Pedro Alves wrote:
>> I wonder if there's ever a case for 'const T &&' parameters,
>> and if GCC could warn about them. At least in non-template
>> functions.
>
FYI, I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82213 now.
> Oops, I started with a const reference, before changing it to rvalue
> reference (and forgot to remove the const). Does the patch look good to
> you otherwise?
>
Sure, with tiny nit below.
> /* Expand the memory allocated stack to contain at least
> @@ -285,14 +282,9 @@ dwarf_expr_context::stack_empty_p () const
> void
> dwarf_expr_context::add_piece (ULONGEST size, ULONGEST offset)
> {
> - struct dwarf_expr_piece *p;
> + this->pieces.emplace_back ();
> + dwarf_expr_piece *p = &(this->pieces.back ());
>
The extra parens look odd to me:
- dwarf_expr_piece *p = &(this->pieces.back ());
+ dwarf_expr_piece *p = &this->pieces.back ();
('p' could be a reference instead, since it's always
non-null, but I guess even better would be to move most
of the code below to a dwarf_expr_piece ctor. But you don't
really have to do that. I definitely understand not wanting
to change too much at a time. And I wouldn't have said a thing
if it weren't for the parens making me pause. :-))
Thanks,
Pedro Alves
On 2017-09-14 12:24, Pedro Alves wrote:
> On 09/14/2017 10:26 AM, Simon Marchi wrote:
>> On 2017-09-14 11:21, Pedro Alves wrote:
>
>>> I wonder if there's ever a case for 'const T &&' parameters,
>>> and if GCC could warn about them. At least in non-template
>>> functions.
>>
>
> FYI, I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82213 now.
Thanks, I couldn't have stated it as clearly as you did.
>> Oops, I started with a const reference, before changing it to rvalue
>> reference (and forgot to remove the const). Does the patch look good
>> to
>> you otherwise?
>>
>
> Sure, with tiny nit below.
>
>> /* Expand the memory allocated stack to contain at least
>> @@ -285,14 +282,9 @@ dwarf_expr_context::stack_empty_p () const
>> void
>> dwarf_expr_context::add_piece (ULONGEST size, ULONGEST offset)
>> {
>> - struct dwarf_expr_piece *p;
>> + this->pieces.emplace_back ();
>> + dwarf_expr_piece *p = &(this->pieces.back ());
>>
>
> The extra parens look odd to me:
>
> - dwarf_expr_piece *p = &(this->pieces.back ());
> + dwarf_expr_piece *p = &this->pieces.back ();
>
> ('p' could be a reference instead, since it's always
> non-null, but I guess even better would be to move most
> of the code below to a dwarf_expr_piece ctor. But you don't
> really have to do that. I definitely understand not wanting
> to change too much at a time. And I wouldn't have said a thing
> if it weren't for the parens making me pause. :-))
I initially left it as a pointer out of laziness, but I've changed it to
a reference now. I am pushing the patch with this and the const rvalue
reference fixed.
I tried to add some constructors, but I didn't know how to design it
nicely to support different types of value locations. Maybe the best
would be to add some factory functions that build various kinds of
pieces, and make the constructor private. Something like:
struct dwarf_expr_piece
{
static dwarf_expr_piece make_memory_piece (CORE_ADDR addr, bool
in_stack_memory)
static dwarf_expr_piece make_register_piece (int regno);
private:
dwarf_expr_piece ();
};
I'll try that for a follow-up patch.
Simon
On 09/14/2017 02:57 PM, Simon Marchi wrote:
> I initially left it as a pointer out of laziness, but I've changed it to
> a reference now. I am pushing the patch with this and the const rvalue
> reference fixed.
>
> I tried to add some constructors, but I didn't know how to design it
> nicely to support different types of value locations.
Ah, yes, now that I look better, I see how the function is calling
private context functions.
> Maybe the best
> would be to add some factory functions that build various kinds of
> pieces, and make the constructor private. Something like:
>
> struct dwarf_expr_piece
> {
> static dwarf_expr_piece make_memory_piece (CORE_ADDR addr, bool
> in_stack_memory)
> static dwarf_expr_piece make_register_piece (int regno);
>
> private:
> dwarf_expr_piece ();
> };
>
> I'll try that for a follow-up patch.
That might work. I'm fine with the status quo too, btw.
Thanks,
Pedro Alves
@@ -100,9 +100,7 @@ dwarf_expr_context::dwarf_expr_context ()
location (DWARF_VALUE_MEMORY),
len (0),
data (NULL),
- initialized (0),
- num_pieces (0),
- pieces (NULL)
+ initialized (0)
{
this->stack = XNEWVEC (struct dwarf_stack_value, this->stack_allocated);
}
@@ -112,7 +110,6 @@ dwarf_expr_context::dwarf_expr_context ()
dwarf_expr_context::~dwarf_expr_context ()
{
xfree (this->stack);
- xfree (this->pieces);
}
/* Expand the memory allocated stack to contain at least
@@ -285,14 +282,9 @@ dwarf_expr_context::stack_empty_p () const
void
dwarf_expr_context::add_piece (ULONGEST size, ULONGEST offset)
{
- struct dwarf_expr_piece *p;
+ this->pieces.emplace_back ();
+ dwarf_expr_piece *p = &(this->pieces.back ());
- this->num_pieces++;
-
- this->pieces
- = XRESIZEVEC (struct dwarf_expr_piece, this->pieces, this->num_pieces);
-
- p = &this->pieces[this->num_pieces - 1];
p->location = this->location;
p->size = size;
p->offset = offset;
@@ -49,6 +49,53 @@ enum dwarf_value_location
DWARF_VALUE_IMPLICIT_POINTER
};
+/* A piece of an object, as recorded by DW_OP_piece or DW_OP_bit_piece. */
+struct dwarf_expr_piece
+{
+ enum dwarf_value_location location;
+
+ union
+ {
+ struct
+ {
+ /* This piece's address, for DWARF_VALUE_MEMORY pieces. */
+ CORE_ADDR addr;
+ /* Non-zero if the piece is known to be in memory and on
+ the program's stack. */
+ int in_stack_memory;
+ } mem;
+
+ /* The piece's register number, for DWARF_VALUE_REGISTER pieces. */
+ int regno;
+
+ /* The piece's literal value, for DWARF_VALUE_STACK pieces. */
+ struct value *value;
+
+ struct
+ {
+ /* A pointer to the data making up this piece,
+ for DWARF_VALUE_LITERAL pieces. */
+ const gdb_byte *data;
+ /* The length of the available data. */
+ ULONGEST length;
+ } literal;
+
+ /* Used for DWARF_VALUE_IMPLICIT_POINTER. */
+ struct
+ {
+ /* The referent DIE from DW_OP_implicit_pointer. */
+ sect_offset die_sect_off;
+ /* The byte offset into the resulting data. */
+ LONGEST offset;
+ } ptr;
+ } v;
+
+ /* The length of the piece, in bits. */
+ ULONGEST size;
+ /* The piece offset, in bits. */
+ ULONGEST offset;
+};
+
/* The dwarf expression stack. */
struct dwarf_stack_value
@@ -114,8 +161,7 @@ struct dwarf_expr_context
initialized; zero otherwise. */
int initialized;
- /* An array of pieces. PIECES points to its first element;
- NUM_PIECES is its length.
+ /* A vector of pieces.
Each time DW_OP_piece is executed, we add a new element to the
end of this array, recording the current top of the stack, the
@@ -137,8 +183,7 @@ struct dwarf_expr_context
no DW_OP_piece operations have no value to place in a piece's
'size' field; the size comes from the surrounding data. So the
two cases need to be handled separately.) */
- int num_pieces;
- struct dwarf_expr_piece *pieces;
+ std::vector<dwarf_expr_piece> pieces;
/* Return the value of register number REGNUM (a DWARF register number),
read as an address. */
@@ -213,54 +258,6 @@ private:
void pop ();
};
-
-/* A piece of an object, as recorded by DW_OP_piece or DW_OP_bit_piece. */
-struct dwarf_expr_piece
-{
- enum dwarf_value_location location;
-
- union
- {
- struct
- {
- /* This piece's address, for DWARF_VALUE_MEMORY pieces. */
- CORE_ADDR addr;
- /* Non-zero if the piece is known to be in memory and on
- the program's stack. */
- int in_stack_memory;
- } mem;
-
- /* The piece's register number, for DWARF_VALUE_REGISTER pieces. */
- int regno;
-
- /* The piece's literal value, for DWARF_VALUE_STACK pieces. */
- struct value *value;
-
- struct
- {
- /* A pointer to the data making up this piece,
- for DWARF_VALUE_LITERAL pieces. */
- const gdb_byte *data;
- /* The length of the available data. */
- ULONGEST length;
- } literal;
-
- /* Used for DWARF_VALUE_IMPLICIT_POINTER. */
- struct
- {
- /* The referent DIE from DW_OP_implicit_pointer. */
- sect_offset die_sect_off;
- /* The byte offset into the resulting data. */
- LONGEST offset;
- } ptr;
- } v;
-
- /* The length of the piece, in bits. */
- ULONGEST size;
- /* The piece offset, in bits. */
- ULONGEST offset;
-};
-
void dwarf_expr_require_composition (const gdb_byte *, const gdb_byte *,
const char *);
@@ -1479,16 +1479,13 @@ value_of_dwarf_block_entry (struct type *type, struct frame_info *frame,
struct piece_closure
{
/* Reference count. */
- int refc;
+ int refc = 0;
/* The CU from which this closure's expression came. */
- struct dwarf2_per_cu_data *per_cu;
-
- /* The number of pieces used to describe this variable. */
- int n_pieces;
+ struct dwarf2_per_cu_data *per_cu = NULL;
- /* The pieces themselves. */
- struct dwarf_expr_piece *pieces;
+ /* The pieces describing this variable. */
+ std::vector<dwarf_expr_piece> pieces;
/* Frame ID of frame to which a register value is relative, used
only by DWARF_VALUE_REGISTER. */
@@ -1500,25 +1497,23 @@ struct piece_closure
static struct piece_closure *
allocate_piece_closure (struct dwarf2_per_cu_data *per_cu,
- int n_pieces, struct dwarf_expr_piece *pieces,
+ const std::vector<dwarf_expr_piece> &&pieces,
struct frame_info *frame)
{
- struct piece_closure *c = XCNEW (struct piece_closure);
+ struct piece_closure *c = new piece_closure;
int i;
c->refc = 1;
c->per_cu = per_cu;
- c->n_pieces = n_pieces;
- c->pieces = XCNEWVEC (struct dwarf_expr_piece, n_pieces);
+ c->pieces = std::move (pieces);
if (frame == NULL)
c->frame_id = null_frame_id;
else
c->frame_id = get_frame_id (frame);
- memcpy (c->pieces, pieces, n_pieces * sizeof (struct dwarf_expr_piece));
- for (i = 0; i < n_pieces; ++i)
- if (c->pieces[i].location == DWARF_VALUE_STACK)
- value_incref (c->pieces[i].v.value);
+ for (dwarf_expr_piece &piece : c->pieces)
+ if (piece.location == DWARF_VALUE_STACK)
+ value_incref (piece.v.value);
return c;
}
@@ -1816,10 +1811,10 @@ rw_pieced_value (struct value *v, struct value *from)
max_offset = 8 * TYPE_LENGTH (value_type (v));
/* Advance to the first non-skipped piece. */
- for (i = 0; i < c->n_pieces && bits_to_skip >= c->pieces[i].size; i++)
+ for (i = 0; i < c->pieces.size () && bits_to_skip >= c->pieces[i].size; i++)
bits_to_skip -= c->pieces[i].size;
- for (; i < c->n_pieces && offset < max_offset; i++)
+ for (; i < c->pieces.size () && offset < max_offset; i++)
{
struct dwarf_expr_piece *p = &c->pieces[i];
size_t this_size_bits, this_size;
@@ -2079,7 +2074,7 @@ check_pieced_synthetic_pointer (const struct value *value, LONGEST bit_offset,
if (value_bitsize (value))
bit_offset += value_bitpos (value);
- for (i = 0; i < c->n_pieces && bit_length > 0; i++)
+ for (i = 0; i < c->pieces.size () && bit_length > 0; i++)
{
struct dwarf_expr_piece *p = &c->pieces[i];
size_t this_size_bits = p->size;
@@ -2200,7 +2195,7 @@ indirect_pieced_value (struct value *value)
if (value_bitsize (value))
bit_offset += value_bitpos (value);
- for (i = 0; i < c->n_pieces && bit_length > 0; i++)
+ for (i = 0; i < c->pieces.size () && bit_length > 0; i++)
{
struct dwarf_expr_piece *p = &c->pieces[i];
size_t this_size_bits = p->size;
@@ -2271,11 +2266,12 @@ coerce_pieced_ref (const struct value *value)
/* gdb represents synthetic pointers as pieced values with a single
piece. */
gdb_assert (closure != NULL);
- gdb_assert (closure->n_pieces == 1);
+ gdb_assert (closure->pieces.size () == 1);
- return indirect_synthetic_pointer (closure->pieces->v.ptr.die_sect_off,
- closure->pieces->v.ptr.offset,
- closure->per_cu, frame, type);
+ return indirect_synthetic_pointer
+ (closure->pieces[0].v.ptr.die_sect_off,
+ closure->pieces[0].v.ptr.offset,
+ closure->per_cu, frame, type);
}
else
{
@@ -2303,14 +2299,11 @@ free_pieced_value_closure (struct value *v)
--c->refc;
if (c->refc == 0)
{
- int i;
-
- for (i = 0; i < c->n_pieces; ++i)
- if (c->pieces[i].location == DWARF_VALUE_STACK)
- value_free (c->pieces[i].v.value);
+ for (dwarf_expr_piece &p : c->pieces)
+ if (p.location == DWARF_VALUE_STACK)
+ value_free (p.v.value);
- xfree (c->pieces);
- xfree (c);
+ delete c;
}
}
@@ -2390,21 +2383,20 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
}
END_CATCH
- if (ctx.num_pieces > 0)
+ if (ctx.pieces.size () > 0)
{
struct piece_closure *c;
ULONGEST bit_size = 0;
int i;
- for (i = 0; i < ctx.num_pieces; ++i)
- bit_size += ctx.pieces[i].size;
+ for (dwarf_expr_piece &piece : ctx.pieces)
+ bit_size += piece.size;
/* Complain if the expression is larger than the size of the
outer type. */
if (bit_size > 8 * TYPE_LENGTH (type))
invalid_synthetic_pointer ();
- c = allocate_piece_closure (per_cu, ctx.num_pieces, ctx.pieces,
- frame);
+ c = allocate_piece_closure (per_cu, std::move (ctx.pieces), frame);
/* We must clean up the value chain after creating the piece
closure but before allocating the result. */
free_values.free_to_mark ();
@@ -2866,16 +2858,11 @@ dwarf2_loc_desc_get_symbol_read_needs (const gdb_byte *data, size_t size,
in_reg = ctx.location == DWARF_VALUE_REGISTER;
- if (ctx.num_pieces > 0)
- {
- int i;
-
- /* If the location has several pieces, and any of them are in
- registers, then we will need a frame to fetch them from. */
- for (i = 0; i < ctx.num_pieces; i++)
- if (ctx.pieces[i].location == DWARF_VALUE_REGISTER)
- in_reg = 1;
- }
+ /* If the location has several pieces, and any of them are in
+ registers, then we will need a frame to fetch them from. */
+ for (dwarf_expr_piece &p : ctx.pieces)
+ if (p.location == DWARF_VALUE_REGISTER)
+ in_reg = 1;
if (in_reg)
ctx.needs = SYMBOL_NEEDS_FRAME;