* [RFA] bug fixes for varobj.c
@ 2002-06-10 23:09 Martin M. Hunt
2002-06-12 19:24 ` Andrew Cagney
0 siblings, 1 reply; 3+ messages in thread
From: Martin M. Hunt @ 2002-06-10 23:09 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 739 bytes --]
Anything using varobj is randomly corrupting memory and will cause crashes in
Insight or anything using mi varobjs. This patch fixes that and some other
minor problems.
--
Martin Hunt
GDB Engineer
Red Hat, Inc.
2002-06-10 Martin M. Hunt <hunt@redhat.com>
* varobj.c (struct varobj_root): Change frame from CORE_ADDR to
struct frame_id.
(varobj_create): Store frame_id for root.
(varobj_gen_name): Use xasprintf.
(varobj_update): Save and restore frame using get_frame_id() and
frame_find_by_id().
(create_child): Use xasprintf.
(new_root_variable): Initialize frame_id.
(c_name_of_child): Use xasprintf. Call find_frame_by_id().
(c_value_of_variable): Use xasprintf. Move mem_fileopen call
to prevent memory leak.
[-- Attachment #2: v --]
[-- Type: text/x-diff, Size: 8358 bytes --]
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.28
diff -u -u -r1.28 varobj.c
--- varobj.c 5 May 2002 01:15:13 -0000 1.28
+++ varobj.c 11 Jun 2002 06:04:23 -0000
@@ -27,6 +27,7 @@
#include "varobj.h"
+
/* Non-zero if we want to see trace of varobj level stuff. */
int varobjdebug = 0;
@@ -52,7 +53,7 @@
struct block *valid_block;
/* The frame for this expression */
- CORE_ADDR frame;
+ struct frame_id frame;
/* If 1, "update" always recomputes the frame & valid block
using the currently selected frame. */
@@ -311,61 +312,61 @@
/* Array of known source language routines. */
static struct language_specific
- languages[vlang_end][sizeof (struct language_specific)] = {
+languages[vlang_end][sizeof (struct language_specific)] = {
/* Unknown (try treating as C */
{
- vlang_unknown,
- c_number_of_children,
- c_name_of_variable,
- c_name_of_child,
- c_value_of_root,
- c_value_of_child,
- c_type_of_child,
- c_variable_editable,
- c_value_of_variable}
+ vlang_unknown,
+ c_number_of_children,
+ c_name_of_variable,
+ c_name_of_child,
+ c_value_of_root,
+ c_value_of_child,
+ c_type_of_child,
+ c_variable_editable,
+ c_value_of_variable}
,
/* C */
{
- vlang_c,
- c_number_of_children,
- c_name_of_variable,
- c_name_of_child,
- c_value_of_root,
- c_value_of_child,
- c_type_of_child,
- c_variable_editable,
- c_value_of_variable}
+ vlang_c,
+ c_number_of_children,
+ c_name_of_variable,
+ c_name_of_child,
+ c_value_of_root,
+ c_value_of_child,
+ c_type_of_child,
+ c_variable_editable,
+ c_value_of_variable}
,
/* C++ */
{
- vlang_cplus,
- cplus_number_of_children,
- cplus_name_of_variable,
- cplus_name_of_child,
- cplus_value_of_root,
- cplus_value_of_child,
- cplus_type_of_child,
- cplus_variable_editable,
- cplus_value_of_variable}
+ vlang_cplus,
+ cplus_number_of_children,
+ cplus_name_of_variable,
+ cplus_name_of_child,
+ cplus_value_of_root,
+ cplus_value_of_child,
+ cplus_type_of_child,
+ cplus_variable_editable,
+ cplus_value_of_variable}
,
/* Java */
{
- vlang_java,
- java_number_of_children,
- java_name_of_variable,
- java_name_of_child,
- java_value_of_root,
- java_value_of_child,
- java_type_of_child,
- java_variable_editable,
- java_value_of_variable}
+ vlang_java,
+ java_number_of_children,
+ java_name_of_variable,
+ java_name_of_child,
+ java_value_of_root,
+ java_value_of_child,
+ java_type_of_child,
+ java_variable_editable,
+ java_value_of_variable}
};
/* A little convenience enum for dealing with C++/Java */
enum vsections
-{
- v_public = 0, v_private, v_protected
-};
+ {
+ v_public = 0, v_private, v_protected
+ };
/* Private data */
@@ -456,7 +457,7 @@
Since select_frame is so benign, just call it for all cases. */
if (fi != NULL)
{
- var->root->frame = FRAME_FP (fi);
+ get_frame_id (fi, &var->root->frame);
old_fi = selected_frame;
select_frame (fi);
}
@@ -514,13 +515,13 @@
varobj_gen_name (void)
{
static int id = 0;
- char obj_name[31];
+ char *obj_name;
/* generate a name for this object */
id++;
- sprintf (obj_name, "var%d", id);
-
- return xstrdup (obj_name);
+ xasprintf (&obj_name, "var%d", id);
+
+ return obj_name;
}
/* Given an "objname", returns the pointer to the corresponding varobj
@@ -826,9 +827,9 @@
through its children, reconstructing them and noting if they've
changed.
Return value:
- -1 if there was an error updating the varobj
- -2 if the type changed
- Otherwise it is the number of children + parent changed
+ -1 if there was an error updating the varobj
+ -2 if the type changed
+ Otherwise it is the number of children + parent changed
Only root variables can be updated...
@@ -850,7 +851,8 @@
struct value *new;
struct vstack *stack = NULL;
struct vstack *result = NULL;
- struct frame_info *old_fi;
+ struct frame_id old_fid;
+ struct frame *fi;
/* sanity check: have we been passed a pointer? */
if (changelist == NULL)
@@ -863,7 +865,7 @@
/* Save the selected stack frame, since we will need to change it
in order to evaluate expressions. */
- old_fi = selected_frame;
+ get_frame_id (selected_frame, &old_fid);
/* Update the root variable. value_of_root can return NULL
if the variable is no longer around, i.e. we stepped out of
@@ -983,8 +985,10 @@
}
/* Restore selected frame */
- select_frame (old_fi);
-
+ fi = frame_find_by_id (old_fid);
+ if (fi)
+ select_frame (fi);
+
if (type_changed)
return -2;
else
@@ -1214,10 +1218,7 @@
child->error = 1;
child->parent = parent;
child->root = parent->root;
- childs_name =
- (char *) xmalloc ((strlen (parent->obj_name) + strlen (name) + 2) *
- sizeof (char));
- sprintf (childs_name, "%s.%s", parent->obj_name, name);
+ xasprintf (&childs_name, "%s.%s", parent->obj_name, name);
child->obj_name = childs_name;
install_variable (child);
@@ -1306,7 +1307,8 @@
var->root->lang = NULL;
var->root->exp = NULL;
var->root->valid_block = NULL;
- var->root->frame = (CORE_ADDR) -1;
+ var->root->frame.base = (CORE_ADDR) -1;
+ var->root->frame.pc = (CORE_ADDR) -1;
var->root->use_selected_frame = 0;
var->root->rootvar = NULL;
@@ -1794,14 +1796,7 @@
switch (TYPE_CODE (type))
{
case TYPE_CODE_ARRAY:
- {
- /* We never get here unless parent->num_children is greater than 0... */
- int len = 1;
- while ((int) pow ((double) 10, (double) len) < index)
- len++;
- name = (char *) xmalloc (1 + len * sizeof (char));
- sprintf (name, "%d", index);
- }
+ xasprintf (&name, "%d", index);
break;
case TYPE_CODE_STRUCT:
@@ -1820,9 +1815,7 @@
break;
default:
- name =
- (char *) xmalloc ((strlen (parent->name) + 2) * sizeof (char));
- sprintf (name, "*%s", parent->name);
+ xasprintf (&name, "*%s", parent->name);
break;
}
break;
@@ -1855,10 +1848,7 @@
else
{
reinit_frame_cache ();
-
-
- fi = find_frame_addr_in_frame_chain (var->root->frame);
-
+ fi = frame_find_by_id (var->root->frame);
within_scope = fi != NULL;
/* FIXME: select_frame could fail */
if (within_scope)
@@ -2024,33 +2014,26 @@
static char *
c_value_of_variable (struct varobj *var)
{
- struct type *type;
-
/* BOGUS: if val_print sees a struct/class, it will print out its
children instead of "{...}" */
- type = get_type (var);
- switch (TYPE_CODE (type))
+
+ switch (TYPE_CODE (get_type (var)))
{
case TYPE_CODE_STRUCT:
case TYPE_CODE_UNION:
return xstrdup ("{...}");
/* break; */
-
+
case TYPE_CODE_ARRAY:
{
- char number[18];
- sprintf (number, "[%d]", var->num_children);
- return xstrdup (number);
+ char *number;
+ xasprintf (&number, "[%d]", var->num_children);
+ return (number);
}
/* break; */
default:
{
- long dummy;
- struct ui_file *stb = mem_fileopen ();
- struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
- char *thevalue;
-
if (var->value == NULL)
{
/* This can happen if we attempt to get the value of a struct
@@ -2060,6 +2043,11 @@
}
else
{
+ long dummy;
+ struct ui_file *stb = mem_fileopen ();
+ struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+ char *thevalue;
+
if (VALUE_LAZY (var->value))
gdb_value_fetch_lazy (var->value);
val_print (VALUE_TYPE (var->value), VALUE_CONTENTS_RAW (var->value), 0,
@@ -2067,11 +2055,9 @@
stb, format_code[(int) var->format], 1, 0, 0);
thevalue = ui_file_xstrdup (stb, &dummy);
do_cleanups (old_chain);
+ return thevalue;
}
-
- return thevalue;
}
- /* break; */
}
}
\f
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFA] bug fixes for varobj.c
2002-06-10 23:09 [RFA] bug fixes for varobj.c Martin M. Hunt
@ 2002-06-12 19:24 ` Andrew Cagney
2002-07-03 13:30 ` Martin M. Hunt
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cagney @ 2002-06-12 19:24 UTC (permalink / raw)
To: Martin M. Hunt; +Cc: gdb-patches
> Anything using varobj is randomly corrupting memory and will cause crashes in
> Insight or anything using mi varobjs. This patch fixes that and some other
> minor problems.
Hmm, I'm not seeing MI test failures. Would you, by chance, have
something reproduceable for an MI testcase?
Anyway, separating out the changes:
I consider the sprintf() -> xasprintf() transformations:
> (varobj_gen_name): Use xasprintf.
> (create_child): Use xasprintf.
to be ``obvious'' and can, separatly, go straight in (only ~300 other
calls to go ...).
The frame_id stuff:
> /* Save the selected stack frame, since we will need to change it
> in order to evaluate expressions. */
> - old_fi = selected_frame;
> + get_frame_id (selected_frame, &old_fid);
is fine except I'm not sure about:
> - var->root->frame = (CORE_ADDR) -1;
> + var->root->frame.base = (CORE_ADDR) -1;
> + var->root->frame.pc = (CORE_ADDR) -1;
The function:
frame_find_by_id (struct frame_id id)
has:
/* ZERO denotes the null frame, let the caller decide what to do
about it. Should it instead return get_current_frame()? */
if (id.base == 0 && id.pc == 0)
return NULL;
(see find_frame_addr_in_frame_chain for where this came from) so I think
zero would be better.
For the indentation changes, the way to do this is to (separate patch /
commit) run the file through gdb_indent.sh. BTW, the script would not
re-indent:
-{
- v_public = 0, v_private, v_protected
-};
+ {
+ v_public = 0, v_private, v_protected
+ };
Andrew
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFA] bug fixes for varobj.c
2002-06-12 19:24 ` Andrew Cagney
@ 2002-07-03 13:30 ` Martin M. Hunt
0 siblings, 0 replies; 3+ messages in thread
From: Martin M. Hunt @ 2002-07-03 13:30 UTC (permalink / raw)
To: Andrew Cagney; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]
I have fixed the patch and checked it in. Final version is attached.
On Wednesday 12 June 2002 07:24 pm, Andrew Cagney wrote:
> > Anything using varobj is randomly corrupting memory and will cause
> > crashes in Insight or anything using mi varobjs. This patch fixes that
> > and some other minor problems.
>
> Hmm, I'm not seeing MI test failures. Would you, by chance, have
> something reproduceable for an MI testcase?
Memory corruption bugs are not easy to reproduce. I never even had a
reproduceable test case for Insight. The best I could do was a sequence of a
dozen or so operations that crashed most of the time. I just used valgrind
and tracked down all the offending code.
> Anyway, separating out the changes:
>
> I consider the sprintf() -> xasprintf() transformations:
> > (varobj_gen_name): Use xasprintf.
> > (create_child): Use xasprintf.
>
> to be ``obvious'' and can, separatly, go straight in (only ~300 other
> calls to go ...).
OK.
> The frame_id stuff:
> > /* Save the selected stack frame, since we will need to change it
> > in order to evaluate expressions. */
> > - old_fi = selected_frame;
> > + get_frame_id (selected_frame, &old_fid);
>
> is fine except I'm not sure about:
> > - var->root->frame = (CORE_ADDR) -1;
> > + var->root->frame.base = (CORE_ADDR) -1;
> > + var->root->frame.pc = (CORE_ADDR) -1;
>
> The function:
>
> frame_find_by_id (struct frame_id id)
>
> has:
>
> /* ZERO denotes the null frame, let the caller decide what to do
> about it. Should it instead return get_current_frame()? */
> if (id.base == 0 && id.pc == 0)
> return NULL;
>
> (see find_frame_addr_in_frame_chain for where this came from) so I think
> zero would be better.
Agreed. Changed in the patch.
>
> For the indentation changes,
They were accidentally included and are removed from the patch.
--
Martin Hunt
GDB Engineer
Red Hat, Inc.
[-- Attachment #2: p --]
[-- Type: text/x-diff, Size: 5229 bytes --]
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.29
diff -u -u -b -r1.29 varobj.c
--- varobj.c 15 Jun 2002 18:45:31 -0000 1.29
+++ varobj.c 3 Jul 2002 20:12:26 -0000
@@ -52,7 +52,7 @@
struct block *valid_block;
/* The frame for this expression */
- CORE_ADDR frame;
+ struct frame_id frame;
/* If 1, "update" always recomputes the frame & valid block
using the currently selected frame. */
@@ -456,7 +456,7 @@
Since select_frame is so benign, just call it for all cases. */
if (fi != NULL)
{
- var->root->frame = FRAME_FP (fi);
+ get_frame_id (fi, &var->root->frame);
old_fi = selected_frame;
select_frame (fi);
}
@@ -514,13 +514,13 @@
varobj_gen_name (void)
{
static int id = 0;
- char obj_name[31];
+ char *obj_name;
/* generate a name for this object */
id++;
- sprintf (obj_name, "var%d", id);
+ xasprintf (&obj_name, "var%d", id);
- return xstrdup (obj_name);
+ return obj_name;
}
/* Given an "objname", returns the pointer to the corresponding varobj
@@ -850,7 +850,8 @@
struct value *new;
struct vstack *stack = NULL;
struct vstack *result = NULL;
- struct frame_info *old_fi;
+ struct frame_id old_fid;
+ struct frame_info *fi;
/* sanity check: have we been passed a pointer? */
if (changelist == NULL)
@@ -863,7 +864,7 @@
/* Save the selected stack frame, since we will need to change it
in order to evaluate expressions. */
- old_fi = selected_frame;
+ get_frame_id (selected_frame, &old_fid);
/* Update the root variable. value_of_root can return NULL
if the variable is no longer around, i.e. we stepped out of
@@ -983,7 +984,9 @@
}
/* Restore selected frame */
- select_frame (old_fi);
+ fi = frame_find_by_id (old_fid);
+ if (fi)
+ select_frame (fi);
if (type_changed)
return -2;
@@ -1214,10 +1217,7 @@
child->error = 1;
child->parent = parent;
child->root = parent->root;
- childs_name =
- (char *) xmalloc ((strlen (parent->obj_name) + strlen (name) + 2) *
- sizeof (char));
- sprintf (childs_name, "%s.%s", parent->obj_name, name);
+ xasprintf (&childs_name, "%s.%s", parent->obj_name, name);
child->obj_name = childs_name;
install_variable (child);
@@ -1306,7 +1306,8 @@
var->root->lang = NULL;
var->root->exp = NULL;
var->root->valid_block = NULL;
- var->root->frame = (CORE_ADDR) -1;
+ var->root->frame.base = 0;
+ var->root->frame.pc = 0;
var->root->use_selected_frame = 0;
var->root->rootvar = NULL;
@@ -1794,14 +1795,7 @@
switch (TYPE_CODE (type))
{
case TYPE_CODE_ARRAY:
- {
- /* We never get here unless parent->num_children is greater than 0... */
- int len = 1;
- while ((int) pow ((double) 10, (double) len) < index)
- len++;
- name = (char *) xmalloc (1 + len * sizeof (char));
- sprintf (name, "%d", index);
- }
+ xasprintf (&name, "%d", index);
break;
case TYPE_CODE_STRUCT:
@@ -1820,9 +1814,7 @@
break;
default:
- name =
- (char *) xmalloc ((strlen (parent->name) + 2) * sizeof (char));
- sprintf (name, "*%s", parent->name);
+ xasprintf (&name, "*%s", parent->name);
break;
}
break;
@@ -1855,10 +1847,7 @@
else
{
reinit_frame_cache ();
-
-
- fi = find_frame_addr_in_frame_chain (var->root->frame);
-
+ fi = frame_find_by_id (var->root->frame);
within_scope = fi != NULL;
/* FIXME: select_frame could fail */
if (within_scope)
@@ -2026,12 +2015,10 @@
static char *
c_value_of_variable (struct varobj *var)
{
- struct type *type;
-
/* BOGUS: if val_print sees a struct/class, it will print out its
children instead of "{...}" */
- type = get_type (var);
- switch (TYPE_CODE (type))
+
+ switch (TYPE_CODE (get_type (var)))
{
case TYPE_CODE_STRUCT:
case TYPE_CODE_UNION:
@@ -2040,19 +2027,14 @@
case TYPE_CODE_ARRAY:
{
- char number[18];
- sprintf (number, "[%d]", var->num_children);
- return xstrdup (number);
+ char *number;
+ xasprintf (&number, "[%d]", var->num_children);
+ return (number);
}
/* break; */
default:
{
- long dummy;
- struct ui_file *stb = mem_fileopen ();
- struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
- char *thevalue;
-
if (var->value == NULL)
{
/* This can happen if we attempt to get the value of a struct
@@ -2062,6 +2044,11 @@
}
else
{
+ long dummy;
+ struct ui_file *stb = mem_fileopen ();
+ struct cleanup *old_chain = make_cleanup_ui_file_delete (stb);
+ char *thevalue;
+
if (VALUE_LAZY (var->value))
gdb_value_fetch_lazy (var->value);
val_print (VALUE_TYPE (var->value),
@@ -2070,11 +2057,9 @@
format_code[(int) var->format], 1, 0, 0);
thevalue = ui_file_xstrdup (stb, &dummy);
do_cleanups (old_chain);
- }
-
return thevalue;
}
- /* break; */
+ }
}
}
\f
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2002-07-03 20:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2002-06-10 23:09 [RFA] bug fixes for varobj.c Martin M. Hunt
2002-06-12 19:24 ` Andrew Cagney
2002-07-03 13:30 ` Martin M. Hunt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox