* [RFA 12/12] Change value::contents to be a unique_xmalloc_ptr
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 05/12] Change value history to use value_ref_ptr Tom Tromey
` (11 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This changes value::contents to be a unique_xmalloc_ptr, removing a
small bit of manual memory management.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (~value): Update.
(struct value) <contents>: Now unique_xmalloc_ptr.
(value_contents_bits_eq, allocate_value_contents)
(value_contents_raw, value_contents_all_raw)
(value_contents_for_printing, value_contents_for_printing_const)
(set_value_enclosing_type): Update.
---
gdb/ChangeLog | 9 +++++++++
gdb/value.c | 23 +++++++++++------------
2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index e39a3f0aac..acef33ccd9 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -186,8 +186,6 @@ struct value
}
else if (VALUE_LVAL (this) == lval_xcallable)
delete location.xm_worker;
-
- xfree (contents);
}
DISABLE_COPY_AND_ASSIGN (value);
@@ -332,7 +330,7 @@ struct value
/* Actual contents of the value. Target byte-order. NULL or not
valid if lazy is nonzero. */
- gdb_byte *contents = nullptr;
+ gdb::unique_xmalloc_ptr<gdb_byte> contents;
/* Unavailable ranges in CONTENTS. We mark unavailable ranges,
rather than available, since the common and default case is for a
@@ -875,8 +873,8 @@ value_contents_bits_eq (const struct value *val1, int offset1,
}
/* Compare the available/valid contents. */
- if (memcmp_with_bit_offsets (val1->contents, offset1,
- val2->contents, offset2, l) != 0)
+ if (memcmp_with_bit_offsets (val1->contents.get (), offset1,
+ val2->contents.get (), offset2, l) != 0)
return false;
length -= h;
@@ -1012,8 +1010,8 @@ allocate_value_contents (struct value *val)
if (!val->contents)
{
check_type_length_before_alloc (val->enclosing_type);
- val->contents
- = (gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type));
+ val->contents.reset
+ ((gdb_byte *) xzalloc (TYPE_LENGTH (val->enclosing_type)));
}
}
@@ -1137,14 +1135,14 @@ value_contents_raw (struct value *value)
int unit_size = gdbarch_addressable_memory_unit_size (arch);
allocate_value_contents (value);
- return value->contents + value->embedded_offset * unit_size;
+ return value->contents.get () + value->embedded_offset * unit_size;
}
gdb_byte *
value_contents_all_raw (struct value *value)
{
allocate_value_contents (value);
- return value->contents;
+ return value->contents.get ();
}
struct type *
@@ -1227,14 +1225,14 @@ value_contents_for_printing (struct value *value)
{
if (value->lazy)
value_fetch_lazy (value);
- return value->contents;
+ return value->contents.get ();
}
const gdb_byte *
value_contents_for_printing_const (const struct value *value)
{
gdb_assert (!value->lazy);
- return value->contents;
+ return value->contents.get ();
}
const gdb_byte *
@@ -2876,7 +2874,8 @@ set_value_enclosing_type (struct value *val, struct type *new_encl_type)
{
check_type_length_before_alloc (new_encl_type);
val->contents
- = (gdb_byte *) xrealloc (val->contents, TYPE_LENGTH (new_encl_type));
+ .reset ((gdb_byte *) xrealloc (val->contents.release (),
+ TYPE_LENGTH (new_encl_type)));
}
val->enclosing_type = new_encl_type;
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 05/12] Change value history to use value_ref_ptr
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
2018-04-05 21:16 ` [RFA 12/12] Change value::contents to be a unique_xmalloc_ptr Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 01/12] Introduce a gdb_ref_ptr specialization for struct value Tom Tromey
` (10 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This simplifies the value history implementation by replacing the
current data structure with a std::vector, and by making the value
history simply hold a reference to each value.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (VALUE_HISTORY_CHUNK, struct value_history_chunk)
(value_history_chain, value_history_count): Remove.
(value_history): New global.
(record_latest_value, access_value_history, show_values)
(preserve_values): Update.
---
gdb/ChangeLog | 8 ++++++++
gdb/value.c | 66 +++++++++++------------------------------------------------
2 files changed, 20 insertions(+), 54 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index 013fcfe3ed..677ec42e63 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -881,25 +881,10 @@ value_contents_eq (const struct value *val1, LONGEST offset1,
}
-/* The value-history records all the values printed
- by print commands during this session. Each chunk
- records 60 consecutive values. The first chunk on
- the chain records the most recent values.
- The total number of values is in value_history_count. */
+/* The value-history records all the values printed by print commands
+ during this session. */
-#define VALUE_HISTORY_CHUNK 60
-
-struct value_history_chunk
- {
- struct value_history_chunk *next;
- struct value *values[VALUE_HISTORY_CHUNK];
- };
-
-/* Chain of chunks now in use. */
-
-static struct value_history_chunk *value_history_chain;
-
-static int value_history_count; /* Abs number of last entry stored. */
+static std::vector<value_ref_ptr> value_history;
\f
/* List of all value objects currently allocated
@@ -1898,24 +1883,9 @@ record_latest_value (struct value *val)
but the current contents of that location. c'est la vie... */
val->modifiable = 0;
- /* Here we treat value_history_count as origin-zero
- and applying to the value being stored now. */
-
- i = value_history_count % VALUE_HISTORY_CHUNK;
- if (i == 0)
- {
- struct value_history_chunk *newobj = XCNEW (struct value_history_chunk);
+ value_history.push_back (release_value (val));
- newobj->next = value_history_chain;
- value_history_chain = newobj;
- }
-
- value_history_chain->values[i] = release_value (val).release ();
-
- /* Now we regard value_history_count as origin-one
- and applying to the value just stored. */
-
- return ++value_history_count;
+ return value_history.size ();
}
/* Return a copy of the value in the history with sequence number NUM. */
@@ -1923,12 +1893,11 @@ record_latest_value (struct value *val)
struct value *
access_value_history (int num)
{
- struct value_history_chunk *chunk;
int i;
int absnum = num;
if (absnum <= 0)
- absnum += value_history_count;
+ absnum += value_history.size ();
if (absnum <= 0)
{
@@ -1939,20 +1908,12 @@ access_value_history (int num)
else
error (_("History does not go back to $$%d."), -num);
}
- if (absnum > value_history_count)
+ if (absnum > value_history.size ())
error (_("History has not yet reached $%d."), absnum);
absnum--;
- /* Now absnum is always absolute and origin zero. */
-
- chunk = value_history_chain;
- for (i = (value_history_count - 1) / VALUE_HISTORY_CHUNK
- - absnum / VALUE_HISTORY_CHUNK;
- i > 0; i--)
- chunk = chunk->next;
-
- return value_copy (chunk->values[absnum % VALUE_HISTORY_CHUNK]);
+ return value_copy (value_history[absnum].get ());
}
static void
@@ -1972,13 +1933,13 @@ show_values (const char *num_exp, int from_tty)
else
{
/* "show values" means print the last 10 values. */
- num = value_history_count - 9;
+ num = value_history.size () - 9;
}
if (num <= 0)
num = 1;
- for (i = num; i < num + 10 && i <= value_history_count; i++)
+ for (i = num; i < num + 10 && i <= value_history.size (); i++)
{
struct value_print_options opts;
@@ -2626,7 +2587,6 @@ void
preserve_values (struct objfile *objfile)
{
htab_t copied_types;
- struct value_history_chunk *cur;
struct internalvar *var;
int i;
@@ -2634,10 +2594,8 @@ preserve_values (struct objfile *objfile)
it is soon to be deleted. */
copied_types = create_copied_types_hash (objfile);
- for (cur = value_history_chain; cur; cur = cur->next)
- for (i = 0; i < VALUE_HISTORY_CHUNK; i++)
- if (cur->values[i])
- preserve_one_value (cur->values[i], objfile, copied_types);
+ for (const value_ref_ptr &item : value_history)
+ preserve_one_value (item.get (), objfile, copied_types);
for (var = internalvars; var; var = var->next)
preserve_one_internalvar (var, objfile, copied_types);
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 01/12] Introduce a gdb_ref_ptr specialization for struct value
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
2018-04-05 21:16 ` [RFA 12/12] Change value::contents to be a unique_xmalloc_ptr Tom Tromey
2018-04-05 21:16 ` [RFA 05/12] Change value history to use value_ref_ptr Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-06 19:29 ` Pedro Alves
2018-04-05 21:16 ` [RFA 09/12] Use new and delete for values Tom Tromey
` (9 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
struct value is internally reference counted and so, while it also has
some ownership rules unique to it, it makes sense to use a gdb_ref_ptr
when managing it automatically.
This patch removes the existing unique_ptr specialization in favor of
a reference-counted pointer. It also introduces two other
clarifications:
1. Rename value_free to value_decref, which I think is more in line
with what the function actually does; and
2. Change release_value to return a gdb_ref_ptr. This change allows
us to remove the confusing release_value_or_incref function,
primarily by making it much simpler to reason about the result of
release_value.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* varobj.c (varobj_clear_saved_item)
(update_dynamic_varobj_children, install_new_value, ~varobj):
Update.
* value.h (value_incref): Move declaration earlier.
(value_decref): Rename from value_free.
(struct value_ref_policy): New.
(value_ref_ptr): New typedef.
(struct value_deleter): Remove.
(gdb_value_up): Remove typedef.
(release_value): Change return type.
(release_value_or_incref): Remove.
* value.c (set_value_parent): Update.
(value_incref): Change return type.
(value_decref): Rename from value_free.
(value_free_to_mark, free_all_values, free_value_chain): Update.
(release_value): Return value_ref_ptr.
(release_value_or_incref): Remove.
(record_latest_value, set_internalvar, clear_internalvar):
Update.
* stack.c (info_frame_command): Don't call value_free.
* python/py-value.c (valpy_dealloc, valpy_new)
(value_to_value_object): Update.
* printcmd.c (do_examine): Update.
* opencl-lang.c (lval_func_free_closure): Update.
* mi/mi-main.c (register_changed_p): Don't call value_free.
* mep-tdep.c (mep_frame_prev_register): Don't call value_free.
* m88k-tdep.c (m88k_frame_prev_register): Don't call value_free.
* m68hc11-tdep.c (m68hc11_frame_prev_register): Don't call
value_free.
* guile/scm-value.c (vlscm_free_value_smob)
(vlscm_scm_from_value): Update.
* frame.c (frame_register_unwind, frame_unwind_register_signed)
(frame_unwind_register_unsigned, get_frame_register_bytes)
(put_frame_register_bytes): Don't call value_free.
* findvar.c (address_from_register): Don't call value_free.
* dwarf2read.c (dwarf2_compute_name): Don't call value_free.
* dwarf2loc.c (entry_data_value_free_closure)
(value_of_dwarf_reg_entry, free_pieced_value_closure)
(dwarf2_evaluate_loc_desc_full): Update.
* breakpoint.c (update_watchpoint, breakpoint_init_inferior)
(~bpstats, bpstats, bpstat_clear_actions, watchpoint_check)
(~watchpoint, watch_command_1)
(invalidate_bp_value_on_memory_change): Update.
* alpha-tdep.c (alpha_register_to_value): Don't call value_free.
---
gdb/ChangeLog | 47 +++++++++++++++++++++++++++++++++++
gdb/alpha-tdep.c | 2 --
gdb/breakpoint.c | 28 +++++++++------------
gdb/dwarf2loc.c | 9 +++----
gdb/dwarf2read.c | 1 -
gdb/findvar.c | 1 -
gdb/frame.c | 6 -----
gdb/guile/scm-value.c | 5 ++--
gdb/m68hc11-tdep.c | 2 --
gdb/m88k-tdep.c | 1 -
gdb/mep-tdep.c | 3 ---
gdb/mi/mi-main.c | 2 --
gdb/opencl-lang.c | 2 +-
gdb/printcmd.c | 4 +--
gdb/python/py-value.c | 8 +++---
gdb/stack.c | 1 -
gdb/value.c | 68 +++++++++++++++++++++++----------------------------
gdb/value.h | 52 ++++++++++++++++++++++-----------------
gdb/varobj.c | 8 +++---
19 files changed, 136 insertions(+), 114 deletions(-)
diff --git a/gdb/alpha-tdep.c b/gdb/alpha-tdep.c
index 52a46d96ea..78422faa6c 100644
--- a/gdb/alpha-tdep.c
+++ b/gdb/alpha-tdep.c
@@ -252,7 +252,6 @@ alpha_register_to_value (struct frame_info *frame, int regnum,
if (*optimizedp || *unavailablep)
{
release_value (value);
- value_free (value);
return 0;
}
@@ -262,7 +261,6 @@ alpha_register_to_value (struct frame_info *frame, int regnum,
alpha_sts (gdbarch, out, value_contents_all (value));
release_value (value);
- value_free (value);
return 1;
}
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f84fef2bea..68292626d3 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1740,7 +1740,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
no longer relevant. We don't want to report a watchpoint hit
to the user when the old value and the new value may actually
be completely different objects. */
- value_free (b->val);
+ value_decref (b->val);
b->val = NULL;
b->val_valid = 0;
@@ -1795,7 +1795,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
{
v = extract_bitfield_from_watchpoint_value (b, v);
if (v != NULL)
- release_value (v);
+ release_value (v).release ();
}
b->val = v;
b->val_valid = 1;
@@ -1971,7 +1971,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
{
next = value_next (v);
if (v != b->val)
- value_free (v);
+ value_decref (v);
}
/* If a software watchpoint is not watching any memory, then the
@@ -3952,7 +3952,7 @@ breakpoint_init_inferior (enum inf_context context)
/* Reset val field to force reread of starting value in
insert_breakpoints. */
if (w->val)
- value_free (w->val);
+ value_decref (w->val);
w->val = NULL;
w->val_valid = 0;
}
@@ -4200,7 +4200,7 @@ is_catchpoint (struct breakpoint *ep)
bpstats::~bpstats ()
{
if (old_val != NULL)
- value_free (old_val);
+ value_decref (old_val);
if (bp_location_at != NULL)
decref_bp_location (&bp_location_at);
}
@@ -4237,10 +4237,7 @@ bpstats::bpstats (const bpstats &other)
print_it (other.print_it)
{
if (old_val != NULL)
- {
- old_val = value_copy (old_val);
- release_value (old_val);
- }
+ old_val = release_value (value_copy (old_val)).release ();
incref_bp_location (bp_location_at);
}
@@ -4364,7 +4361,7 @@ bpstat_clear_actions (void)
if (bs->old_val != NULL)
{
- value_free (bs->old_val);
+ value_decref (bs->old_val);
bs->old_val = NULL;
}
}
@@ -4942,7 +4939,7 @@ watchpoint_check (bpstat bs)
{
if (new_val != NULL)
{
- release_value (new_val);
+ release_value (new_val).release ();
value_free_to_mark (mark);
}
bs->old_val = b->val;
@@ -10102,7 +10099,7 @@ watchpoint::~watchpoint ()
{
xfree (this->exp_string);
xfree (this->exp_string_reparse);
- value_free (this->val);
+ value_decref (this->val);
}
/* Implement the "re_set" breakpoint_ops method for watchpoints. */
@@ -10725,8 +10722,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
int ret;
exp_valid_block = NULL;
- val = value_addr (result);
- release_value (val);
+ val = release_value (value_addr (result)).release ();
value_free_to_mark (mark);
if (use_mask)
@@ -10740,7 +10736,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
}
}
else if (val != NULL)
- release_value (val);
+ release_value (val).release ();
tok = skip_spaces (arg);
end_tok = skip_to_space (tok);
@@ -14546,7 +14542,7 @@ invalidate_bp_value_on_memory_change (struct inferior *inferior,
&& loc->address + loc->length > addr
&& addr + len > loc->address)
{
- value_free (wp->val);
+ value_decref (wp->val);
wp->val = NULL;
wp->val_valid = 0;
}
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 51f133f1b5..6c84e4ad7e 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -1376,7 +1376,7 @@ entry_data_value_free_closure (struct value *v)
{
struct value *target_val = (struct value *) value_computed_closure (v);
- value_free (target_val);
+ value_decref (target_val);
}
/* Vector for methods for an entry value reference where the referenced value
@@ -1434,7 +1434,7 @@ value_of_dwarf_reg_entry (struct type *type, struct frame_info *frame,
target_type, caller_frame,
caller_per_cu);
- release_value (target_val);
+ release_value (target_val).release ();
val = allocate_computed_value (type, &entry_data_value_funcs,
target_val /* closure */);
@@ -2299,7 +2299,7 @@ free_pieced_value_closure (struct value *v)
{
for (dwarf_expr_piece &p : c->pieces)
if (p.location == DWARF_VALUE_STACK)
- value_free (p.v.value);
+ value_decref (p.v.value);
delete c;
}
@@ -2482,9 +2482,8 @@ dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
/* Preserve VALUE because we are going to free values back
to the mark, but we still need the value contents
below. */
- value_incref (value);
+ value_ref_ptr value_holder (value_incref (value));
free_values.free_to_mark ();
- gdb_value_up value_holder (value);
retval = allocate_value (subobj_type);
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index fd544a7f9b..2eaec2d2a4 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -10974,7 +10974,6 @@ dwarf2_compute_name (const char *name,
opts.raw = 1;
value_print (v, &buf, &opts);
release_value (v);
- value_free (v);
}
}
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ee8f57159d..8ad5e25cb2 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -1004,7 +1004,6 @@ address_from_register (int regnum, struct frame_info *frame)
result = value_as_address (value);
release_value (value);
- value_free (value);
return result;
}
diff --git a/gdb/frame.c b/gdb/frame.c
index c792d1f4e1..90d0ac7b87 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1117,7 +1117,6 @@ frame_register_unwind (struct frame_info *frame, int regnum,
/* Dispose of the new value. This prevents watchpoints from
trying to watch the saved frame pointer. */
release_value (value);
- value_free (value);
}
void
@@ -1264,7 +1263,6 @@ frame_unwind_register_signed (struct frame_info *frame, int regnum)
byte_order);
release_value (value);
- value_free (value);
return r;
}
@@ -1299,7 +1297,6 @@ frame_unwind_register_unsigned (struct frame_info *frame, int regnum)
byte_order);
release_value (value);
- value_free (value);
return r;
}
@@ -1446,12 +1443,10 @@ get_frame_register_bytes (struct frame_info *frame, int regnum,
if (*optimizedp || *unavailablep)
{
release_value (value);
- value_free (value);
return 0;
}
memcpy (myaddr, value_contents_all (value) + offset, curr_len);
release_value (value);
- value_free (value);
}
myaddr += curr_len;
@@ -1500,7 +1495,6 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
curr_len);
put_frame_register (frame, regnum, value_contents_raw (value));
release_value (value);
- value_free (value);
}
myaddr += curr_len;
diff --git a/gdb/guile/scm-value.c b/gdb/guile/scm-value.c
index 5816259330..5a28d4d0ba 100644
--- a/gdb/guile/scm-value.c
+++ b/gdb/guile/scm-value.c
@@ -131,7 +131,7 @@ vlscm_free_value_smob (SCM self)
value_smob *v_smob = (value_smob *) SCM_SMOB_DATA (self);
vlscm_forget_value_smob (v_smob);
- value_free (v_smob->value);
+ value_decref (v_smob->value);
return 0;
}
@@ -253,8 +253,7 @@ vlscm_scm_from_value (struct value *value)
SCM v_scm = vlscm_make_value_smob ();
value_smob *v_smob = (value_smob *) SCM_SMOB_DATA (v_scm);
- v_smob->value = value;
- release_value_or_incref (value);
+ v_smob->value = release_value (value).release ();
vlscm_remember_scheme_value (v_smob);
return v_scm;
diff --git a/gdb/m68hc11-tdep.c b/gdb/m68hc11-tdep.c
index 58ef4a3292..d6f3593f6e 100644
--- a/gdb/m68hc11-tdep.c
+++ b/gdb/m68hc11-tdep.c
@@ -919,13 +919,11 @@ m68hc11_frame_prev_register (struct frame_info *this_frame,
CORE_ADDR page;
release_value (value);
- value_free (value);
value = trad_frame_get_prev_register (this_frame, info->saved_regs,
HARD_PAGE_REGNUM);
page = value_as_long (value);
release_value (value);
- value_free (value);
pc -= 0x08000;
pc += ((page & 0x0ff) << 14);
diff --git a/gdb/m88k-tdep.c b/gdb/m88k-tdep.c
index 6a50126548..dd84350c47 100644
--- a/gdb/m88k-tdep.c
+++ b/gdb/m88k-tdep.c
@@ -726,7 +726,6 @@ m88k_frame_prev_register (struct frame_info *this_frame,
M88K_SXIP_REGNUM);
pc = value_as_long (value);
release_value (value);
- value_free (value);
if (regnum == M88K_SFIP_REGNUM)
pc += 4;
diff --git a/gdb/mep-tdep.c b/gdb/mep-tdep.c
index 1cda2b35af..0e8c3f97e2 100644
--- a/gdb/mep-tdep.c
+++ b/gdb/mep-tdep.c
@@ -2005,7 +2005,6 @@ mep_frame_prev_register (struct frame_info *this_frame,
MEP_LP_REGNUM);
lp = value_as_long (value);
release_value (value);
- value_free (value);
return frame_unwind_got_constant (this_frame, regnum, lp & ~1);
}
@@ -2036,13 +2035,11 @@ mep_frame_prev_register (struct frame_info *this_frame,
psw = value_as_long (value);
release_value (value);
- value_free (value);
/* Get the LP's value, too. */
value = get_frame_register_value (this_frame, MEP_LP_REGNUM);
lp = value_as_long (value);
release_value (value);
- value_free (value);
/* If LP.LTOM is set, then toggle PSW.OM. */
if (lp & 0x1)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 9c4e44ba6b..deb96b4036 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1017,8 +1017,6 @@ register_changed_p (int regnum, readonly_detached_regcache *prev_regs,
release_value (prev_value);
release_value (this_value);
- value_free (prev_value);
- value_free (this_value);
return ret;
}
diff --git a/gdb/opencl-lang.c b/gdb/opencl-lang.c
index 9c5b90cc55..8af63f7620 100644
--- a/gdb/opencl-lang.c
+++ b/gdb/opencl-lang.c
@@ -292,7 +292,7 @@ lval_func_free_closure (struct value *v)
if (c->refc == 0)
{
- value_free (c->val); /* Decrement the reference counter of the value. */
+ value_decref (c->val); /* Decrement the reference counter of the value. */
xfree (c->indices);
xfree (c);
}
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 58bdac6192..8822ae12e9 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1094,7 +1094,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
last_examine_address = next_address;
if (last_examine_value)
- value_free (last_examine_value);
+ value_decref (last_examine_value);
/* The value to be displayed is not fetched greedily.
Instead, to avoid the possibility of a fetched value not
@@ -1108,7 +1108,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
last_examine_value = value_at_lazy (val_type, next_address);
if (last_examine_value)
- release_value (last_examine_value);
+ release_value (last_examine_value).release ();
print_formatted (last_examine_value, size, &opts, gdb_stdout);
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 1673fa41b0..bba6d0b8a4 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -88,7 +88,7 @@ valpy_dealloc (PyObject *obj)
if (self->next)
self->next->prev = self->prev;
- value_free (self->value);
+ value_decref (self->value);
if (self->address)
/* Use braces to appease gcc warning. *sigh* */
@@ -147,8 +147,7 @@ valpy_new (PyTypeObject *subtype, PyObject *args, PyObject *keywords)
return NULL;
}
- value_obj->value = value;
- release_value_or_incref (value);
+ value_obj->value = release_value (value).release ();
value_obj->address = NULL;
value_obj->type = NULL;
value_obj->dynamic_type = NULL;
@@ -1583,8 +1582,7 @@ value_to_value_object (struct value *val)
val_obj = PyObject_New (value_object, &value_object_type);
if (val_obj != NULL)
{
- val_obj->value = val;
- release_value_or_incref (val);
+ val_obj->value = release_value (val).release ();
val_obj->address = NULL;
val_obj->type = NULL;
val_obj->dynamic_type = NULL;
diff --git a/gdb/stack.c b/gdb/stack.c
index 9fdc9eece2..ecf1ee8379 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1645,7 +1645,6 @@ info_frame_command (const char *addr_exp, int from_tty)
}
release_value (value);
- value_free (value);
need_nl = 0;
}
/* else keep quiet. */
diff --git a/gdb/value.c b/gdb/value.c
index 110d16dcf5..002270f634 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1151,7 +1151,7 @@ set_value_parent (struct value *value, struct value *parent)
value->parent = parent;
if (parent != NULL)
value_incref (parent);
- value_free (old);
+ value_decref (old);
}
gdb_byte *
@@ -1594,10 +1594,11 @@ value_mark (void)
/* Take a reference to VAL. VAL will not be deallocated until all
references are released. */
-void
+struct value *
value_incref (struct value *val)
{
val->reference_count++;
+ return val;
}
/* Release a reference to VAL, which was acquired with value_incref.
@@ -1605,7 +1606,7 @@ value_incref (struct value *val)
chain. */
void
-value_free (struct value *val)
+value_decref (struct value *val)
{
if (val)
{
@@ -1617,7 +1618,7 @@ value_free (struct value *val)
/* If there's an associated parent value, drop our reference to
it. */
if (val->parent != NULL)
- value_free (val->parent);
+ value_decref (val->parent);
if (VALUE_LVAL (val) == lval_computed)
{
@@ -1647,7 +1648,7 @@ value_free_to_mark (const struct value *mark)
{
next = val->next;
val->released = 1;
- value_free (val);
+ value_decref (val);
}
all_values = val;
}
@@ -1666,7 +1667,7 @@ free_all_values (void)
{
next = val->next;
val->released = 1;
- value_free (val);
+ value_decref (val);
}
all_values = 0;
@@ -1682,50 +1683,48 @@ free_value_chain (struct value *v)
for (; v; v = next)
{
next = value_next (v);
- value_free (v);
+ value_decref (v);
}
}
/* Remove VAL from the chain all_values
so it will not be freed automatically. */
-void
+value_ref_ptr
release_value (struct value *val)
{
struct value *v;
+ bool released = false;
if (all_values == val)
{
all_values = val->next;
val->next = NULL;
- val->released = 1;
- return;
+ released = true;
}
-
- for (v = all_values; v; v = v->next)
+ else
{
- if (v->next == val)
+ for (v = all_values; v; v = v->next)
{
- v->next = val->next;
- val->next = NULL;
- val->released = 1;
- break;
+ if (v->next == val)
+ {
+ v->next = val->next;
+ val->next = NULL;
+ released = true;
+ break;
+ }
}
}
-}
-/* If the value is not already released, release it.
- If the value is already released, increment its reference count.
- That is, this function ensures that the value is released from the
- value chain and that the caller owns a reference to it. */
+ if (!released)
+ {
+ /* We must always return an owned reference. Normally this
+ happens because we transfer the reference from the value
+ chain, but in this case the value was not on the chain. */
+ value_incref (val);
+ }
-void
-release_value_or_incref (struct value *val)
-{
- if (val->released)
- value_incref (val);
- else
- release_value (val);
+ return value_ref_ptr (val);
}
/* Release all values up to mark */
@@ -1896,11 +1895,6 @@ record_latest_value (struct value *val)
but the current contents of that location. c'est la vie... */
val->modifiable = 0;
- /* The value may have already been released, in which case we're adding a
- new reference for its entry in the history. That is why we call
- release_value_or_incref here instead of release_value. */
- release_value_or_incref (val);
-
/* Here we treat value_history_count as origin-zero
and applying to the value being stored now. */
@@ -1913,7 +1907,7 @@ record_latest_value (struct value *val)
value_history_chain = newobj;
}
- value_history_chain->values[i] = val;
+ value_history_chain->values[i] = release_value (val).release ();
/* Now we regard value_history_count as origin-one
and applying to the value just stored. */
@@ -2416,7 +2410,7 @@ set_internalvar (struct internalvar *var, struct value *val)
deleted by free_all_values. From here on this function should not
call error () until new_data is installed into the var->u to avoid
leaking memory. */
- release_value (new_data.value);
+ release_value (new_data.value).release ();
/* Internal variables which are created from values with a dynamic
location don't need the location property of the origin anymore.
@@ -2478,7 +2472,7 @@ clear_internalvar (struct internalvar *var)
switch (var->kind)
{
case INTERNALVAR_VALUE:
- value_free (var->u.value);
+ value_decref (var->u.value);
break;
case INTERNALVAR_STRING:
diff --git a/gdb/value.h b/gdb/value.h
index 0bc51304ea..f7e7387ff1 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -22,6 +22,7 @@
#include "frame.h" /* For struct frame_id. */
#include "extension.h"
+#include "common/gdb_ref_ptr.h"
struct block;
struct expression;
@@ -87,6 +88,34 @@ struct value_print_options;
struct value;
+/* Decrease VAL's reference count. When the reference count drops to
+ 0, VAL will be freed. */
+
+extern struct value *value_incref (struct value *val);
+
+/* Increate VAL's reference count. VAL is returned. */
+
+extern void value_decref (struct value *val);
+
+/* A policy class to interface gdb::ref_ptr with struct value. */
+
+struct value_ref_policy
+{
+ static void incref (struct value *ptr)
+ {
+ value_incref (ptr);
+ }
+
+ static void decref (struct value *ptr)
+ {
+ value_decref (ptr);
+ }
+};
+
+/* A gdb:;ref_ptr pointer to a struct value. */
+
+typedef gdb::ref_ptr<struct value, value_ref_policy> value_ref_ptr;
+
/* Values are stored in a chain, so that they can be deleted easily
over calls to the inferior. Values assigned to internal variables,
put into the value history or exposed to Python are taken off this
@@ -1024,32 +1053,11 @@ extern int unop_user_defined_p (enum exp_opcode op, struct value *arg1);
extern int destructor_name_p (const char *name, struct type *type);
-extern void value_incref (struct value *val);
-
-extern void value_free (struct value *val);
-
-/* A free policy class to interface std::unique_ptr with
- value_free. */
-
-struct value_deleter
-{
- void operator() (struct value *value) const
- {
- value_free (value);
- }
-};
-
-/* A unique pointer to a struct value. */
-
-typedef std::unique_ptr<struct value, value_deleter> gdb_value_up;
-
extern void free_all_values (void);
extern void free_value_chain (struct value *v);
-extern void release_value (struct value *val);
-
-extern void release_value_or_incref (struct value *val);
+extern value_ref_ptr release_value (struct value *val);
extern int record_latest_value (struct value *val);
diff --git a/gdb/varobj.c b/gdb/varobj.c
index f23243f3b7..58eb531927 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -707,7 +707,7 @@ varobj_clear_saved_item (struct varobj_dynamic *var)
{
if (var->saved_item != NULL)
{
- value_free (var->saved_item->value);
+ value_decref (var->saved_item->value);
delete var->saved_item;
var->saved_item = NULL;
}
@@ -761,7 +761,7 @@ update_dynamic_varobj_children (struct varobj *var,
/* Release vitem->value so its lifetime is not bound to the
execution of a command. */
if (item != NULL && item->value != NULL)
- release_value_or_incref (item->value);
+ release_value (item->value).release ();
}
if (item == NULL)
@@ -1393,7 +1393,7 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
/* We must always keep the new value, since children depend on it. */
if (var->value != NULL && var->value != value)
- value_free (var->value);
+ value_decref (var->value);
var->value = value;
if (value && value_lazy (value) && intentionally_not_fetched)
var->not_fetched = true;
@@ -1990,7 +1990,7 @@ varobj::~varobj ()
varobj_iter_delete (var->dynamic->child_iter);
varobj_clear_saved_item (var->dynamic);
- value_free (var->value);
+ value_decref (var->value);
if (is_root_p (var))
delete var->root;
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 01/12] Introduce a gdb_ref_ptr specialization for struct value
2018-04-05 21:16 ` [RFA 01/12] Introduce a gdb_ref_ptr specialization for struct value Tom Tromey
@ 2018-04-06 19:29 ` Pedro Alves
0 siblings, 0 replies; 22+ messages in thread
From: Pedro Alves @ 2018-04-06 19:29 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
On 04/05/2018 10:14 PM, Tom Tromey wrote:
> struct value is internally reference counted and so, while it also has
> some ownership rules unique to it, it makes sense to use a gdb_ref_ptr
> when managing it automatically.
>
> This patch removes the existing unique_ptr specialization in favor of
> a reference-counted pointer. It also introduces two other
> clarifications:
>
> 1. Rename value_free to value_decref, which I think is more in line
> with what the function actually does; and
>
> 2. Change release_value to return a gdb_ref_ptr. This change allows
> us to remove the confusing release_value_or_incref function,
> primarily by making it much simpler to reason about the result of
> release_value.
Yeah. As I was reading this patch, I was wondering whether
release_value is going to score high in could-use-a-better-name
charts. I.e., wondering whether code like this:
release_value (v).release ();
is likely to cause confusion.
Maybe renaming it to be a bit more explicit would help.
E.g.:
release_from_value_chain (v).release ();
or:
move_out_of_value_chain (v).release ();
But, the following patches eliminate the ".release()" calls, so
it isn't that bad. Anyway, that was a thought for another rainy
day, not for this patch.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFA 09/12] Use new and delete for values
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (2 preceding siblings ...)
2018-04-05 21:16 ` [RFA 01/12] Introduce a gdb_ref_ptr specialization for struct value Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 03/12] Change last_examine_value to value_ref_ptr Tom Tromey
` (8 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This adds a constructor and destructor to struct value, and then
changes value.c to use "new" and "delete".
While doing this I noticed a memory leak -- value_decref was not
freeing value::optimized_out. This patch fixes this leak.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (struct value): Add constructor, destructor, and member
initializers.
(allocate_value_lazy, value_decref): Update.
---
gdb/ChangeLog | 6 ++++
gdb/value.c | 98 ++++++++++++++++++++++++++++++-----------------------------
2 files changed, 56 insertions(+), 48 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index 3137bf48f8..30af33c4d2 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -168,9 +168,44 @@ static struct cmd_list_element *functionlist;
struct value
{
+ explicit value (struct type *type_)
+ : modifiable (1),
+ lazy (1),
+ initialized (1),
+ stack (0),
+ type (type_),
+ enclosing_type (type_)
+ {
+ location.address = 0;
+ }
+
+ ~value ()
+ {
+ /* If there's an associated parent value, drop our reference to
+ it. */
+ if (parent != NULL)
+ value_decref (parent);
+
+ if (VALUE_LVAL (this) == lval_computed)
+ {
+ const struct lval_funcs *funcs = location.computed.funcs;
+
+ if (funcs->free_closure)
+ funcs->free_closure (this);
+ }
+ else if (VALUE_LVAL (this) == lval_xcallable)
+ delete location.xm_worker;
+
+ xfree (contents);
+ VEC_free (range_s, unavailable);
+ VEC_free (range_s, optimized_out);
+ }
+
+ DISABLE_COPY_AND_ASSIGN (value);
+
/* Type of value; either not an lval, or one of the various
different possible kinds of lval. */
- enum lval_type lval;
+ enum lval_type lval = not_lval;
/* Is it modifiable? Only relevant if lval != not_lval. */
unsigned int modifiable : 1;
@@ -237,27 +272,27 @@ struct value
/* Describes offset of a value within lval of a structure in target
addressable memory units. Note also the member embedded_offset
below. */
- LONGEST offset;
+ LONGEST offset = 0;
/* Only used for bitfields; number of bits contained in them. */
- LONGEST bitsize;
+ LONGEST bitsize = 0;
/* Only used for bitfields; position of start of field. For
gdbarch_bits_big_endian=0 targets, it is the position of the LSB. For
gdbarch_bits_big_endian=1 targets, it is the position of the MSB. */
- LONGEST bitpos;
+ LONGEST bitpos = 0;
/* The number of references to this value. When a value is created,
the value chain holds a reference, so REFERENCE_COUNT is 1. If
release_value is called, this value is removed from the chain but
the caller of release_value now has a reference to this value.
The caller must arrange for a call to value_free later. */
- int reference_count;
+ int reference_count = 1;
/* Only used for bitfields; the containing value. This allows a
single read from the target when displaying multiple
bitfields. */
- struct value *parent;
+ struct value *parent = nullptr;
/* Type of the value. */
struct type *type;
@@ -303,12 +338,12 @@ struct value
`type', and `embedded_offset' is zero, so everything works
normally. */
struct type *enclosing_type;
- LONGEST embedded_offset;
- LONGEST pointed_to_offset;
+ LONGEST embedded_offset = 0;
+ LONGEST pointed_to_offset = 0;
/* Actual contents of the value. Target byte-order. NULL or not
valid if lazy is nonzero. */
- gdb_byte *contents;
+ gdb_byte *contents = nullptr;
/* Unavailable ranges in CONTENTS. We mark unavailable ranges,
rather than available, since the common and default case is for a
@@ -316,7 +351,7 @@ struct value
The unavailable ranges are tracked in bits. Note that a contents
bit that has been optimized out doesn't really exist in the
program, so it can't be marked unavailable either. */
- VEC(range_s) *unavailable;
+ VEC(range_s) *unavailable = nullptr;
/* Likewise, but for optimized out contents (a chunk of the value of
a variable that does not actually exist in the program). If LVAL
@@ -325,7 +360,7 @@ struct value
saved registers and optimized-out program variables values are
treated pretty much the same, except not-saved registers have a
different string representation and related error strings. */
- VEC(range_s) *optimized_out;
+ VEC(range_s) *optimized_out = nullptr;
};
/* See value.h. */
@@ -901,23 +936,9 @@ allocate_value_lazy (struct type *type)
description correctly. */
check_typedef (type);
- val = XCNEW (struct value);
- val->contents = NULL;
- val->type = type;
- val->enclosing_type = type;
- VALUE_LVAL (val) = not_lval;
- val->location.address = 0;
- val->offset = 0;
- val->bitpos = 0;
- val->bitsize = 0;
- val->lazy = 1;
- val->embedded_offset = 0;
- val->pointed_to_offset = 0;
- val->modifiable = 1;
- val->initialized = 1; /* Default to initialized. */
+ val = new struct value (type);
/* Values start out on the all_values chain. */
- val->reference_count = 1;
all_values.emplace_back (val);
return val;
@@ -1579,32 +1600,13 @@ value_incref (struct value *val)
void
value_decref (struct value *val)
{
- if (val)
+ if (val != nullptr)
{
gdb_assert (val->reference_count > 0);
val->reference_count--;
- if (val->reference_count > 0)
- return;
-
- /* If there's an associated parent value, drop our reference to
- it. */
- if (val->parent != NULL)
- value_decref (val->parent);
-
- if (VALUE_LVAL (val) == lval_computed)
- {
- const struct lval_funcs *funcs = val->location.computed.funcs;
-
- if (funcs->free_closure)
- funcs->free_closure (val);
- }
- else if (VALUE_LVAL (val) == lval_xcallable)
- delete val->location.xm_worker;
-
- xfree (val->contents);
- VEC_free (range_s, val->unavailable);
+ if (val->reference_count == 0)
+ delete val;
}
- xfree (val);
}
/* Free all values allocated since MARK was obtained by value_mark
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 03/12] Change last_examine_value to value_ref_ptr
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (3 preceding siblings ...)
2018-04-05 21:16 ` [RFA 09/12] Use new and delete for values Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 11/12] Remove range_s VEC Tom Tromey
` (7 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This patch removes some manual reference count manipulation by
changing last_examine_value to be a value_ref_ptr and then updating
the users.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* printcmd.c (last_examine_address): Change type to
value_ref_ptr.
(do_examine, x_command): Update.
---
gdb/ChangeLog | 6 ++++++
gdb/printcmd.c | 21 ++++++++-------------
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 8822ae12e9..a6d6d7e12d 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -78,7 +78,7 @@ static CORE_ADDR last_examine_address;
/* Contents of last address examined.
This is not valid past the end of the `x' command! */
-static struct value *last_examine_value;
+static value_ref_ptr last_examine_value;
/* Largest offset between a symbolic value and an address, that will be
printed as `0x1234 <symbol+offset>'. */
@@ -1093,9 +1093,6 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
object. */
last_examine_address = next_address;
- if (last_examine_value)
- value_decref (last_examine_value);
-
/* The value to be displayed is not fetched greedily.
Instead, to avoid the possibility of a fetched value not
being used, its retrieval is delayed until the print code
@@ -1105,12 +1102,10 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
the disassembler be modified so that LAST_EXAMINE_VALUE
is left with the byte sequence from the last complete
instruction fetched from memory? */
- last_examine_value = value_at_lazy (val_type, next_address);
-
- if (last_examine_value)
- release_value (last_examine_value).release ();
+ last_examine_value
+ = release_value (value_at_lazy (val_type, next_address));
- print_formatted (last_examine_value, size, &opts, gdb_stdout);
+ print_formatted (last_examine_value.get (), size, &opts, gdb_stdout);
/* Display any branch delay slots following the final insn. */
if (format == 'i' && count == 1)
@@ -1668,12 +1663,12 @@ x_command (const char *exp, int from_tty)
last_format = fmt.format;
/* Set a couple of internal variables if appropriate. */
- if (last_examine_value)
+ if (last_examine_value != nullptr)
{
/* Make last address examined available to the user as $_. Use
the correct pointer type. */
struct type *pointer_type
- = lookup_pointer_type (value_type (last_examine_value));
+ = lookup_pointer_type (value_type (last_examine_value.get ()));
set_internalvar (lookup_internalvar ("_"),
value_from_pointer (pointer_type,
last_examine_address));
@@ -1682,10 +1677,10 @@ x_command (const char *exp, int from_tty)
as $__. If the last value has not been fetched from memory
then don't fetch it now; instead mark it by voiding the $__
variable. */
- if (value_lazy (last_examine_value))
+ if (value_lazy (last_examine_value.get ()))
clear_internalvar (lookup_internalvar ("__"));
else
- set_internalvar (lookup_internalvar ("__"), last_examine_value);
+ set_internalvar (lookup_internalvar ("__"), last_examine_value.get ());
}
}
\f
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 11/12] Remove range_s VEC
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (4 preceding siblings ...)
2018-04-05 21:16 ` [RFA 03/12] Change last_examine_value to value_ref_ptr Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 07/12] Remove free_value_chain Tom Tromey
` (6 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This changes the "optimized_out" and "unavailable" VECs in struct
value to be std::vectors, and then fixes up all the uses.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (range_s): Remove typedef, VEC.
(struct range): Add operator<.
(range_lessthan): Remove.
(ranges_contain): Change type.
(~value): Update.
(struct value) <unavailable, optimized_out>: Now std::vector.
(value_entirely_available)
(value_entirely_covered_by_range_vector)
(value_entirely_unavailable, value_entirely_optimized_out):
Update.
(insert_into_bit_range_vector): Change argument type.
(find_first_range_overlap): Likewise.
(struct ranges_and_idx, value_contents_bits_eq)
(require_not_optimized_out, require_available): Update.
(ranges_copy_adjusted): Change argument types.
(value_optimized_out, value_copy, value_fetch_lazy): Update.
---
gdb/ChangeLog | 19 ++++++
gdb/value.c | 207 ++++++++++++++++++++++++++++------------------------------
2 files changed, 119 insertions(+), 107 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index e25934b9e3..e39a3f0aac 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -66,11 +66,17 @@ struct range
/* Length of the range. */
LONGEST length;
-};
-typedef struct range range_s;
+ /* Returns true if THIS is strictly less than OTHER, useful for
+ searching. We keep ranges sorted by offset and coalesce
+ overlapping and contiguous ranges, so this just compares the
+ starting offset. */
-DEF_VEC_O(range_s);
+ bool operator< (const range &other) const
+ {
+ return offset < other.offset;
+ }
+};
/* Returns true if the ranges defined by [offset1, offset1+len1) and
[offset2, offset2+len2) overlap. */
@@ -86,25 +92,14 @@ ranges_overlap (LONGEST offset1, LONGEST len1,
return (l < h);
}
-/* Returns true if the first argument is strictly less than the
- second, useful for VEC_lower_bound. We keep ranges sorted by
- offset and coalesce overlapping and contiguous ranges, so this just
- compares the starting offset. */
-
-static int
-range_lessthan (const range_s *r1, const range_s *r2)
-{
- return r1->offset < r2->offset;
-}
-
/* Returns true if RANGES contains any range that overlaps [OFFSET,
OFFSET+LENGTH). */
static int
-ranges_contain (VEC(range_s) *ranges, LONGEST offset, LONGEST length)
+ranges_contain (const std::vector<range> &ranges, LONGEST offset,
+ LONGEST length)
{
- range_s what;
- LONGEST i;
+ range what;
what.offset = offset;
what.length = length;
@@ -140,21 +135,22 @@ ranges_contain (VEC(range_s) *ranges, LONGEST offset, LONGEST length)
I=1
*/
- i = VEC_lower_bound (range_s, ranges, &what, range_lessthan);
- if (i > 0)
+ auto i = std::lower_bound (ranges.begin (), ranges.end (), what);
+
+ if (i > ranges.begin ())
{
- struct range *bef = VEC_index (range_s, ranges, i - 1);
+ const struct range &bef = *(i - 1);
- if (ranges_overlap (bef->offset, bef->length, offset, length))
+ if (ranges_overlap (bef.offset, bef.length, offset, length))
return 1;
}
- if (i < VEC_length (range_s, ranges))
+ if (i < ranges.end ())
{
- struct range *r = VEC_index (range_s, ranges, i);
+ const struct range &r = *i;
- if (ranges_overlap (r->offset, r->length, offset, length))
+ if (ranges_overlap (r.offset, r.length, offset, length))
return 1;
}
@@ -192,8 +188,6 @@ struct value
delete location.xm_worker;
xfree (contents);
- VEC_free (range_s, unavailable);
- VEC_free (range_s, optimized_out);
}
DISABLE_COPY_AND_ASSIGN (value);
@@ -346,7 +340,7 @@ struct value
The unavailable ranges are tracked in bits. Note that a contents
bit that has been optimized out doesn't really exist in the
program, so it can't be marked unavailable either. */
- VEC(range_s) *unavailable = nullptr;
+ std::vector<range> unavailable;
/* Likewise, but for optimized out contents (a chunk of the value of
a variable that does not actually exist in the program). If LVAL
@@ -355,7 +349,7 @@ struct value
saved registers and optimized-out program variables values are
treated pretty much the same, except not-saved registers have a
different string representation and related error strings. */
- VEC(range_s) *optimized_out = nullptr;
+ std::vector<range> optimized_out;
};
/* See value.h. */
@@ -399,7 +393,7 @@ value_entirely_available (struct value *value)
if (value->lazy)
value_fetch_lazy (value);
- if (VEC_empty (range_s, value->unavailable))
+ if (value->unavailable.empty ())
return 1;
return 0;
}
@@ -410,20 +404,20 @@ value_entirely_available (struct value *value)
static int
value_entirely_covered_by_range_vector (struct value *value,
- VEC(range_s) **ranges)
+ const std::vector<range> &ranges)
{
/* We can only tell whether the whole value is optimized out /
unavailable when we try to read it. */
if (value->lazy)
value_fetch_lazy (value);
- if (VEC_length (range_s, *ranges) == 1)
+ if (ranges.size () == 1)
{
- struct range *t = VEC_index (range_s, *ranges, 0);
+ const struct range &t = ranges[0];
- if (t->offset == 0
- && t->length == (TARGET_CHAR_BIT
- * TYPE_LENGTH (value_enclosing_type (value))))
+ if (t.offset == 0
+ && t.length == (TARGET_CHAR_BIT
+ * TYPE_LENGTH (value_enclosing_type (value))))
return 1;
}
@@ -433,24 +427,23 @@ value_entirely_covered_by_range_vector (struct value *value,
int
value_entirely_unavailable (struct value *value)
{
- return value_entirely_covered_by_range_vector (value, &value->unavailable);
+ return value_entirely_covered_by_range_vector (value, value->unavailable);
}
int
value_entirely_optimized_out (struct value *value)
{
- return value_entirely_covered_by_range_vector (value, &value->optimized_out);
+ return value_entirely_covered_by_range_vector (value, value->optimized_out);
}
/* Insert into the vector pointed to by VECTORP the bit range starting of
OFFSET bits, and extending for the next LENGTH bits. */
static void
-insert_into_bit_range_vector (VEC(range_s) **vectorp,
+insert_into_bit_range_vector (std::vector<range> *vectorp,
LONGEST offset, LONGEST length)
{
- range_s newr;
- int i;
+ range newr;
/* Insert the range sorted. If there's overlap or the new range
would be contiguous with an existing range, merge. */
@@ -538,76 +531,77 @@ insert_into_bit_range_vector (VEC(range_s) **vectorp,
*/
- i = VEC_lower_bound (range_s, *vectorp, &newr, range_lessthan);
- if (i > 0)
+ auto i = std::lower_bound (vectorp->begin (), vectorp->end (), newr);
+ if (i > vectorp->begin ())
{
- struct range *bef = VEC_index (range_s, *vectorp, i - 1);
+ struct range &bef = *(i - 1);
- if (ranges_overlap (bef->offset, bef->length, offset, length))
+ if (ranges_overlap (bef.offset, bef.length, offset, length))
{
/* #1 */
- ULONGEST l = std::min (bef->offset, offset);
- ULONGEST h = std::max (bef->offset + bef->length, offset + length);
+ ULONGEST l = std::min (bef.offset, offset);
+ ULONGEST h = std::max (bef.offset + bef.length, offset + length);
- bef->offset = l;
- bef->length = h - l;
+ bef.offset = l;
+ bef.length = h - l;
i--;
}
- else if (offset == bef->offset + bef->length)
+ else if (offset == bef.offset + bef.length)
{
/* #2 */
- bef->length += length;
+ bef.length += length;
i--;
}
else
{
/* #3 */
- VEC_safe_insert (range_s, *vectorp, i, &newr);
+ i = vectorp->insert (i, newr);
}
}
else
{
/* #4 */
- VEC_safe_insert (range_s, *vectorp, i, &newr);
+ i = vectorp->insert (i, newr);
}
/* Check whether the ranges following the one we've just added or
touched can be folded in (#5 above). */
- if (i + 1 < VEC_length (range_s, *vectorp))
+ if (i != vectorp->end () && i + 1 < vectorp->end ())
{
- struct range *t;
- struct range *r;
int removed = 0;
- int next = i + 1;
+ auto next = i + 1;
/* Get the range we just touched. */
- t = VEC_index (range_s, *vectorp, i);
+ struct range &t = *i;
removed = 0;
i = next;
- for (; VEC_iterate (range_s, *vectorp, i, r); i++)
- if (r->offset <= t->offset + t->length)
- {
- ULONGEST l, h;
-
- l = std::min (t->offset, r->offset);
- h = std::max (t->offset + t->length, r->offset + r->length);
-
- t->offset = l;
- t->length = h - l;
-
- removed++;
- }
- else
- {
- /* If we couldn't merge this one, we won't be able to
- merge following ones either, since the ranges are
- always sorted by OFFSET. */
- break;
- }
+ for (; i < vectorp->end (); i++)
+ {
+ struct range &r = *i;
+ if (r.offset <= t.offset + t.length)
+ {
+ ULONGEST l, h;
+
+ l = std::min (t.offset, r.offset);
+ h = std::max (t.offset + t.length, r.offset + r.length);
+
+ t.offset = l;
+ t.length = h - l;
+
+ removed++;
+ }
+ else
+ {
+ /* If we couldn't merge this one, we won't be able to
+ merge following ones either, since the ranges are
+ always sorted by OFFSET. */
+ break;
+ }
+ }
if (removed != 0)
- VEC_block_remove (range_s, *vectorp, next, removed);
+ vectorp->erase (next, next + removed);
}
}
@@ -633,15 +627,17 @@ mark_value_bytes_unavailable (struct value *value,
found, or -1 if none was found. */
static int
-find_first_range_overlap (VEC(range_s) *ranges, int pos,
+find_first_range_overlap (const std::vector<range> *ranges, int pos,
LONGEST offset, LONGEST length)
{
- range_s *r;
int i;
- for (i = pos; VEC_iterate (range_s, ranges, i, r); i++)
- if (ranges_overlap (r->offset, r->length, offset, length))
- return i;
+ for (i = pos; i < ranges->size (); i++)
+ {
+ const range &r = (*ranges)[i];
+ if (ranges_overlap (r.offset, r.length, offset, length))
+ return i;
+ }
return -1;
}
@@ -754,7 +750,7 @@ memcmp_with_bit_offsets (const gdb_byte *ptr1, size_t offset1_bits,
struct ranges_and_idx
{
/* The ranges. */
- VEC(range_s) *ranges;
+ const std::vector<range> *ranges;
/* The range we've last found in RANGES. Given ranges are sorted,
we can start the next lookup here. */
@@ -788,12 +784,12 @@ find_first_range_overlap_and_match (struct ranges_and_idx *rp1,
return 0;
else
{
- range_s *r1, *r2;
+ const range *r1, *r2;
ULONGEST l1, h1;
ULONGEST l2, h2;
- r1 = VEC_index (range_s, rp1->ranges, rp1->idx);
- r2 = VEC_index (range_s, rp2->ranges, rp2->idx);
+ r1 = &(*rp1->ranges)[rp1->idx];
+ r2 = &(*rp2->ranges)[rp2->idx];
/* Get the unavailable windows intersected by the incoming
ranges. The first and last ranges that overlap the argument
@@ -849,10 +845,10 @@ value_contents_bits_eq (const struct value *val1, int offset1,
memset (&rp1, 0, sizeof (rp1));
memset (&rp2, 0, sizeof (rp2));
- rp1[0].ranges = val1->unavailable;
- rp2[0].ranges = val2->unavailable;
- rp1[1].ranges = val1->optimized_out;
- rp2[1].ranges = val2->optimized_out;
+ rp1[0].ranges = &val1->unavailable;
+ rp2[0].ranges = &val2->unavailable;
+ rp1[1].ranges = &val1->optimized_out;
+ rp2[1].ranges = &val2->optimized_out;
while (length > 0)
{
@@ -1210,7 +1206,7 @@ error_value_optimized_out (void)
static void
require_not_optimized_out (const struct value *value)
{
- if (!VEC_empty (range_s, value->optimized_out))
+ if (!value->optimized_out.empty ())
{
if (value->lval == lval_register)
error (_("register has not been saved in frame"));
@@ -1222,7 +1218,7 @@ require_not_optimized_out (const struct value *value)
static void
require_available (const struct value *value)
{
- if (!VEC_empty (range_s, value->unavailable))
+ if (!value->unavailable.empty ())
throw_error (NOT_AVAILABLE_ERROR, _("value is not available"));
}
@@ -1254,19 +1250,16 @@ value_contents_all (struct value *value)
SRC_BIT_OFFSET+BIT_LENGTH) ranges into *DST_RANGE, adjusted. */
static void
-ranges_copy_adjusted (VEC (range_s) **dst_range, int dst_bit_offset,
- VEC (range_s) *src_range, int src_bit_offset,
+ranges_copy_adjusted (std::vector<range> *dst_range, int dst_bit_offset,
+ const std::vector<range> &src_range, int src_bit_offset,
int bit_length)
{
- range_s *r;
- int i;
-
- for (i = 0; VEC_iterate (range_s, src_range, i, r); i++)
+ for (const range &r : src_range)
{
ULONGEST h, l;
- l = std::max (r->offset, (LONGEST) src_bit_offset);
- h = std::min (r->offset + r->length,
+ l = std::max (r.offset, (LONGEST) src_bit_offset);
+ h = std::min (r.offset + r.length,
(LONGEST) src_bit_offset + bit_length);
if (l < h)
@@ -1405,7 +1398,7 @@ value_optimized_out (struct value *value)
{
/* We can only know if a value is optimized out once we have tried to
fetch it. */
- if (VEC_empty (range_s, value->optimized_out) && value->lazy)
+ if (value->optimized_out.empty () && value->lazy)
{
TRY
{
@@ -1418,7 +1411,7 @@ value_optimized_out (struct value *value)
END_CATCH
}
- return !VEC_empty (range_s, value->optimized_out);
+ return !value->optimized_out.empty ();
}
/* Mark contents of VALUE as optimized out, starting at OFFSET bytes, and
@@ -1687,8 +1680,8 @@ value_copy (struct value *arg)
TYPE_LENGTH (value_enclosing_type (arg)));
}
- val->unavailable = VEC_copy (range_s, arg->unavailable);
- val->optimized_out = VEC_copy (range_s, arg->optimized_out);
+ val->unavailable = arg->unavailable;
+ val->optimized_out = arg->optimized_out;
val->parent = arg->parent;
if (VALUE_LVAL (val) == lval_computed)
{
@@ -3738,8 +3731,8 @@ value_fetch_lazy (struct value *val)
/* A value is either lazy, or fully fetched. The
availability/validity is only established as we try to fetch a
value. */
- gdb_assert (VEC_empty (range_s, val->optimized_out));
- gdb_assert (VEC_empty (range_s, val->unavailable));
+ gdb_assert (val->optimized_out.empty ());
+ gdb_assert (val->unavailable.empty ());
if (value_bitsize (val))
{
/* To read a lazy bitfield, read the entire enclosing value. This
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 07/12] Remove free_value_chain
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (5 preceding siblings ...)
2018-04-05 21:16 ` [RFA 11/12] Remove range_s VEC Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 02/12] Change breakpoints to use value_ref_ptr Tom Tromey
` (5 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This patch changes value_release_to_mark and fetch_subexp_value to
return a std::vector of value references, rather than relying on the
"next" field that is contained in a struct value. This makes it
simpler to reason about the returned values, and also allows for the
removal of free_value_chain.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.h (fetch_subexp_value, value_release_to_mark): Update.
(free_value_chain): Remove.
* value.c (free_value_chain): Remove.
(value_release_to_mark): Return a std::vector.
* ppc-linux-nat.c (num_memory_accesses): Change "chain" to a
std::vector.
(check_condition): Update.
* eval.c (fetch_subexp_value): Change "val_chain" to a
std::vector.
* breakpoint.c (update_watchpoint): Update.
(can_use_hardware_watchpoint): Change "vals" to a std::vector.
---
gdb/ChangeLog | 14 ++++++++++++++
gdb/breakpoint.c | 30 ++++++++++++++++--------------
gdb/eval.c | 15 +++++++--------
gdb/ppc-linux-nat.c | 33 +++++++++------------------------
gdb/value.c | 34 ++++++++++++----------------------
gdb/value.h | 7 +++----
6 files changed, 61 insertions(+), 72 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 67ba5a6b31..c2b9f55461 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -117,7 +117,8 @@ static std::vector<symtab_and_line> decode_location_default
(struct breakpoint *b, const struct event_location *location,
struct program_space *search_pspace);
-static int can_use_hardware_watchpoint (struct value *);
+static int can_use_hardware_watchpoint
+ (const std::vector<value_ref_ptr> &vals);
static void mention (struct breakpoint *);
@@ -1777,7 +1778,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
else if (within_current_scope && b->exp)
{
int pc = 0;
- struct value *val_chain, *v, *result, *next;
+ std::vector<value_ref_ptr> val_chain;
+ struct value *v, *result, *next;
struct program_space *frame_pspace;
fetch_subexp_value (b->exp.get (), &pc, &v, &result, &val_chain, 0);
@@ -1799,15 +1801,18 @@ update_watchpoint (struct watchpoint *b, int reparse)
frame_pspace = get_frame_program_space (get_selected_frame (NULL));
/* Look at each value on the value chain. */
- for (v = val_chain; v; v = value_next (v))
+ gdb_assert (!val_chain.empty ());
+ for (const value_ref_ptr &iter : val_chain)
{
+ v = iter.get ();
+
/* If it's a memory location, and GDB actually needed
its contents to evaluate the expression, then we
must watch it. If the first value returned is
still lazy, that means an error occurred reading it;
watch it anyway in case it becomes readable. */
if (VALUE_LVAL (v) == lval_memory
- && (v == val_chain || ! value_lazy (v)))
+ && (v == val_chain[0] || ! value_lazy (v)))
{
struct type *vtype = check_typedef (value_type (v));
@@ -1962,13 +1967,6 @@ update_watchpoint (struct watchpoint *b, int reparse)
bl->loc_type = loc_type;
}
- for (v = val_chain; v; v = next)
- {
- next = value_next (v);
- if (v != b->val)
- value_decref (v);
- }
-
/* If a software watchpoint is not watching any memory, then the
above left it without any location set up. But,
bpstat_stop_status requires a location to be able to report
@@ -10875,15 +10873,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
If the watchpoint cannot be handled in hardware return zero. */
static int
-can_use_hardware_watchpoint (struct value *v)
+can_use_hardware_watchpoint (const std::vector<value_ref_ptr> &vals)
{
int found_memory_cnt = 0;
- struct value *head = v;
/* Did the user specifically forbid us to use hardware watchpoints? */
if (!can_use_hw_watchpoints)
return 0;
+ gdb_assert (!vals.empty ());
+ struct value *head = vals[0].get ();
+
/* Make sure that the value of the expression depends only upon
memory contents, and values computed from them within GDB. If we
find any register references or function calls, we can't use a
@@ -10903,8 +10903,10 @@ can_use_hardware_watchpoint (struct value *v)
function calls are special in any way. So this function may not
notice that an expression involving an inferior function call
can't be watched with hardware watchpoints. FIXME. */
- for (; v; v = value_next (v))
+ for (const value_ref_ptr &iter : vals)
{
+ struct value *v = iter.get ();
+
if (VALUE_LVAL (v) == lval_memory)
{
if (v != head && value_lazy (v))
diff --git a/gdb/eval.c b/gdb/eval.c
index 021503e9dd..b6fbfcf6c9 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -179,14 +179,14 @@ evaluate_subexpression_type (struct expression *exp, int subexp)
set to any referenced values. *VALP will never be a lazy value.
This is the value which we store in struct breakpoint.
- If VAL_CHAIN is non-NULL, *VAL_CHAIN will be released from the
- value chain. The caller must free the values individually. If
- VAL_CHAIN is NULL, all generated values will be left on the value
- chain. */
+ If VAL_CHAIN is non-NULL, the values put into *VAL_CHAIN will be
+ released from the value chain. If VAL_CHAIN is NULL, all generated
+ values will be left on the value chain. */
void
fetch_subexp_value (struct expression *exp, int *pc, struct value **valp,
- struct value **resultp, struct value **val_chain,
+ struct value **resultp,
+ std::vector<value_ref_ptr> *val_chain,
int preserve_errors)
{
struct value *mark, *new_mark, *result;
@@ -195,7 +195,7 @@ fetch_subexp_value (struct expression *exp, int *pc, struct value **valp,
if (resultp)
*resultp = NULL;
if (val_chain)
- *val_chain = NULL;
+ val_chain->clear ();
/* Evaluate the expression. */
mark = value_mark ();
@@ -253,8 +253,7 @@ fetch_subexp_value (struct expression *exp, int *pc, struct value **valp,
{
/* Return the chain of intermediate values. We use this to
decide which addresses to watch. */
- *val_chain = new_mark;
- value_release_to_mark (mark);
+ *val_chain = value_release_to_mark (mark);
}
}
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 1d2769a983..2cd8792d30 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1840,10 +1840,9 @@ calculate_dvc (CORE_ADDR addr, int len, CORE_ADDR data_value,
other kinds of values which are not acceptable in a condition
expression (e.g., lval_computed or lval_internalvar). */
static int
-num_memory_accesses (struct value *v)
+num_memory_accesses (const std::vector<value_ref_ptr> &chain)
{
int found_memory_cnt = 0;
- struct value *head = v;
/* The idea here is that evaluating an expression generates a series
of values, one holding the value of every subexpression. (The
@@ -1860,8 +1859,10 @@ num_memory_accesses (struct value *v)
notice that an expression contains an inferior function call.
FIXME. */
- for (; v; v = value_next (v))
+ for (const value_ref_ptr &iter : chain)
{
+ struct value *v = iter.get ();
+
/* Constants and values from the history are fine. */
if (VALUE_LVAL (v) == not_lval || deprecated_value_modifiable (v) == 0)
continue;
@@ -1892,7 +1893,8 @@ check_condition (CORE_ADDR watch_addr, struct expression *cond,
CORE_ADDR *data_value, int *len)
{
int pc = 1, num_accesses_left, num_accesses_right;
- struct value *left_val, *right_val, *left_chain, *right_chain;
+ struct value *left_val, *right_val;
+ std::vector<value_ref_ptr> left_chain, right_chain;
if (cond->elts[0].opcode != BINOP_EQUAL)
return 0;
@@ -1901,22 +1903,13 @@ check_condition (CORE_ADDR watch_addr, struct expression *cond,
num_accesses_left = num_memory_accesses (left_chain);
if (left_val == NULL || num_accesses_left < 0)
- {
- free_value_chain (left_chain);
-
- return 0;
- }
+ return 0;
fetch_subexp_value (cond, &pc, &right_val, NULL, &right_chain, 0);
num_accesses_right = num_memory_accesses (right_chain);
if (right_val == NULL || num_accesses_right < 0)
- {
- free_value_chain (left_chain);
- free_value_chain (right_chain);
-
- return 0;
- }
+ return 0;
if (num_accesses_left == 1 && num_accesses_right == 0
&& VALUE_LVAL (left_val) == lval_memory
@@ -1939,15 +1932,7 @@ check_condition (CORE_ADDR watch_addr, struct expression *cond,
*len = TYPE_LENGTH (check_typedef (value_type (right_val)));
}
else
- {
- free_value_chain (left_chain);
- free_value_chain (right_chain);
-
- return 0;
- }
-
- free_value_chain (left_chain);
- free_value_chain (right_chain);
+ return 0;
return 1;
}
diff --git a/gdb/value.c b/gdb/value.c
index a84c196aaa..fed722da51 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1638,20 +1638,6 @@ value_free_to_mark (const struct value *mark)
all_values = val;
}
-/* Frees all the elements in a chain of values. */
-
-void
-free_value_chain (struct value *v)
-{
- struct value *next;
-
- for (; v; v = next)
- {
- next = value_next (v);
- value_decref (v);
- }
-}
-
/* Remove VAL from the chain all_values
so it will not be freed automatically. */
@@ -1696,24 +1682,28 @@ release_value (struct value *val)
}
/* Release all values up to mark */
-struct value *
+std::vector<value_ref_ptr>
value_release_to_mark (const struct value *mark)
{
- struct value *val;
+ std::vector<value_ref_ptr> result;
struct value *next;
- for (val = next = all_values; next; next = next->next)
+ for (next = all_values; next; next = next->next)
{
+ next->released = 1;
+ result.emplace_back (next);
+
if (next->next == mark)
{
- all_values = next->next;
+ struct value *save = next->next;
next->next = NULL;
- return val;
+ next = save;
+ break;
}
- next->released = 1;
}
- all_values = 0;
- return val;
+
+ all_values = next;
+ return result;
}
/* Return a copy of the value ARG.
diff --git a/gdb/value.h b/gdb/value.h
index 2016937406..9c6a1b8859 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -915,7 +915,7 @@ extern value *eval_skip_value (expression *exp);
extern void fetch_subexp_value (struct expression *exp, int *pc,
struct value **valp, struct value **resultp,
- struct value **val_chain,
+ std::vector<value_ref_ptr> *val_chain,
int preserve_errors);
extern const char *extract_field_op (struct expression *exp, int *subexp);
@@ -1053,8 +1053,6 @@ extern int unop_user_defined_p (enum exp_opcode op, struct value *arg1);
extern int destructor_name_p (const char *name, struct type *type);
-extern void free_value_chain (struct value *v);
-
extern value_ref_ptr release_value (struct value *val);
extern int record_latest_value (struct value *val);
@@ -1084,7 +1082,8 @@ extern void value_print_array_elements (struct value *val,
struct ui_file *stream, int format,
enum val_prettyformat pretty);
-extern struct value *value_release_to_mark (const struct value *mark);
+extern std::vector<value_ref_ptr> value_release_to_mark
+ (const struct value *mark);
extern void val_print (struct type *type,
LONGEST embedded_offset, CORE_ADDR address,
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 02/12] Change breakpoints to use value_ref_ptr
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (6 preceding siblings ...)
2018-04-05 21:16 ` [RFA 07/12] Remove free_value_chain Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-06 19:31 ` Pedro Alves
2018-04-05 21:16 ` [RFA 08/12] Remove value::next and value::released Tom Tromey
` (4 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
Now that value_ref_ptr exists, it is possible to simplify breakpoint
and bpstat memory management by using a value_ref_ptr rather than
manually handling the reference counts.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (release_value): Update.
* breakpoint.h (struct watchpoint) <val>: Now a value_ref_ptr.
(struct bpstats) <val>: Now a value_ref_ptr.
* breakpoint.c (update_watchpoint, breakpoint_init_inferior)
(~bpstats, bpstats, bpstat_clear_actions, watchpoint_check)
(~watchpoint, print_it_watchpoint, watch_command_1)
(invalidate_bp_value_on_memory_change): Update.
---
gdb/ChangeLog | 10 +++++++
gdb/breakpoint.c | 84 ++++++++++++++++++++++----------------------------------
gdb/breakpoint.h | 5 ++--
gdb/value.c | 3 ++
4 files changed, 48 insertions(+), 54 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 68292626d3..67ba5a6b31 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1740,8 +1740,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
no longer relevant. We don't want to report a watchpoint hit
to the user when the old value and the new value may actually
be completely different objects. */
- value_decref (b->val);
- b->val = NULL;
+ b->val.reset (nullptr);
b->val_valid = 0;
/* Note that unlike with breakpoints, the watchpoint's condition
@@ -1792,12 +1791,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
if (!b->val_valid && !is_masked_watchpoint (b))
{
if (b->val_bitsize != 0)
- {
- v = extract_bitfield_from_watchpoint_value (b, v);
- if (v != NULL)
- release_value (v).release ();
- }
- b->val = v;
+ v = extract_bitfield_from_watchpoint_value (b, v);
+ b->val = release_value (v);
b->val_valid = 1;
}
@@ -3951,9 +3946,7 @@ breakpoint_init_inferior (enum inf_context context)
{
/* Reset val field to force reread of starting value in
insert_breakpoints. */
- if (w->val)
- value_decref (w->val);
- w->val = NULL;
+ w->val.reset (nullptr);
w->val_valid = 0;
}
}
@@ -4199,8 +4192,6 @@ is_catchpoint (struct breakpoint *ep)
bpstats::~bpstats ()
{
- if (old_val != NULL)
- value_decref (old_val);
if (bp_location_at != NULL)
decref_bp_location (&bp_location_at);
}
@@ -4231,13 +4222,12 @@ bpstats::bpstats (const bpstats &other)
bp_location_at (other.bp_location_at),
breakpoint_at (other.breakpoint_at),
commands (other.commands),
- old_val (other.old_val),
print (other.print),
stop (other.stop),
print_it (other.print_it)
{
- if (old_val != NULL)
- old_val = release_value (value_copy (old_val)).release ();
+ if (other.old_val != NULL)
+ old_val = release_value (value_copy (other.old_val.get ()));
incref_bp_location (bp_location_at);
}
@@ -4358,12 +4348,7 @@ bpstat_clear_actions (void)
for (bs = tp->control.stop_bpstat; bs != NULL; bs = bs->next)
{
bs->commands = NULL;
-
- if (bs->old_val != NULL)
- {
- value_decref (bs->old_val);
- bs->old_val = NULL;
- }
+ bs->old_val.reset (nullptr);
}
}
@@ -4725,7 +4710,6 @@ bpstats::bpstats (struct bp_location *bl, bpstat **bs_link_pointer)
bp_location_at (bl),
breakpoint_at (bl->owner),
commands (NULL),
- old_val (NULL),
print (0),
stop (0),
print_it (print_it_normal)
@@ -4740,7 +4724,6 @@ bpstats::bpstats ()
bp_location_at (NULL),
breakpoint_at (NULL),
commands (NULL),
- old_val (NULL),
print (0),
stop (0),
print_it (print_it_normal)
@@ -4935,16 +4918,14 @@ watchpoint_check (bpstat bs)
the address of the array instead of its contents. This is
not what we want. */
if ((b->val != NULL) != (new_val != NULL)
- || (b->val != NULL && !value_equal_contents (b->val, new_val)))
+ || (b->val != NULL && !value_equal_contents (b->val.get (),
+ new_val)))
{
- if (new_val != NULL)
- {
- release_value (new_val).release ();
- value_free_to_mark (mark);
- }
bs->old_val = b->val;
- b->val = new_val;
+ b->val = release_value (new_val);
b->val_valid = 1;
+ if (new_val != NULL)
+ value_free_to_mark (mark);
return WP_VALUE_CHANGED;
}
else
@@ -10099,7 +10080,6 @@ watchpoint::~watchpoint ()
{
xfree (this->exp_string);
xfree (this->exp_string_reparse);
- value_decref (this->val);
}
/* Implement the "re_set" breakpoint_ops method for watchpoints. */
@@ -10241,10 +10221,10 @@ print_it_watchpoint (bpstat bs)
mention (b);
tuple_emitter.emplace (uiout, "value");
uiout->text ("\nOld value = ");
- watchpoint_value_print (bs->old_val, &stb);
+ watchpoint_value_print (bs->old_val.get (), &stb);
uiout->field_stream ("old", stb);
uiout->text ("\nNew value = ");
- watchpoint_value_print (w->val, &stb);
+ watchpoint_value_print (w->val.get (), &stb);
uiout->field_stream ("new", stb);
uiout->text ("\n");
/* More than one watchpoint may have been triggered. */
@@ -10258,7 +10238,7 @@ print_it_watchpoint (bpstat bs)
mention (b);
tuple_emitter.emplace (uiout, "value");
uiout->text ("\nValue = ");
- watchpoint_value_print (w->val, &stb);
+ watchpoint_value_print (w->val.get (), &stb);
uiout->field_stream ("value", stb);
uiout->text ("\n");
result = PRINT_UNKNOWN;
@@ -10274,7 +10254,7 @@ print_it_watchpoint (bpstat bs)
mention (b);
tuple_emitter.emplace (uiout, "value");
uiout->text ("\nOld value = ");
- watchpoint_value_print (bs->old_val, &stb);
+ watchpoint_value_print (bs->old_val.get (), &stb);
uiout->field_stream ("old", stb);
uiout->text ("\nNew value = ");
}
@@ -10288,7 +10268,7 @@ print_it_watchpoint (bpstat bs)
tuple_emitter.emplace (uiout, "value");
uiout->text ("\nValue = ");
}
- watchpoint_value_print (w->val, &stb);
+ watchpoint_value_print (w->val.get (), &stb);
uiout->field_stream ("new", stb);
uiout->text ("\n");
result = PRINT_UNKNOWN;
@@ -10583,7 +10563,7 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
{
struct breakpoint *scope_breakpoint = NULL;
const struct block *exp_valid_block = NULL, *cond_exp_valid_block = NULL;
- struct value *val, *mark, *result;
+ struct value *mark, *result;
int saved_bitpos = 0, saved_bitsize = 0;
const char *exp_start = NULL;
const char *exp_end = NULL;
@@ -10709,25 +10689,28 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
exp_valid_block = innermost_block.block ();
mark = value_mark ();
- fetch_subexp_value (exp.get (), &pc, &val, &result, NULL, just_location);
+ struct value *val_as_value = nullptr;
+ fetch_subexp_value (exp.get (), &pc, &val_as_value, &result, NULL,
+ just_location);
- if (val != NULL && just_location)
+ if (val_as_value != NULL && just_location)
{
- saved_bitpos = value_bitpos (val);
- saved_bitsize = value_bitsize (val);
+ saved_bitpos = value_bitpos (val_as_value);
+ saved_bitsize = value_bitsize (val_as_value);
}
+ value_ref_ptr val;
if (just_location)
{
int ret;
exp_valid_block = NULL;
- val = release_value (value_addr (result)).release ();
+ val = release_value (value_addr (result));
value_free_to_mark (mark);
if (use_mask)
{
- ret = target_masked_watch_num_registers (value_as_address (val),
+ ret = target_masked_watch_num_registers (value_as_address (val.get ()),
mask);
if (ret == -1)
error (_("This target does not support masked watchpoints."));
@@ -10735,8 +10718,8 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
error (_("Invalid mask or memory region."));
}
}
- else if (val != NULL)
- release_value (val).release ();
+ else if (val_as_value != NULL)
+ val = release_value (val_as_value);
tok = skip_spaces (arg);
end_tok = skip_to_space (tok);
@@ -10830,8 +10813,8 @@ watch_command_1 (const char *arg, int accessflag, int from_tty,
w->cond_exp_valid_block = cond_exp_valid_block;
if (just_location)
{
- struct type *t = value_type (val);
- CORE_ADDR addr = value_as_address (val);
+ struct type *t = value_type (val.get ());
+ CORE_ADDR addr = value_as_address (val.get ());
w->exp_string_reparse
= current_language->la_watch_location_expression (t, addr).release ();
@@ -14533,7 +14516,7 @@ invalidate_bp_value_on_memory_change (struct inferior *inferior,
{
struct watchpoint *wp = (struct watchpoint *) bp;
- if (wp->val_valid && wp->val)
+ if (wp->val_valid && wp->val.get ())
{
struct bp_location *loc;
@@ -14542,8 +14525,7 @@ invalidate_bp_value_on_memory_change (struct inferior *inferior,
&& loc->address + loc->length > addr
&& addr + len > loc->address)
{
- value_decref (wp->val);
- wp->val = NULL;
+ wp->val.reset (nullptr);
wp->val_valid = 0;
}
}
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d8e2b73edc..062e390469 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -31,7 +31,6 @@
#include "common/array-view.h"
#include "cli/cli-script.h"
-struct value;
struct block;
struct gdbpy_breakpoint_object;
struct gdbscm_breakpoint_object;
@@ -813,7 +812,7 @@ struct watchpoint : public breakpoint
/* Value of the watchpoint the last time we checked it, or NULL when
we do not know the value yet or the value was not readable. VAL
is never lazy. */
- struct value *val;
+ value_ref_ptr val;
/* Nonzero if VAL is valid. If VAL_VALID is set but VAL is NULL,
then an error occurred reading the value. */
int val_valid;
@@ -1125,7 +1124,7 @@ struct bpstats
counted_command_line commands;
/* Old value associated with a watchpoint. */
- struct value *old_val;
+ value_ref_ptr old_val;
/* Nonzero if this breakpoint tells us to print the frame. */
char print;
diff --git a/gdb/value.c b/gdb/value.c
index 002270f634..013fcfe3ed 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1696,6 +1696,9 @@ release_value (struct value *val)
struct value *v;
bool released = false;
+ if (val == nullptr)
+ return value_ref_ptr ();
+
if (all_values == val)
{
all_values = val->next;
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 02/12] Change breakpoints to use value_ref_ptr
2018-04-05 21:16 ` [RFA 02/12] Change breakpoints to use value_ref_ptr Tom Tromey
@ 2018-04-06 19:31 ` Pedro Alves
2018-04-06 21:31 ` Tom Tromey
0 siblings, 1 reply; 22+ messages in thread
From: Pedro Alves @ 2018-04-06 19:31 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
On 04/05/2018 10:14 PM, Tom Tromey wrote:
> 4 files changed, 48 insertions(+), 54 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 68292626d3..67ba5a6b31 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1740,8 +1740,7 @@ update_watchpoint (struct watchpoint *b, int reparse)
> no longer relevant. We don't want to report a watchpoint hit
> to the user when the old value and the new value may actually
> be completely different objects. */
> - value_decref (b->val);
> - b->val = NULL;
> + b->val.reset (nullptr);
Just OOC, wouldn't the old "b->val = NULL;" work the same as the
reset call?
> b->val_valid = 0;
> @@ -14533,7 +14516,7 @@ invalidate_bp_value_on_memory_change (struct inferior *inferior,
> {
> struct watchpoint *wp = (struct watchpoint *) bp;
>
> - if (wp->val_valid && wp->val)
> + if (wp->val_valid && wp->val.get ())
Nit, about this get(): I wonder whether we should add an
explicit operator bool()
implementation to gdb_ref_ptr to avoid it.
I guess we should instead write the explicit:
if (wp->val_valid && wp->val != nullptr)
making that moot.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 02/12] Change breakpoints to use value_ref_ptr
2018-04-06 19:31 ` Pedro Alves
@ 2018-04-06 21:31 ` Tom Tromey
0 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-06 21:31 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> + b->val.reset (nullptr);
Pedro> Just OOC, wouldn't the old "b->val = NULL;" work the same as the
Pedro> reset call?
Yeah. I reverted the two spots where I changed this.
>> + if (wp->val_valid && wp->val.get ())
Pedro> Nit, about this get(): I wonder whether we should add an
Pedro> explicit operator bool()
Pedro> implementation to gdb_ref_ptr to avoid it.
Pedro> I guess we should instead write the explicit:
Pedro> if (wp->val_valid && wp->val != nullptr)
Pedro> making that moot.
I did this.
I'm on the fence about the operator bool thing.
I think 'bool (wp)" and "wp != nullptr" are both about as explicit and
readable. But, the latter is currently used more widely.
Tom
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFA 08/12] Remove value::next and value::released
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (7 preceding siblings ...)
2018-04-05 21:16 ` [RFA 02/12] Change breakpoints to use value_ref_ptr Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-06 19:32 ` Pedro Alves
2018-04-05 21:16 ` [RFA 06/12] Remove free_all_values Tom Tromey
` (3 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This patch converts all_values to simply hold a list of references to
values. Now, there's no need to have a value record whether or not it
is released -- there is only a single reference-counting mechanism for
values. So, this also removes value::next, value::released, and
value_next.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (struct value) <released, next>: Remove.
(all_values): Now a std::vector.
(allocate_value_lazy): Update.
(value_next): Remove.
(value_mark, value_free_to_mark, release_value)
(value_release_to_mark): Update.
---
gdb/ChangeLog | 9 ++++++
gdb/value.c | 95 +++++++++++++++++------------------------------------------
2 files changed, 36 insertions(+), 68 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index fed722da51..3137bf48f8 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -198,9 +198,6 @@ struct value
used instead of read_memory to enable extra caching. */
unsigned int stack : 1;
- /* If the value has been released. */
- unsigned int released : 1;
-
/* Location of value (if lval). */
union
{
@@ -309,12 +306,6 @@ struct value
LONGEST embedded_offset;
LONGEST pointed_to_offset;
- /* Values are stored in a chain, so that they can be deleted easily
- over calls to the inferior. Values assigned to internal
- variables, put into the value history or exposed to Python are
- taken off this list. */
- struct value *next;
-
/* Actual contents of the value. Target byte-order. NULL or not
valid if lazy is nonzero. */
gdb_byte *contents;
@@ -891,7 +882,7 @@ static std::vector<value_ref_ptr> value_history;
(except for those released by calls to release_value)
This is so they can be freed after each command. */
-static struct value *all_values;
+static std::vector<value_ref_ptr> all_values;
/* Allocate a lazy value for type TYPE. Its actual content is
"lazily" allocated too: the content field of the return value is
@@ -912,8 +903,6 @@ allocate_value_lazy (struct type *type)
val = XCNEW (struct value);
val->contents = NULL;
- val->next = all_values;
- all_values = val;
val->type = type;
val->enclosing_type = type;
VALUE_LVAL (val) = not_lval;
@@ -929,6 +918,7 @@ allocate_value_lazy (struct type *type)
/* Values start out on the all_values chain. */
val->reference_count = 1;
+ all_values.emplace_back (val);
return val;
}
@@ -1070,12 +1060,6 @@ allocate_optimized_out_value (struct type *type)
/* Accessor methods. */
-struct value *
-value_next (const struct value *value)
-{
- return value->next;
-}
-
struct type *
value_type (const struct value *value)
{
@@ -1573,7 +1557,9 @@ deprecated_value_modifiable (const struct value *value)
struct value *
value_mark (void)
{
- return all_values;
+ if (all_values.empty ())
+ return nullptr;
+ return all_values.back ().get ();
}
/* Take a reference to VAL. VAL will not be deallocated until all
@@ -1626,16 +1612,11 @@ value_decref (struct value *val)
void
value_free_to_mark (const struct value *mark)
{
- struct value *val;
- struct value *next;
-
- for (val = all_values; val && val != mark; val = next)
- {
- next = val->next;
- val->released = 1;
- value_decref (val);
- }
- all_values = val;
+ auto iter = std::find (all_values.begin (), all_values.end (), mark);
+ if (iter == all_values.end ())
+ all_values.clear ();
+ else
+ all_values.erase (iter + 1, all_values.end ());
}
/* Remove VAL from the chain all_values
@@ -1645,40 +1626,25 @@ value_ref_ptr
release_value (struct value *val)
{
struct value *v;
- bool released = false;
if (val == nullptr)
return value_ref_ptr ();
- if (all_values == val)
- {
- all_values = val->next;
- val->next = NULL;
- released = true;
- }
- else
+ std::vector<value_ref_ptr>::reverse_iterator iter;
+ for (iter = all_values.rbegin (); iter != all_values.rend (); ++iter)
{
- for (v = all_values; v; v = v->next)
+ if (*iter == val)
{
- if (v->next == val)
- {
- v->next = val->next;
- val->next = NULL;
- released = true;
- break;
- }
+ value_ref_ptr result = *iter;
+ all_values.erase (iter.base () - 1);
+ return result;
}
}
- if (!released)
- {
- /* We must always return an owned reference. Normally this
- happens because we transfer the reference from the value
- chain, but in this case the value was not on the chain. */
- value_incref (val);
- }
-
- return value_ref_ptr (val);
+ /* We must always return an owned reference. Normally this happens
+ because we transfer the reference from the value chain, but in
+ this case the value was not on the chain. */
+ return value_ref_ptr (value_incref (val));
}
/* Release all values up to mark */
@@ -1686,23 +1652,16 @@ std::vector<value_ref_ptr>
value_release_to_mark (const struct value *mark)
{
std::vector<value_ref_ptr> result;
- struct value *next;
- for (next = all_values; next; next = next->next)
+ auto iter = std::find (all_values.begin (), all_values.end (), mark);
+ if (iter == all_values.end ())
+ std::swap (result, all_values);
+ else
{
- next->released = 1;
- result.emplace_back (next);
-
- if (next->next == mark)
- {
- struct value *save = next->next;
- next->next = NULL;
- next = save;
- break;
- }
+ std::move (iter + 1, all_values.end (), std::back_inserter (result));
+ all_values.erase (iter + 1, all_values.end ());
}
-
- all_values = next;
+ std::reverse (result.begin (), result.end ());
return result;
}
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 08/12] Remove value::next and value::released
2018-04-05 21:16 ` [RFA 08/12] Remove value::next and value::released Tom Tromey
@ 2018-04-06 19:32 ` Pedro Alves
2018-04-06 21:40 ` Tom Tromey
0 siblings, 1 reply; 22+ messages in thread
From: Pedro Alves @ 2018-04-06 19:32 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
On 04/05/2018 10:15 PM, Tom Tromey wrote:
> value_release_to_mark (const struct value *mark)
> {
...
> + std::reverse (result.begin (), result.end ());
> return result;
> }
Is there a comment somewhere mentioning the order of the
returned values?
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 08/12] Remove value::next and value::released
2018-04-06 19:32 ` Pedro Alves
@ 2018-04-06 21:40 ` Tom Tromey
0 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-06 21:40 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> On 04/05/2018 10:15 PM, Tom Tromey wrote:
>> value_release_to_mark (const struct value *mark)
>> {
Pedro> ...
>> + std::reverse (result.begin (), result.end ());
>> return result;
>> }
Pedro> Is there a comment somewhere mentioning the order of the
Pedro> returned values?
Nope. value.h doesn't generally seem to have comments; but since really
it should, I've done this:
/* Release values from the value chain and return them. Values
created after MARK are released. If MARK is nullptr, or if MARK is
not found on the value chain, then all values are released. Values
are returned in reverse order of creation; that is, newest
first. */
extern std::vector<value_ref_ptr> value_release_to_mark
(const struct value *mark);
Then in value.c I added the usual:
/* See value.h. */
std::vector<value_ref_ptr>
value_release_to_mark (const struct value *mark)
{
...
Tom
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFA 06/12] Remove free_all_values
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (8 preceding siblings ...)
2018-04-05 21:16 ` [RFA 08/12] Remove value::next and value::released Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 10/12] Change value::parent to a value_ref_ptr Tom Tromey
` (2 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
free_all_values is unused, so this removes it.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.h (free_all_values): Remove.
* value.c (free_all_values): Remove.
---
gdb/ChangeLog | 5 +++++
gdb/value.c | 20 --------------------
gdb/value.h | 2 --
3 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index 677ec42e63..a84c196aaa 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1638,26 +1638,6 @@ value_free_to_mark (const struct value *mark)
all_values = val;
}
-/* Free all the values that have been allocated (except for those released).
- Call after each command, successful or not.
- In practice this is called before each command, which is sufficient. */
-
-void
-free_all_values (void)
-{
- struct value *val;
- struct value *next;
-
- for (val = all_values; val; val = next)
- {
- next = val->next;
- val->released = 1;
- value_decref (val);
- }
-
- all_values = 0;
-}
-
/* Frees all the elements in a chain of values. */
void
diff --git a/gdb/value.h b/gdb/value.h
index f7e7387ff1..2016937406 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1053,8 +1053,6 @@ extern int unop_user_defined_p (enum exp_opcode op, struct value *arg1);
extern int destructor_name_p (const char *name, struct type *type);
-extern void free_all_values (void);
-
extern void free_value_chain (struct value *v);
extern value_ref_ptr release_value (struct value *val);
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 10/12] Change value::parent to a value_ref_ptr
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (9 preceding siblings ...)
2018-04-05 21:16 ` [RFA 06/12] Remove free_all_values Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-05 21:16 ` [RFA 04/12] Change varobj to use value_ref_ptr Tom Tromey
2018-04-06 19:33 ` [RFA 00/12] (somewhat) clean up struct value ownership Pedro Alves
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This changes value::parent to a value_ref_ptr. This removes a bit of
manual reference count management.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* value.c (~value): Update.
(struct value) <parent>: Now a value_ref_ptr.
(value_parent, set_value_parent, value_address, value_copy):
Update.
---
gdb/ChangeLog | 7 +++++++
gdb/value.c | 20 +++++---------------
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index 30af33c4d2..e25934b9e3 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -181,11 +181,6 @@ struct value
~value ()
{
- /* If there's an associated parent value, drop our reference to
- it. */
- if (parent != NULL)
- value_decref (parent);
-
if (VALUE_LVAL (this) == lval_computed)
{
const struct lval_funcs *funcs = location.computed.funcs;
@@ -292,7 +287,7 @@ struct value
/* Only used for bitfields; the containing value. This allows a
single read from the target when displaying multiple
bitfields. */
- struct value *parent = nullptr;
+ value_ref_ptr parent;
/* Type of the value. */
struct type *type;
@@ -1128,7 +1123,7 @@ set_value_bitsize (struct value *value, LONGEST bit)
struct value *
value_parent (const struct value *value)
{
- return value->parent;
+ return value->parent.get ();
}
/* See value.h. */
@@ -1136,12 +1131,7 @@ value_parent (const struct value *value)
void
set_value_parent (struct value *value, struct value *parent)
{
- struct value *old = value->parent;
-
- value->parent = parent;
- if (parent != NULL)
- value_incref (parent);
- value_decref (old);
+ value->parent = value_ref_ptr (value_incref (parent));
}
gdb_byte *
@@ -1521,7 +1511,7 @@ value_address (const struct value *value)
if (value->lval != lval_memory)
return 0;
if (value->parent != NULL)
- return value_address (value->parent) + value->offset;
+ return value_address (value->parent.get ()) + value->offset;
if (NULL != TYPE_DATA_LOCATION (value_type (value)))
{
gdb_assert (PROP_CONST == TYPE_DATA_LOCATION_KIND (value_type (value)));
@@ -1699,7 +1689,7 @@ value_copy (struct value *arg)
}
val->unavailable = VEC_copy (range_s, arg->unavailable);
val->optimized_out = VEC_copy (range_s, arg->optimized_out);
- set_value_parent (val, arg->parent);
+ val->parent = arg->parent;
if (VALUE_LVAL (val) == lval_computed)
{
const struct lval_funcs *funcs = val->location.computed.funcs;
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* [RFA 04/12] Change varobj to use value_ref_ptr
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (10 preceding siblings ...)
2018-04-05 21:16 ` [RFA 10/12] Change value::parent to a value_ref_ptr Tom Tromey
@ 2018-04-05 21:16 ` Tom Tromey
2018-04-06 19:33 ` [RFA 00/12] (somewhat) clean up struct value ownership Pedro Alves
12 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-05 21:16 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
This changes varobj to use value_ref_ptr, allowing the removal of some
manual reference count management.
ChangeLog
2018-04-05 Tom Tromey <tom@tromey.com>
* varobj.h (struct varobj) <value>: Now a value_ref_ptr.
* varobj.c (varobj_set_display_format, varobj_set_value)
(install_default_visualizer, construct_visualizer)
(install_new_value, ~varobj, varobj_get_value_type)
(my_value_of_variable, varobj_editable_p): Update.
* c-varobj.c (c_describe_child, c_value_of_variable)
(cplus_number_of_children, cplus_describe_child): Update.
* ada-varobj.c (ada_number_of_children, ada_name_of_child)
(ada_path_expr_of_child, ada_value_of_child, ada_type_of_child)
(ada_value_of_variable, ada_value_is_changeable_p): Update.
---
gdb/ChangeLog | 13 +++++++++++++
gdb/ada-varobj.c | 16 +++++++++-------
gdb/c-varobj.c | 15 ++++++++-------
gdb/varobj.c | 43 ++++++++++++++++++++++---------------------
gdb/varobj.h | 3 ++-
5 files changed, 54 insertions(+), 36 deletions(-)
diff --git a/gdb/ada-varobj.c b/gdb/ada-varobj.c
index 1257ee864e..6dafe472c7 100644
--- a/gdb/ada-varobj.c
+++ b/gdb/ada-varobj.c
@@ -872,7 +872,7 @@ ada_varobj_get_value_of_variable (struct value *value,
static int
ada_number_of_children (const struct varobj *var)
{
- return ada_varobj_get_number_of_children (var->value, var->type);
+ return ada_varobj_get_number_of_children (var->value.get (), var->type);
}
static std::string
@@ -884,7 +884,7 @@ ada_name_of_variable (const struct varobj *parent)
static std::string
ada_name_of_child (const struct varobj *parent, int index)
{
- return ada_varobj_get_name_of_child (parent->value, parent->type,
+ return ada_varobj_get_name_of_child (parent->value.get (), parent->type,
parent->name.c_str (), index);
}
@@ -894,7 +894,7 @@ ada_path_expr_of_child (const struct varobj *child)
const struct varobj *parent = child->parent;
const char *parent_path_expr = varobj_get_path_expr (parent);
- return ada_varobj_get_path_expr_of_child (parent->value,
+ return ada_varobj_get_path_expr_of_child (parent->value.get (),
parent->type,
parent->name.c_str (),
parent_path_expr,
@@ -904,14 +904,14 @@ ada_path_expr_of_child (const struct varobj *child)
static struct value *
ada_value_of_child (const struct varobj *parent, int index)
{
- return ada_varobj_get_value_of_child (parent->value, parent->type,
+ return ada_varobj_get_value_of_child (parent->value.get (), parent->type,
parent->name.c_str (), index);
}
static struct type *
ada_type_of_child (const struct varobj *parent, int index)
{
- return ada_varobj_get_type_of_child (parent->value, parent->type,
+ return ada_varobj_get_type_of_child (parent->value.get (), parent->type,
index);
}
@@ -923,7 +923,8 @@ ada_value_of_variable (const struct varobj *var,
varobj_formatted_print_options (&opts, format);
- return ada_varobj_get_value_of_variable (var->value, var->type, &opts);
+ return ada_varobj_get_value_of_variable (var->value.get (), var->type,
+ &opts);
}
/* Implement the "value_is_changeable_p" routine for Ada. */
@@ -931,7 +932,8 @@ ada_value_of_variable (const struct varobj *var,
static bool
ada_value_is_changeable_p (const struct varobj *var)
{
- struct type *type = var->value ? value_type (var->value) : var->type;
+ struct type *type = (var->value != nullptr
+ ? value_type (var->value.get ()) : var->type);
if (ada_is_array_descriptor_type (type)
&& TYPE_CODE (type) == TYPE_CODE_TYPEDEF)
diff --git a/gdb/c-varobj.c b/gdb/c-varobj.c
index babda29278..03656c0a81 100644
--- a/gdb/c-varobj.c
+++ b/gdb/c-varobj.c
@@ -286,7 +286,7 @@ c_describe_child (const struct varobj *parent, int index,
std::string *cname, struct value **cvalue,
struct type **ctype, std::string *cfull_expression)
{
- struct value *value = parent->value;
+ struct value *value = parent->value.get ();
struct type *type = varobj_get_value_type (parent);
std::string parent_expression;
int was_ptr;
@@ -513,21 +513,22 @@ c_value_of_variable (const struct varobj *var,
}
else
{
- if (var->not_fetched && value_lazy (var->value))
+ if (var->not_fetched && value_lazy (var->value.get ()))
/* Frozen variable and no value yet. We don't
implicitly fetch the value. MI response will
use empty string for the value, which is OK. */
return std::string ();
gdb_assert (varobj_value_is_changeable_p (var));
- gdb_assert (!value_lazy (var->value));
+ gdb_assert (!value_lazy (var->value.get ()));
/* If the specified format is the current one,
we can reuse print_value. */
if (format == var->format)
return var->print_value;
else
- return varobj_value_get_print_value (var->value, format, var);
+ return varobj_value_get_print_value (var->value.get (), format,
+ var);
}
}
}
@@ -579,7 +580,7 @@ cplus_number_of_children (const struct varobj *var)
/* It is necessary to access a real type (via RTTI). */
if (opts.objectprint)
{
- value = var->value;
+ value = var->value.get ();
lookup_actual_type = (TYPE_IS_REFERENCE (var->type)
|| TYPE_CODE (var->type) == TYPE_CODE_PTR);
}
@@ -616,7 +617,7 @@ cplus_number_of_children (const struct varobj *var)
{
const struct varobj *parent = var->parent;
- value = parent->value;
+ value = parent->value.get ();
lookup_actual_type = (TYPE_IS_REFERENCE (parent->type)
|| TYPE_CODE (parent->type) == TYPE_CODE_PTR);
}
@@ -724,7 +725,7 @@ cplus_describe_child (const struct varobj *parent, int index,
if (opts.objectprint)
lookup_actual_type = (TYPE_IS_REFERENCE (var->type)
|| TYPE_CODE (var->type) == TYPE_CODE_PTR);
- value = var->value;
+ value = var->value.get ();
type = varobj_get_value_type (var);
if (cfull_expression)
parent_expression
diff --git a/gdb/varobj.c b/gdb/varobj.c
index 58eb531927..2d00964317 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -522,9 +522,9 @@ varobj_set_display_format (struct varobj *var,
}
if (varobj_value_is_changeable_p (var)
- && var->value && !value_lazy (var->value))
+ && var->value != nullptr && !value_lazy (var->value.get ()))
{
- var->print_value = varobj_value_get_print_value (var->value,
+ var->print_value = varobj_value_get_print_value (var->value.get (),
var->format, var);
}
@@ -1045,7 +1045,7 @@ varobj_set_value (struct varobj *var, const char *expression)
gdb_assert (varobj_value_is_changeable_p (var));
/* The value of a changeable variable object must not be lazy. */
- gdb_assert (!value_lazy (var->value));
+ gdb_assert (!value_lazy (var->value.get ()));
/* Need to coerce the input. We want to check if the
value of the variable object will be different
@@ -1060,7 +1060,7 @@ varobj_set_value (struct varobj *var, const char *expression)
rather value_contents, will take care of this. */
TRY
{
- val = value_assign (var->value, value);
+ val = value_assign (var->value.get (), value);
}
CATCH (except, RETURN_MASK_ERROR)
@@ -1112,9 +1112,9 @@ install_default_visualizer (struct varobj *var)
{
PyObject *pretty_printer = NULL;
- if (var->value)
+ if (var->value != nullptr)
{
- pretty_printer = gdbpy_get_varobj_pretty_printer (var->value);
+ pretty_printer = gdbpy_get_varobj_pretty_printer (var->value.get ());
if (! pretty_printer)
{
gdbpy_print_stack ();
@@ -1149,7 +1149,8 @@ construct_visualizer (struct varobj *var, PyObject *constructor)
pretty_printer = NULL;
else
{
- pretty_printer = instantiate_pretty_printer (constructor, var->value);
+ pretty_printer = instantiate_pretty_printer (constructor,
+ var->value.get ());
if (! pretty_printer)
{
gdbpy_print_stack ();
@@ -1326,8 +1327,9 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
/* Get a reference now, before possibly passing it to any Python
code that might release it. */
+ value_ref_ptr value_holder;
if (value != NULL)
- value_incref (value);
+ value_holder.reset (value_incref (value));
/* Below, we'll be comparing string rendering of old and new
values. Don't get string rendering if the value is
@@ -1354,7 +1356,7 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
{
/* Try to compare the values. That requires that both
values are non-lazy. */
- if (var->not_fetched && value_lazy (var->value))
+ if (var->not_fetched && value_lazy (var->value.get ()))
{
/* This is a frozen varobj and the value was never read.
Presumably, UI shows some "never read" indicator.
@@ -1372,7 +1374,7 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
}
else
{
- gdb_assert (!value_lazy (var->value));
+ gdb_assert (!value_lazy (var->value.get ()));
gdb_assert (!value_lazy (value));
gdb_assert (!var->print_value.empty () && !print_value.empty ());
@@ -1392,9 +1394,7 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
}
/* We must always keep the new value, since children depend on it. */
- if (var->value != NULL && var->value != value)
- value_decref (var->value);
- var->value = value;
+ var->value = value_holder;
if (value && value_lazy (value) && intentionally_not_fetched)
var->not_fetched = true;
else
@@ -1407,8 +1407,8 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
to see if the variable changed. */
if (var->dynamic->pretty_printer != NULL)
{
- print_value = varobj_value_get_print_value (var->value, var->format,
- var);
+ print_value = varobj_value_get_print_value (var->value.get (),
+ var->format, var);
if ((var->print_value.empty () && !print_value.empty ())
|| (!var->print_value.empty () && print_value.empty ())
|| (!var->print_value.empty () && !print_value.empty ()
@@ -1417,7 +1417,7 @@ install_new_value (struct varobj *var, struct value *value, bool initial)
}
var->print_value = print_value;
- gdb_assert (!var->value || value_type (var->value));
+ gdb_assert (var->value == nullptr || value_type (var->value.get ()));
return changed;
}
@@ -1990,7 +1990,6 @@ varobj::~varobj ()
varobj_iter_delete (var->dynamic->child_iter);
varobj_clear_saved_item (var->dynamic);
- value_decref (var->value);
if (is_root_p (var))
delete var->root;
@@ -2014,8 +2013,8 @@ varobj_get_value_type (const struct varobj *var)
{
struct type *type;
- if (var->value)
- type = value_type (var->value);
+ if (var->value != nullptr)
+ type = value_type (var->value.get ());
else
type = var->type;
@@ -2261,7 +2260,8 @@ my_value_of_variable (struct varobj *var, enum varobj_display_formats format)
if (var->root->is_valid)
{
if (var->dynamic->pretty_printer != NULL)
- return varobj_value_get_print_value (var->value, var->format, var);
+ return varobj_value_get_print_value (var->value.get (), var->format,
+ var);
return (*var->root->lang_ops->value_of_variable) (var, format);
}
else
@@ -2397,7 +2397,8 @@ varobj_editable_p (const struct varobj *var)
{
struct type *type;
- if (!(var->root->is_valid && var->value && VALUE_LVAL (var->value)))
+ if (!(var->root->is_valid && var->value != nullptr
+ && VALUE_LVAL (var->value.get ())))
return false;
type = varobj_get_value_type (var);
diff --git a/gdb/varobj.h b/gdb/varobj.h
index 6e80d1b89e..3aba0cda67 100644
--- a/gdb/varobj.h
+++ b/gdb/varobj.h
@@ -20,6 +20,7 @@
#include "symtab.h"
#include "gdbtypes.h"
#include "vec.h"
+#include "value.h"
/* Enumeration for the format types */
enum varobj_display_formats
@@ -122,7 +123,7 @@ struct varobj
indicates there was an error getting this value.
Invariant: if varobj_value_is_changeable_p (this) is non-zero,
the value is either NULL, or not lazy. */
- struct value *value = NULL;
+ value_ref_ptr value;
/* The number of (immediate) children this variable has. */
int num_children = -1;
--
2.13.6
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 00/12] (somewhat) clean up struct value ownership
2018-04-05 21:16 [RFA 00/12] (somewhat) clean up struct value ownership Tom Tromey
` (11 preceding siblings ...)
2018-04-05 21:16 ` [RFA 04/12] Change varobj to use value_ref_ptr Tom Tromey
@ 2018-04-06 19:33 ` Pedro Alves
2018-04-06 21:20 ` Tom Tromey
` (2 more replies)
12 siblings, 3 replies; 22+ messages in thread
From: Pedro Alves @ 2018-04-06 19:33 UTC (permalink / raw)
To: Tom Tromey, gdb-patches
On 04/05/2018 10:14 PM, Tom Tromey wrote:
> struct value has long suffered from a complicated approach to
> ownership. In particular, values are reference counted, but they are
> also handled specially if they are on the "value chain". I believe
> this has led to bugs on occasion; and anyway requires oddities like
> release_value_or_incref.
>
> This series goes some way toward cleaning up ownership for values. It
> introduces a gdb_ref_ptr specialization for values and then changes
> various things to use it. After this, it cleans up various code doing
> manual memory mangement related to value; and in particular unifies
> the value chain with the reference counting mechanism.
>
This is seriously cool. Thanks much for doing this.
I've sent a few comments here and there, but overall this looks
good to me.
> There is still more work that could be done in this area. For
> example:
>
> * struct lval_funcs could be turned into a base class and then the
> implementers rewritten as ordinary objects.
>
> * Likewise struct internalvar_funcs; and this would allow better type
> safety through the removal of union internalvar_data.
>
> * Perhaps the "location" union could be removed from struct value,
> also providing more type safety.
>
Yeah, this would be nice.
> However, I didn't do these, as this series seemed to reach a
> reasonable stopping point.
Definitely agreed.
As I'm reading the series, I'm also wishing for making some of the
value-related free-functions methods of value instead, so that
instead of having to write ref_ptr::get() calls like in:
struct type *t = value_type (val.get ());
we'd be able to write instead:
struct type *t = val->type ();
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFA 00/12] (somewhat) clean up struct value ownership
2018-04-06 19:33 ` [RFA 00/12] (somewhat) clean up struct value ownership Pedro Alves
@ 2018-04-06 21:20 ` Tom Tromey
2018-04-06 21:44 ` Tom Tromey
2018-04-08 21:32 ` Tom Tromey
2 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-06 21:20 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> As I'm reading the series, I'm also wishing for making some of the
Pedro> value-related free-functions methods of value instead
Yeah. When I wrote my note I meant to mention a couple of other things,
and then forgot to. Class-ifying value more completely was one of them.
Another one would be to do a bigger transform from "struct value *" to
"value_ref_ptr" (and "const value_ref_ptr &") everywhere, to try to
reduce the number of inc/dec-ref calls to a bare minimum, maybe even zero.
I'm not sure if I'll attempt those, since I have other maintenance
cleanups in mind as well.
Tom
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFA 00/12] (somewhat) clean up struct value ownership
2018-04-06 19:33 ` [RFA 00/12] (somewhat) clean up struct value ownership Pedro Alves
2018-04-06 21:20 ` Tom Tromey
@ 2018-04-06 21:44 ` Tom Tromey
2018-04-08 21:32 ` Tom Tromey
2 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-06 21:44 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> I've sent a few comments here and there, but overall this looks
Pedro> good to me.
Since the requested changes were minor, I'm going to check it in.
Tom
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFA 00/12] (somewhat) clean up struct value ownership
2018-04-06 19:33 ` [RFA 00/12] (somewhat) clean up struct value ownership Pedro Alves
2018-04-06 21:20 ` Tom Tromey
2018-04-06 21:44 ` Tom Tromey
@ 2018-04-08 21:32 ` Tom Tromey
2 siblings, 0 replies; 22+ messages in thread
From: Tom Tromey @ 2018-04-08 21:32 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches
Pedro> As I'm reading the series, I'm also wishing for making some of the
Pedro> value-related free-functions methods of value instead, so that
Pedro> instead of having to write ref_ptr::get() calls like in:
I did some of this today, but it's hard to know where to stop.
I was thinking maybe just the things requiring direct access to the
members; and then anything else could remain a free function. I'm not
sure though and there might be exceptions to that.
Tom
^ permalink raw reply [flat|nested] 22+ messages in thread