* [PATCH] keeping convenience variables (take 2)
@ 2005-11-21 22:05 Andrew STUBBS
2005-11-22 4:58 ` Eli Zaretskii
2005-11-22 8:43 ` Jim Blandy
0 siblings, 2 replies; 32+ messages in thread
From: Andrew STUBBS @ 2005-11-21 22:05 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]
Hi all,
Attached is a new attempt at keeping convenience variables over
symbol-file commands.
This time the 'keep-variable' command has been ditched in favour of
always attempting to keep all variables.
Additionally, it no longer throws away the value if it cannot find a
suitable type. Instead it keeps the value stored away and reinstates it
if a suitable type reappears at some point in the future. Note that here
'suitable' means 'has the same name' - it does not try to check if the
types are in any way compatible.
While the type does not exist, the value is 'void'. 'show convenience'
will annotate this value with '<type 'whatever' unknown>'. I would
prefer that this were the case when the value is printed with 'print'
also, but that would require changing all language val_print functions
(???) and need some sort of scheme for passing the information to those
functions (they only see the type and the raw contents, not the struct
value).
If this sort of annotation is appropriate I would do something similar
for endian-challenged variables.
When it does not find a type, parse_expression() prints a diagnostic
message. If possible I would like to suppress this, but I am not sure
what the best approach to this might be. Any suggestions?
Note that this patch is intended to be applied on top of my endian fixup
patch posted recently.
Thanks
Andrew Stubbs
[-- Attachment #2: keepvariable-2.patch --]
[-- Type: text/plain, Size: 14757 bytes --]
2005-11-21 Andrew Stubbs <andrew.stubbs@st.com>
* value.c: Include exceptions.h.
(fixup_internalvar_endian): Don't fix the endian if the type
needs fixing.
(fixup_internalvar_type_info): New function.
(lookup_internalvar): Initialise new 'saved_value' and
'type_needs_fixing' fields.
(value_of_internalvar): Call fixup_internalvar_type_info() if needed.
(set_internalvar_component): Likewise.
(set_internalvar): Reset type_needs_fixing and saved_value.
(get_name_from_type): New function.
(clear_internalvars): Rename to ...
(save_internalvars): ... this and rewrite to keep type information.
(show_convenience): Call fixup_internalvar_type_info() if needed.
* exec.c (exec_file_attach). Call save_internalvars().
* symfile.c (symbol_file_command). Call save_internalvars().
(clear_symtab_users): Change clear_internalvars to save_internalvars.
* value.h (struct internalvar): Add 'type_needs_fixing', 'type_name',
'enclosing_type_name', and 'saved_value' fields.
(clear_internalvars): Rename to ...
(save_internalvars): ... this.
* Makefile.in (value.o): Add dependency on exceptions.h.
doc/
* gdb.texinfo (Convenience variables): Mention preservation of types.
testsuite/
* default.exp (test show convenience): Update.
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c 2005-11-18 17:02:35.000000000 +0000
+++ src/gdb/value.c 2005-11-21 14:36:40.000000000 +0000
@@ -37,6 +37,7 @@
#include "gdb_assert.h"
#include "regcache.h"
#include "block.h"
+#include "exceptions.h"
/* Prototypes for exported functions. */
@@ -779,10 +780,12 @@ fixup_internalvar_endian (struct interna
gdb_byte temp;
gdb_byte *array = value_contents_raw (var->value);
- /* Don't do anything if the endian has not changed.
+ /* Don't do anything if the endian has not changed or if the type needs
+ fixing - the endian will be fixed with the type later.
Also disregard FP types because they don't seem to vary with
endian - at least, not on i686 Linux or sparc Solaris. */
if (var->endian != TARGET_BYTE_ORDER
+ && var->type_needs_fixing == 0
&& TYPE_CODE (value_type (var->value)) != TYPE_CODE_FLT)
{
/* Reverse the bytes.
@@ -811,6 +814,65 @@ fixup_all_internalvars_endian (void)
}
+/* If the symbol table is reloaded then many variables will lose
+ their type info. save_internalvars() makes a note of the name of
+ any such types in the variable.
+ This function is called whenever the variable is next required to
+ get the new type info. It also fixes the endianess if required. */
+
+static void
+fixup_internalvar_type_info (struct internalvar *var)
+{
+ int i, error=0;
+
+ /* For each in type and enclosing type ... */
+ for (i=0; i<2 && !error; i++)
+ {
+ char *type_name = i==0 ? var->type_name
+ : var->enclosing_type_name;
+ struct type **var_type = i==0 ? &var->saved_value->type
+ : &var->saved_value->enclosing_type;
+
+ if (type_name != NULL)
+ {
+ /* Get the typename using parse expression - it can cope
+ with builtin types even when there is no symbol table. */
+ struct expression *expr = (struct expression *)
+ catch_errors ((catch_errors_ftype *)parse_expression,
+ type_name, NULL, RETURN_MASK_ALL);
+ register struct cleanup *old_chain =
+ make_cleanup (free_current_contents, &expr);
+
+ if (expr != NULL && expr->nelts >= 2
+ && expr->elts[0].opcode == OP_TYPE)
+ /* Overwite the old (now broken) type with the shiny new one. */
+ *var_type = expr->elts[1].type;
+ else
+ error=1;
+
+ do_cleanups (old_chain);
+ }
+ }
+
+
+ if (error)
+ /* Types no longer exist.
+ Leave it unfixed - the type may come back later. */
+ return;
+
+ /* Return the internal variable to its normal state. */
+ xfree (var->type_name);
+ xfree (var->enclosing_type_name);
+ var->type_needs_fixing = 0;
+ xfree (var->value);
+ var->value = var->saved_value;
+ var->saved_value = NULL;
+
+ /* Fix up the endianess. */
+ fixup_internalvar_endian (var);
+}
+
+
/* Look up an internal variable with name NAME. NAME should not
normally include a dollar sign.
@@ -830,6 +892,8 @@ lookup_internalvar (char *name)
var->name = concat (name, (char *)NULL);
var->value = allocate_value (builtin_type_void);
var->endian = TARGET_BYTE_ORDER;
+ var->type_needs_fixing = 0;
+ var->saved_value = NULL;
release_value (var->value);
var->next = internalvars;
internalvars = var;
@@ -841,6 +905,9 @@ value_of_internalvar (struct internalvar
{
struct value *val;
+ if (var->type_needs_fixing)
+ fixup_internalvar_type_info (var);
+
val = value_copy (var->value);
if (value_lazy (val))
value_fetch_lazy (val);
@@ -853,7 +920,12 @@ void
set_internalvar_component (struct internalvar *var, int offset, int bitpos,
int bitsize, struct value *newval)
{
- gdb_byte *addr = value_contents_writeable (var->value) + offset;
+ gdb_byte *addr;
+
+ if (var->type_needs_fixing)
+ fixup_internalvar_type_info (var);
+
+ addr = value_contents_writeable (var->value) + offset;
if (bitsize)
modify_field (addr, value_as_long (newval),
@@ -885,6 +957,12 @@ set_internalvar (struct internalvar *var
xfree (var->value);
var->value = newval;
var->endian = TARGET_BYTE_ORDER;
+ var->type_needs_fixing = 0;
+
+ /* If there was a saved value then it is no longer valid. */
+ xfree (var->saved_value);
+ var->saved_value = NULL;
+
release_value (newval);
/* End code which must not call error(). */
}
@@ -895,21 +973,114 @@ internalvar_name (struct internalvar *va
return var->name;
}
-/* Free all internalvars. Done when new symtabs are loaded,
- because that makes the values invalid. */
+
+/* Get the typename from a struct type.
+ Returns the name of type as parse_expresion will understand.
+ Returns NULL if the type is unsupported. */
+
+static char *
+get_name_from_type (const struct type *type)
+{
+ char *result;
+ char *prefix = NULL, pointers=0;
+
+ /* Follow the pointer types until the base type is discovered. */
+ while (TYPE_CODE (type) == TYPE_CODE_PTR)
+ {
+ pointers++;
+ type = TYPE_TARGET_TYPE (type);
+ }
+
+ /* Find the appropriate string for the type.
+ If we can't find one then return NULL and fixup_internalvar_type_info()
+ will replace the variable with void. If this is a problem then this
+ table will have to be extended to support the relavent type. */
+ switch (TYPE_CODE (type))
+ {
+ case TYPE_CODE_STRUCT:
+ prefix = "struct";
+ /* Fall through. */
+ case TYPE_CODE_UNION:
+ if (prefix == NULL)
+ prefix = "union";
+ /* Fall through. */
+ case TYPE_CODE_ENUM:
+ if (prefix == NULL)
+ prefix = "enum";
+
+ /* Note that these types do not have names, but do have tag names. */
+
+ if (TYPE_TAG_NAME (type) == NULL)
+ return NULL;
+
+ result = xmalloc (strlen (prefix)
+ + 1 /* space */
+ + strlen (TYPE_TAG_NAME (type))
+ + 1 /* space */
+ + pointers
+ + 1 /* terminator */);
+ sprintf (result, "%s %s ", prefix, TYPE_TAG_NAME(type));
+
+ break;
+ default:
+ /* Other types all (?) have names. */
+
+ if (TYPE_NAME (type) == NULL)
+ return NULL;
+
+ result = xmalloc (strlen (TYPE_NAME (type))
+ + 1 /* space */
+ + pointers
+ + 1 /* terminator */);
+ sprintf (result, "%s ", TYPE_NAME (type));
+ }
+
+ /* Add stars for pointer types. */
+ while (pointers-- > 0)
+ strcat (result, "*");
+
+ return result;
+}
+
+
+/* Traverse list of intervalvars and save the types for later.
+
+ All variables whose types do not belong to an object file are left alone.
+
+ Otherwise steps must be taken to survive the loss of the type information.
+
+ Therefore take a note of the textual names. The types will then
+ be patched up later when they are referenced. The function that
+ does this is fixup_internalvars_type_info().
+
+ This function must be called BEFORE the type information is deleted. */
void
-clear_internalvars (void)
+save_internalvars (void)
{
struct internalvar *var;
- while (internalvars)
+ for (var = internalvars; var; var = var->next)
{
- var = internalvars;
- internalvars = var->next;
- xfree (var->name);
- xfree (var->value);
- xfree (var);
+ /* Check if this symbol's type is from an object file.
+ Don't do it twice - the lazy fixup means it may not have been
+ fixed since the last time we were here. */
+ if (var->type_needs_fixing == 0
+ && TYPE_OBJFILE (value_enclosing_type (var->value)))
+ {
+ /* Note down the values of the types.
+ This ensures that they can be fixed up after the symbol
+ tables are rebuilt. */
+ var->type_needs_fixing = 1;
+ var->type_name = get_name_from_type (value_type (var->value));
+ var->enclosing_type_name =
+ get_name_from_type (value_enclosing_type (var->value));
+ var->saved_value = var->value;
+
+ /* Replace the value with void until it is fixed. */
+ var->value = allocate_value (builtin_type_void);
+ release_value (var->value);
+ }
}
}
@@ -921,12 +1092,17 @@ show_convenience (char *ignore, int from
for (var = internalvars; var; var = var->next)
{
+ if (var->type_needs_fixing)
+ fixup_internalvar_type_info (var);
+
if (!varseen)
{
varseen = 1;
}
printf_filtered (("$%s = "), var->name);
value_print (var->value, gdb_stdout, 0, Val_pretty_default);
+ if (var->type_needs_fixing)
+ printf_filtered (_(" <type '%s' unknown>"), var->enclosing_type_name);
printf_filtered (("\n"));
}
if (!varseen)
Index: src/gdb/exec.c
===================================================================
--- src.orig/gdb/exec.c 2005-11-18 16:59:35.000000000 +0000
+++ src/gdb/exec.c 2005-11-18 17:33:47.000000000 +0000
@@ -266,6 +266,11 @@ exec_file_attach (char *filename, int fr
validate_files ();
+ /* Save the convenience variables BEFORE their types become invalid.
+ A note can be made of what the types of the variables were and
+ they can be recovered later. */
+ save_internalvars ();
+
set_gdbarch_from_file (exec_bfd);
push_target (&exec_ops);
Index: src/gdb/symfile.c
===================================================================
--- src.orig/gdb/symfile.c 2005-11-18 16:59:35.000000000 +0000
+++ src/gdb/symfile.c 2005-11-18 17:47:07.000000000 +0000
@@ -1268,6 +1268,11 @@ symbol_file_command (char *args, int fro
{
dont_repeat ();
+ /* Save the convenience variables BEFORE their types become invalid.
+ A note can be made of what the types of the variables were and
+ they can be recovered later. */
+ save_internalvars ();
+
if (args == NULL)
{
symbol_file_clear (from_tty);
@@ -2484,7 +2489,7 @@ clear_symtab_users (void)
clear_value_history ();
clear_displays ();
- clear_internalvars ();
+ save_internalvars ();
breakpoint_re_set ();
set_default_breakpoint (0, 0, 0, 0);
clear_pc_function_cache ();
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h 2005-11-18 17:02:35.000000000 +0000
+++ src/gdb/value.h 2005-11-18 18:18:48.000000000 +0000
@@ -246,6 +246,13 @@ struct internalvar
char *name;
struct value *value;
int endian;
+
+ /* These are used to fixup the type pointers after the symbol tables
+ are reloaded. */
+ char type_needs_fixing;
+ char *type_name;
+ char *enclosing_type_name;
+ struct value *saved_value;
};
\f
@@ -507,7 +514,7 @@ extern char *internalvar_name (struct in
extern void clear_value_history (void);
-extern void clear_internalvars (void);
+extern void save_internalvars (void);
/* From values.c */
Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in 2005-11-18 17:02:35.000000000 +0000
+++ src/gdb/Makefile.in 2005-11-18 17:02:36.000000000 +0000
@@ -2735,7 +2735,7 @@ valprint.o: valprint.c $(defs_h) $(gdb_s
value.o: value.c $(defs_h) $(gdb_string_h) $(symtab_h) $(gdbtypes_h) \
$(value_h) $(gdbcore_h) $(command_h) $(gdbcmd_h) $(target_h) \
$(language_h) $(scm_lang_h) $(demangle_h) $(doublest_h) \
- $(gdb_assert_h) $(regcache_h) $(block_h)
+ $(gdb_assert_h) $(regcache_h) $(block_h) $(exceptions_h)
varobj.o: varobj.c $(defs_h) $(value_h) $(expression_h) $(frame_h) \
$(language_h) $(wrapper_h) $(gdbcmd_h) $(gdb_string_h) $(varobj_h)
vaxbsd-nat.o: vaxbsd-nat.c $(defs_h) $(inferior_h) $(regcache_h) $(target_h) \
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo 2005-11-18 17:02:34.000000000 +0000
+++ src/gdb/doc/gdb.texinfo 2005-11-21 14:39:00.000000000 +0000
@@ -6125,6 +6125,15 @@ variable any type of value, including st
that variable already has a value of a different type. The convenience
variable, when used as an expression, has the type of its current value.
+@value{GDBN} commands that wipe the symbol table, such as @samp{file} and
+@samp{symbol-file}, cause problems for convenience variables---their types
+may be lost so their values may become meaningless. @value{GDBN} tries to
+avoid this by selecting a type from the new symbol table (if any). If a
+suitable type does not exist (at the time the variable is accessed) then
+@value{GDBN} will show the value as @code{void} until the type becomes
+available once more. The @samp{show convenience} command will tag these
+variables with @samp{<type '...' unknown>}.
+
@table @code
@kindex show convenience
@cindex show all user variables
Index: src/gdb/testsuite/gdb.base/default.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/default.exp 2005-07-14 15:49:23.000000000 +0100
+++ src/gdb/testsuite/gdb.base/default.exp 2005-11-21 14:00:50.000000000 +0000
@@ -602,7 +602,7 @@ gdb_test "show complaints" "Max number o
#test show confirm
gdb_test "show confirm" "Whether to confirm potentially dangerous operations is o\[a-z\]*." "show confirm"
#test show convenience
-gdb_test "show convenience" "No debugger convenience variables now defined.(\[^\r\n\]*\[\r\n\])+Convenience variables have names starting with \".\";(\[^\r\n\]*\[\r\n\])+use \"set\" as in \"set .foo = 5\" to define them." "show convenience"
+gdb_test "show convenience" ".trace_frame = -1(\[^\r\n\]*\[\r\n\])+.tpnum = 0(\[^\r\n\]*\[\r\n\])+" "show convenience"
#test show directories
gdb_test "show directories" "Source directories searched: .cdir:.cwd" "show directories"
#test show editing
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] keeping convenience variables (take 2) 2005-11-21 22:05 [PATCH] keeping convenience variables (take 2) Andrew STUBBS @ 2005-11-22 4:58 ` Eli Zaretskii 2005-11-22 8:43 ` Jim Blandy 1 sibling, 0 replies; 32+ messages in thread From: Eli Zaretskii @ 2005-11-22 4:58 UTC (permalink / raw) To: Andrew STUBBS; +Cc: gdb-patches > Date: Mon, 21 Nov 2005 14:40:53 +0000 > From: Andrew STUBBS <andrew.stubbs@st.com> > > +@value{GDBN} commands that wipe the symbol table, such as @samp{file} and > +@samp{symbol-file}, cause problems for convenience variables---their types > +may be lost so their values may become meaningless. @value{GDBN} tries to > +avoid this by selecting a type from the new symbol table (if any). If a > +suitable type does not exist (at the time the variable is accessed) then > +@value{GDBN} will show the value as @code{void} until the type becomes > +available once more. The @samp{show convenience} command will tag these > +variables with @samp{<type '...' unknown>}. ^^^ Please use @dots{} here instead of a literal "..." (the former produces better results in print). Also, please give an example of a variable whose definition makes it dependant on the symbol table. Since only that kind of convenience variables is prone to this problem, I think it's important that we show the reader when such a problem can happen. Other than that, this part is approved. Thanks. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] keeping convenience variables (take 2) 2005-11-21 22:05 [PATCH] keeping convenience variables (take 2) Andrew STUBBS 2005-11-22 4:58 ` Eli Zaretskii @ 2005-11-22 8:43 ` Jim Blandy 2005-11-22 19:27 ` Andrew STUBBS 1 sibling, 1 reply; 32+ messages in thread From: Jim Blandy @ 2005-11-22 8:43 UTC (permalink / raw) To: Andrew STUBBS; +Cc: gdb-patches I may be missing something, be patient: If I understand the original motivation for this, you don't actually need to keep around any values that aren't using the builtin types. Or at least, that could be using the built-in types; it wasn't clear whether they actually did. I'm uncomfortable with this whole "print type name and then re-parse later" approach. I think it's a real kludge. And the fact that it doesn't seem really necessary for the problem that originally motivated the change is a red flag, it seems to me. I can certainly see that it's useful to keep the convenience variables around across symfile selections and endianness switches; I just think we should find a better way to handle the types than this. If the convenience variables you set up in your script, holding addresses and such, don't refer to the builtin types, why don't they? Could they? I understand that changing architectures or architectural variants affects the builtin types, but it seems more feasible to track those changes than to try to track objfile-based types as they disappear and reappear. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] keeping convenience variables (take 2) 2005-11-22 8:43 ` Jim Blandy @ 2005-11-22 19:27 ` Andrew STUBBS 2005-12-10 4:46 ` [RFC] Alternate approach to keeping convenience variables Daniel Jacobowitz 0 siblings, 1 reply; 32+ messages in thread From: Andrew STUBBS @ 2005-11-22 19:27 UTC (permalink / raw) To: Jim Blandy; +Cc: gdb-patches Jim Blandy wrote: > I may be missing something, be patient: > > If I understand the original motivation for this, you don't actually > need to keep around any values that aren't using the builtin types. > Or at least, that could be using the built-in types; it wasn't clear > whether they actually did. Yes, most likely they are all builtin types, but when I first did the work (in 5.3) I found that it could not be assumed that the builtin types would stay in the same place. I do not remember under what circumstances I observed this. So I fixed it and the result was that we got 'textual compatibility' of all other types for free. Now it does appear that builtin types can be safely left alone - there have been many changes to the code in this area. > I'm uncomfortable with this whole "print type name and then re-parse > later" approach. I think it's a real kludge. And the fact that it > doesn't seem really necessary for the problem that originally > motivated the change is a red flag, it seems to me. You may be right - I probably don't _need_ it. However, it does work - it provides this 'textual compatibility' very effectively. A better way to print the type name might be nice though. Your point about it not being necessary might be a good reason not to do something in the first place, but it is not, in itself, a good reason to disregard something that has _already_ been done (mostly anyway). This is especially true when what has been done is 'nice to have'. > I can certainly see that it's useful to keep the convenience variables > around across symfile selections and endianness switches; I just think > we should find a better way to handle the types than this. Well, we could recursively copy the entire type list for type and enclosing_type. This would certainly be enough to print the value and to use it in some expressions. I can imagine that there might be some issues because these types would exist outside of the normal type table and/or might clash with types in that table. However, this would require intimate knowledge of the value and type structures and everything they reference. Also, it would be impossible to change the value, even though the existing contents could be read, because parse_expression() wouldn't see the types. It would need a flag in the internalvar to say it would need freeing if the value changes to something else. It might then be possible to match a copied type up against new types at some point (still with all the problems of context etc.), but this does not seem pretty - not least because a lot of pointer types are generated on the fly (something parse_expression does for us in my technique). If anybody has any other suggestions or insight on these matters then that will be welcome. > If the convenience variables you set up in your script, holding > addresses and such, don't refer to the builtin types, why don't they? > Could they? I understand that changing architectures or > architectural variants affects the builtin types, but it seems more > feasible to track those changes than to try to track objfile-based > types as they disappear and reappear. Another question is 'why wouldn't they?' I make no attempt the track changes across architectural variants (other than endian) or indeed from one architecture to another. Doing that would require actual comparison of the before and after types and a type conversion of some kind. It seems much harder to me than just extracting type information (in whatever form) and keeping it or reinstating it later. Perhaps I misunderstand your point. If I understand you, I think your problem is that the conversion process is a) incomplete with respect to non-C languages; b) prone to mistakes with commonly used type-names; and c) different to how it was before. I think the first two can be fixed by means of a better name capturing technique (GDB can print this to screen, but is there a way to capture it in a string?) and by comparing more than just the name (size of base type, number of fields, perhaps some kind of checksum???). Andrew Stubbs ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC] Alternate approach to keeping convenience variables 2005-11-22 19:27 ` Andrew STUBBS @ 2005-12-10 4:46 ` Daniel Jacobowitz 2005-12-10 5:07 ` Jim Blandy ` (3 more replies) 0 siblings, 4 replies; 32+ messages in thread From: Daniel Jacobowitz @ 2005-12-10 4:46 UTC (permalink / raw) To: Andrew STUBBS; +Cc: Jim Blandy, gdb-patches On Tue, Nov 22, 2005 at 05:10:44PM +0000, Andrew STUBBS wrote: > Well, we could recursively copy the entire type list for type and > enclosing_type. This would certainly be enough to print the value and to > use it in some expressions. I can imagine that there might be some > issues because these types would exist outside of the normal type table > and/or might clash with types in that table. > > However, this would require intimate knowledge of the value and type > structures and everything they reference. Also, it would be impossible > to change the value, even though the existing contents could be read, > because parse_expression() wouldn't see the types. Sure it would - you could still only assign to internal variables things that you could write in the expression evaluator, so if it's been reloaded from an objfile, then the name will be in scope again. > It would need a flag in the internalvar to say it would need freeing if > the value changes to something else. Nah, we can leak this memory. It's part of the value history, more or less. I believe I'd earlier suggested this approach. Here's an implementation of it; all comments welcome. It appears to work. I can now print "object_files", discard GDB's symbol table, and still print out its type - which recursively descends as far as the bfd xvec. As a free bonus it should cut down on the watchpoint segfaults I mentioned on gdb@ this afternoon, though they still need a separate resolution. I needed to fix a bug in dwarf2read where some unused pointers would contain garbage. The tracepoint.c change is necessary for default.exp. To see why, try this: $ gdb (gdb) show convenience $trace_frame = -1 $tpnum = 0 vs. $ gdb /bin/ls (gdb) show convenience No debugger convenience variables now defined. Convenience variables have names starting with "$"; use "set" as in "set $foo = 5" to define them. If you want the inconsistency resolve the other way round, please holler. How does this look? -- Daniel Jacobowitz CodeSourcery, LLC 2005-12-09 Daniel Jacobowitz <dan@codesourcery.com> * Makefile.in (gdbtypes_h, gdbtypes.o, utils.o): Update. * defs.h (hashtab_obstack_allocate, dummy_obstack_deallocate): Add prototypes. * dwarf2read.c (read_subroutine_type): Use TYPE_ZALLOC. (hashtab_obstack_allocate, dummy_obstack_deallocate): Moved to... * utils.c (hashtab_obstack_allocate, dummy_obstack_deallocate): ...here. * gdbtypes.c: Include "hashtab.h". (build_gdbtypes): Remove extra prototype. (struct type_pair, type_pair_hash, type_pair_eq) (create_copied_types_hash, copy_type_recursive): New. * gdbtypes.h: Include "hashtab.h". (TYPE_ZALLOC): New. (create_copied_types_hash, copy_type_recursive): New prototypes. * objfiles.c (free_objfile): Call preserve_values. * symfile.c (reread_symbols): Likewise. (clear_symtab_users): Remove calls to clear_value_history and clear_internalvars. * value.c (clear_value_history, clear_internalvars): Removed. (preserve_one_value, preserve_values): New functions. * value.h (clear_value_history, clear_internalvars): Removed. (preserve_values): New prototype. * tracepoint.c (_initialize_tracepoint): Do not initialize convenience variables here. Index: src/gdb/Makefile.in =================================================================== --- src.orig/gdb/Makefile.in 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/Makefile.in 2005-12-09 15:19:49.000000000 -0500 @@ -696,7 +696,7 @@ gdb_stat_h = gdb_stat.h gdb_string_h = gdb_string.h gdb_thread_db_h = gdb_thread_db.h gdbthread_h = gdbthread.h $(breakpoint_h) $(frame_h) -gdbtypes_h = gdbtypes.h +gdbtypes_h = gdbtypes.h $(hashtab_h) gdb_vfork_h = gdb_vfork.h gdb_wait_h = gdb_wait.h glibc_tdep_h = glibc-tdep.h @@ -1979,7 +1979,7 @@ gdb-events.o: gdb-events.c $(defs_h) $(g gdbtypes.o: gdbtypes.c $(defs_h) $(gdb_string_h) $(bfd_h) $(symtab_h) \ $(symfile_h) $(objfiles_h) $(gdbtypes_h) $(expression_h) \ $(language_h) $(target_h) $(value_h) $(demangle_h) $(complaints_h) \ - $(gdbcmd_h) $(wrapper_h) $(cp_abi_h) $(gdb_assert_h) + $(gdbcmd_h) $(wrapper_h) $(cp_abi_h) $(gdb_assert_h) $(hashtab_h) glibc-tdep.o: glibc-tdep.c $(defs_h) $(frame_h) $(symtab_h) $(symfile_h) \ $(objfiles_h) $(glibc_tdep_h) gnu-nat.o: gnu-nat.c $(gdb_string_h) $(defs_h) $(inferior_h) $(symtab_h) \ @@ -2714,7 +2714,7 @@ utils.o: utils.c $(defs_h) $(gdb_assert_ $(exceptions_h) $(tui_h) $(gdbcmd_h) $(serial_h) $(bfd_h) \ $(target_h) $(demangle_h) $(expression_h) $(language_h) $(charset_h) \ $(annotate_h) $(filenames_h) $(symfile_h) $(inferior_h) \ - $(gdb_curses_h) $(readline_h) + $(gdb_curses_h) $(readline_h) $(gdb_obstack_h) uw-thread.o: uw-thread.c $(defs_h) $(gdbthread_h) $(target_h) $(inferior_h) \ $(regcache_h) $(gregset_h) v850-tdep.o: v850-tdep.c $(defs_h) $(frame_h) $(frame_base_h) $(trad_frame_h) \ Index: src/gdb/defs.h =================================================================== --- src.orig/gdb/defs.h 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/defs.h 2005-12-09 15:19:49.000000000 -0500 @@ -1217,4 +1217,9 @@ extern int use_windows; extern ULONGEST align_up (ULONGEST v, int n); extern ULONGEST align_down (ULONGEST v, int n); +/* Allocation and deallocation functions for the libiberty hash table + which use obstacks. */ +void *hashtab_obstack_allocate (void *data, size_t size, size_t count); +void dummy_obstack_deallocate (void *object, void *data); + #endif /* #ifndef DEFS_H */ Index: src/gdb/dwarf2read.c =================================================================== --- src.orig/gdb/dwarf2read.c 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/dwarf2read.c 2005-12-09 15:19:49.000000000 -0500 @@ -1026,10 +1026,6 @@ static char *skip_one_die (char *info_pt static void free_stack_comp_unit (void *); -static void *hashtab_obstack_allocate (void *data, size_t size, size_t count); - -static void dummy_obstack_deallocate (void *object, void *data); - static hashval_t partial_die_hash (const void *item); static int partial_die_eq (const void *item_lhs, const void *item_rhs); @@ -4622,7 +4618,7 @@ read_subroutine_type (struct die_info *d /* Allocate storage for parameters and fill them in. */ TYPE_NFIELDS (ftype) = nparams; TYPE_FIELDS (ftype) = (struct field *) - TYPE_ALLOC (ftype, nparams * sizeof (struct field)); + TYPE_ZALLOC (ftype, nparams * sizeof (struct field)); child_die = die->child; while (child_die && child_die->tag) @@ -9617,28 +9613,6 @@ dwarf2_clear_marks (struct dwarf2_per_cu } } -/* Allocation function for the libiberty hash table which uses an - obstack. */ - -static void * -hashtab_obstack_allocate (void *data, size_t size, size_t count) -{ - unsigned int total = size * count; - void *ptr = obstack_alloc ((struct obstack *) data, total); - memset (ptr, 0, total); - return ptr; -} - -/* Trivial deallocation function for the libiberty splay tree and hash - table - don't deallocate anything. Rely on later deletion of the - obstack. */ - -static void -dummy_obstack_deallocate (void *object, void *data) -{ - return; -} - /* Trivial hash function for partial_die_info: the hash value of a DIE is its offset in .debug_info for this objfile. */ Index: src/gdb/gdbtypes.c =================================================================== --- src.orig/gdb/gdbtypes.c 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/gdbtypes.c 2005-12-09 15:19:49.000000000 -0500 @@ -1,6 +1,6 @@ /* Support routines for manipulating internal types for GDB. Copyright 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2001, 2002, 2003, - 2004 Free Software Foundation, Inc. + 2004, 2005 Free Software Foundation, Inc. Contributed by Cygnus Support, using pieces from other GDB modules. This file is part of GDB. @@ -37,6 +37,7 @@ #include "wrapper.h" #include "cp-abi.h" #include "gdb_assert.h" +#include "hashtab.h" /* These variables point to the objects representing the predefined C data types. */ @@ -3111,7 +3112,132 @@ recursive_dump_type (struct type *type, obstack_free (&dont_print_type_obstack, NULL); } -static void build_gdbtypes (void); +struct type_pair +{ + struct type *old, *new; +}; + +static hashval_t +type_pair_hash (const void *item) +{ + const struct type_pair *pair = item; + return htab_hash_pointer (pair->old); +} + +static int +type_pair_eq (const void *item_lhs, const void *item_rhs) +{ + const struct type_pair *lhs = item_lhs, *rhs = item_rhs; + return lhs->old == rhs->old; +} + +htab_t +create_copied_types_hash (struct objfile *objfile) +{ + return htab_create_alloc_ex (1, type_pair_hash, type_pair_eq, + NULL, &objfile->objfile_obstack, + hashtab_obstack_allocate, + dummy_obstack_deallocate); +} + +struct type * +copy_type_recursive (struct objfile *objfile, struct type *type, + htab_t copied_types) +{ + struct type_pair *stored, pair; + void **slot; + struct type *new_type; + + if (TYPE_OBJFILE (type) == NULL) + return type; + + gdb_assert (TYPE_OBJFILE (type) == objfile); + + pair.old = type; + slot = htab_find_slot (copied_types, &pair, INSERT); + if (*slot != NULL) + return ((struct type_pair *) *slot)->new; + + new_type = alloc_type (NULL); + + /* Add the new type to the hash table immediately. */ + stored = xmalloc (sizeof (struct type_pair)); + stored->old = type; + stored->new = new_type; + *slot = stored; + + /* Copy the common fields of types. */ + TYPE_CODE (new_type) = TYPE_CODE (type); + TYPE_ARRAY_UPPER_BOUND_TYPE (new_type) = TYPE_ARRAY_UPPER_BOUND_TYPE (type); + TYPE_ARRAY_LOWER_BOUND_TYPE (new_type) = TYPE_ARRAY_LOWER_BOUND_TYPE (type); + if (TYPE_NAME (type)) + TYPE_NAME (new_type) = xstrdup (TYPE_NAME (type)); + if (TYPE_TAG_NAME (type)) + TYPE_TAG_NAME (new_type) = xstrdup (TYPE_TAG_NAME (type)); + TYPE_FLAGS (new_type) = TYPE_FLAGS (type); + TYPE_VPTR_FIELDNO (new_type) = TYPE_VPTR_FIELDNO (type); + + TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type); + TYPE_LENGTH (new_type) = TYPE_LENGTH (type); + + /* Copy the fields. */ + TYPE_NFIELDS (new_type) = TYPE_NFIELDS (type); + if (TYPE_NFIELDS (type)) + { + int i, nfields; + + nfields = TYPE_NFIELDS (type); + TYPE_FIELDS (new_type) = xmalloc (sizeof (struct field) * nfields); + for (i = 0; i < nfields; i++) + { + TYPE_FIELD_ARTIFICIAL (new_type, i) = TYPE_FIELD_ARTIFICIAL (type, i); + TYPE_FIELD_BITSIZE (new_type, i) = TYPE_FIELD_BITSIZE (type, i); + if (TYPE_FIELD_TYPE (type, i)) + TYPE_FIELD_TYPE (new_type, i) + = copy_type_recursive (objfile, TYPE_FIELD_TYPE (type, i), + copied_types); + if (TYPE_FIELD_NAME (type, i)) + TYPE_FIELD_NAME (new_type, i) = xstrdup (TYPE_FIELD_NAME (type, i)); + if (TYPE_FIELD_STATIC_HAS_ADDR (type, i)) + SET_FIELD_PHYSADDR (TYPE_FIELD (new_type, i), + TYPE_FIELD_STATIC_PHYSADDR (type, i)); + else if (TYPE_FIELD_STATIC (type, i)) + SET_FIELD_PHYSNAME (TYPE_FIELD (new_type, i), + xstrdup (TYPE_FIELD_STATIC_PHYSNAME (type, i))); + else + { + TYPE_FIELD_BITPOS (new_type, i) = TYPE_FIELD_BITPOS (type, i); + TYPE_FIELD_STATIC_KIND (new_type, i) = 0; + } + } + } + + /* Copy pointers to other types. */ + if (TYPE_TARGET_TYPE (type)) + TYPE_TARGET_TYPE (new_type) = copy_type_recursive (objfile, + TYPE_TARGET_TYPE (type), + copied_types); + if (TYPE_VPTR_BASETYPE (type)) + TYPE_VPTR_BASETYPE (new_type) = copy_type_recursive (objfile, + TYPE_VPTR_BASETYPE (type), + copied_types); + /* Maybe copy the type_specific bits. + + NOTE drow/2005-12-09: We do not copy the C++-specific bits like + base classes and methods. There's no fundamental reason why we + can't, but at the moment it is not needed. */ + + if (TYPE_CODE (type) == TYPE_CODE_FLT) + TYPE_FLOATFORMAT (new_type) == TYPE_FLOATFORMAT (type); + else if (TYPE_CODE (type) == TYPE_CODE_STRUCT + || TYPE_CODE (type) == TYPE_CODE_UNION + || TYPE_CODE (type) == TYPE_CODE_TEMPLATE + || TYPE_CODE (type) == TYPE_CODE_NAMESPACE) + INIT_CPLUS_SPECIFIC (new_type); + + return new_type; +} + static void build_gdbtypes (void) { Index: src/gdb/gdbtypes.h =================================================================== --- src.orig/gdb/gdbtypes.h 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/gdbtypes.h 2005-12-09 15:19:49.000000000 -0500 @@ -25,6 +25,8 @@ #if !defined (GDBTYPES_H) #define GDBTYPES_H 1 +#include "hashtab.h" + /* Forward declarations for prototypes. */ struct field; struct block; @@ -1175,6 +1177,12 @@ extern struct type *builtin_type_f_void; ? obstack_alloc (&TYPE_OBJFILE (t) -> objfile_obstack, size) \ : xmalloc (size)) +#define TYPE_ZALLOC(t,size) \ + (TYPE_OBJFILE (t) != NULL \ + ? memset (obstack_alloc (&TYPE_OBJFILE (t)->objfile_obstack, size), \ + 0, size) \ + : xzalloc (size)) + extern struct type *alloc_type (struct objfile *); extern struct type *init_type (enum type_code, int, int, char *, @@ -1364,4 +1372,10 @@ extern int is_integral_type (struct type extern void maintenance_print_type (char *, int); +extern htab_t create_copied_types_hash (struct objfile *objfile); + +extern struct type *copy_type_recursive (struct objfile *objfile, + struct type *type, + htab_t copied_types); + #endif /* GDBTYPES_H */ Index: src/gdb/objfiles.c =================================================================== --- src.orig/gdb/objfiles.c 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/objfiles.c 2005-12-09 15:19:49.000000000 -0500 @@ -392,6 +392,10 @@ free_objfile (struct objfile *objfile) objfile->separate_debug_objfile_backlink->separate_debug_objfile = NULL; } + /* Remove any references to this objfile in the global value + lists. */ + preserve_values (objfile); + /* First do any symbol file specific actions required when we are finished with a particular symbol file. Note that if the objfile is using reusable symbol information (via mmalloc) then each of Index: src/gdb/symfile.c =================================================================== --- src.orig/gdb/symfile.c 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/symfile.c 2005-12-09 15:19:49.000000000 -0500 @@ -2012,6 +2012,10 @@ reread_symbols (void) memcpy (offsets, objfile->section_offsets, SIZEOF_N_SECTION_OFFSETS (num_offsets)); + /* Remove any references to this objfile in the global + value lists. */ + preserve_values (objfile); + /* Nuke all the state that we will re-read. Much of the following code which sets things to NULL really is necessary to tell other parts of GDB that there is nothing currently there. */ @@ -2484,9 +2488,7 @@ clear_symtab_users (void) breakpoint_re_set may try to access the current symtab. */ clear_current_source_symtab_and_line (); - clear_value_history (); clear_displays (); - clear_internalvars (); breakpoint_re_set (); set_default_breakpoint (0, 0, 0, 0); clear_pc_function_cache (); Index: src/gdb/tracepoint.c =================================================================== --- src.orig/gdb/tracepoint.c 2005-11-20 19:30:22.000000000 -0500 +++ src/gdb/tracepoint.c 2005-12-09 15:45:30.000000000 -0500 @@ -2694,11 +2694,6 @@ _initialize_tracepoint (void) traceframe_number = -1; tracepoint_number = -1; - set_internalvar (lookup_internalvar ("tpnum"), - value_from_longest (builtin_type_int, (LONGEST) 0)); - set_internalvar (lookup_internalvar ("trace_frame"), - value_from_longest (builtin_type_int, (LONGEST) - 1)); - if (tracepoint_list.list == NULL) { tracepoint_list.listsize = 128; Index: src/gdb/utils.c =================================================================== --- src.orig/gdb/utils.c 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/utils.c 2005-12-09 15:19:49.000000000 -0500 @@ -53,6 +53,7 @@ #include "annotate.h" #include "filenames.h" #include "symfile.h" +#include "gdb_obstack.h" #include "inferior.h" /* for signed_pointer_to_address */ @@ -3124,3 +3125,25 @@ align_down (ULONGEST v, int n) gdb_assert (n && (n & (n-1)) == 0); return (v & -n); } + +/* Allocation function for the libiberty hash table which uses an + obstack. */ + +void * +hashtab_obstack_allocate (void *data, size_t size, size_t count) +{ + unsigned int total = size * count; + void *ptr = obstack_alloc ((struct obstack *) data, total); + memset (ptr, 0, total); + return ptr; +} + +/* Trivial deallocation function for the libiberty splay tree and hash + table - don't deallocate anything. Rely on later deletion of the + obstack. */ + +void +dummy_obstack_deallocate (void *object, void *data) +{ + return; +} Index: src/gdb/value.c =================================================================== --- src.orig/gdb/value.c 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/value.c 2005-12-09 15:19:49.000000000 -0500 @@ -653,29 +653,6 @@ access_value_history (int num) return value_copy (chunk->values[absnum % VALUE_HISTORY_CHUNK]); } -/* Clear the value history entirely. - Must be done when new symbol tables are loaded, - because the type pointers become invalid. */ - -void -clear_value_history (void) -{ - struct value_history_chunk *next; - int i; - struct value *val; - - while (value_history_chain) - { - for (i = 0; i < VALUE_HISTORY_CHUNK; i++) - if ((val = value_history_chain->values[i]) != NULL) - xfree (val); - next = value_history_chain->next; - xfree (value_history_chain); - value_history_chain = next; - } - value_history_count = 0; -} - static void show_values (char *num_exp, int from_tty) { @@ -842,22 +819,46 @@ internalvar_name (struct internalvar *va return var->name; } -/* Free all internalvars. Done when new symtabs are loaded, - because that makes the values invalid. */ +/* Update VALUE before discarding OBJFILE. COPIED_TYPES is used to + prevent cycles / duplicates. */ + +static void +preserve_one_value (struct value *value, struct objfile *objfile, + htab_t copied_types) +{ + if (TYPE_OBJFILE (value->type) == objfile) + value->type = copy_type_recursive (objfile, value->type, copied_types); + + if (TYPE_OBJFILE (value->enclosing_type) == objfile) + value->enclosing_type = copy_type_recursive (objfile, + value->enclosing_type, + copied_types); +} + +/* Update the internal variables and value history when OBJFILE is + discarded; we must copy the types out of the objfile. */ void -clear_internalvars (void) +preserve_values (struct objfile *objfile) { + htab_t copied_types; + struct value_history_chunk *cur; struct internalvar *var; + int i; - while (internalvars) - { - var = internalvars; - internalvars = var->next; - xfree (var->name); - xfree (var->value); - xfree (var); - } + /* Create the hash table. We allocate on the objfile's obstack, since + 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 (var = internalvars; var; var = var->next) + preserve_one_value (var->value, objfile, copied_types); + + htab_delete (copied_types); } static void Index: src/gdb/value.h =================================================================== --- src.orig/gdb/value.h 2005-12-09 14:48:06.000000000 -0500 +++ src/gdb/value.h 2005-12-09 15:19:49.000000000 -0500 @@ -502,9 +502,7 @@ extern void typedef_print (struct type * extern char *internalvar_name (struct internalvar *var); -extern void clear_value_history (void); - -extern void clear_internalvars (void); +extern void preserve_values (struct objfile *); /* From values.c */ ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 4:46 ` [RFC] Alternate approach to keeping convenience variables Daniel Jacobowitz @ 2005-12-10 5:07 ` Jim Blandy 2005-12-10 8:24 ` Daniel Jacobowitz 2005-12-11 19:17 ` Eli Zaretskii ` (2 subsequent siblings) 3 siblings, 1 reply; 32+ messages in thread From: Jim Blandy @ 2005-12-10 5:07 UTC (permalink / raw) To: Andrew STUBBS, Jim Blandy, gdb-patches On 12/9/05, Daniel Jacobowitz <drow@false.org> wrote: > 2005-12-09 Daniel Jacobowitz <dan@codesourcery.com> > > * Makefile.in (gdbtypes_h, gdbtypes.o, utils.o): Update. > * defs.h (hashtab_obstack_allocate, dummy_obstack_deallocate): Add > prototypes. > * dwarf2read.c (read_subroutine_type): Use TYPE_ZALLOC. > (hashtab_obstack_allocate, dummy_obstack_deallocate): Moved to... > * utils.c (hashtab_obstack_allocate, dummy_obstack_deallocate): > ...here. > * gdbtypes.c: Include "hashtab.h". > (build_gdbtypes): Remove extra prototype. > (struct type_pair, type_pair_hash, type_pair_eq) > (create_copied_types_hash, copy_type_recursive): New. > * gdbtypes.h: Include "hashtab.h". > (TYPE_ZALLOC): New. > (create_copied_types_hash, copy_type_recursive): New prototypes. > * objfiles.c (free_objfile): Call preserve_values. > * symfile.c (reread_symbols): Likewise. > (clear_symtab_users): Remove calls to clear_value_history and > clear_internalvars. > * value.c (clear_value_history, clear_internalvars): Removed. > (preserve_one_value, preserve_values): New functions. > * value.h (clear_value_history, clear_internalvars): Removed. > (preserve_values): New prototype. > > * tracepoint.c (_initialize_tracepoint): Do not initialize convenience > variables here. Well, that's a head-on attack. I think Mr. Stubbs will be pleased when he comes back. I don't understand what the tracepoint.c change has to do with the rest of the patch. Commit the removal of the extra build_gdbtypes prototype as a separate obvious change. Same for TYPE_ZALLOC. Make this a function, not a macro. It would be nice if the comments for hashtab_obstack_allocate and the dummy free actually explained what sort of DATA argument they expect. copy_type_recursive undoes the sharing given a bunch of 'struct type' objects all referring to a given 'struct main_type' object. You could just stick both kinds of pointers in the type hash, at the cost of some static typing. And it doesn't preserve 'pointer_type', 'reference_type', or 'chain' groupings. Go ahead and expand the comment in copy_type_recursive to spell out why it is *necessary* to add the type pair to the hash table before the type is completely constructed. Don't just point out that we do it where we do it. Because they would have different lifetimes, we should never have a type in one objfile pointing to a type in another objfile. And if we ever did, then that assert would catch it. Good. There should be a comment to that effect in some appropriate place. preserve_values also needs a comment indicating that it's meant to be called only when we're about to free the given objfile, which ensures that we never make more than one copy of a given type. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 5:07 ` Jim Blandy @ 2005-12-10 8:24 ` Daniel Jacobowitz 2005-12-10 22:20 ` Jim Blandy 0 siblings, 1 reply; 32+ messages in thread From: Daniel Jacobowitz @ 2005-12-10 8:24 UTC (permalink / raw) To: Jim Blandy; +Cc: Andrew STUBBS, gdb-patches Meta-issue: When you're reviewing a patch, especially from a peer, please keep courtesy in mind. I value your comments and will honor any suggestions or objections; that doesn't mean that I need your permission to do things, or that I want to be instructed to fix problems with pre-existing code. Especially true when I don't agree that the usage is problematic (e.g. the use of a macro for TYPE_ZALLOC, paralleling TYPE_ALLOC). It's the difference between offering comments and advice, and giving orders. I will listen to your advice; I will not accept your commands. I've edited out the remainder of my annoyed comments from this version. On Fri, Dec 09, 2005 at 03:13:29PM -0800, Jim Blandy wrote: > I don't understand what the tracepoint.c change has to do with the > rest of the patch. Take another look at the experiment in my message. With the patch applied, the initially created convenience variables are no longer discarded by loading the initial objfile. Either tracepoint.c has to change, or the output of "show convenience" in default.exp does. > It would be nice if the comments for hashtab_obstack_allocate and the > dummy free actually explained what sort of DATA argument they expect. I've taken care of that locally. > copy_type_recursive undoes the sharing given a bunch of 'struct type' > objects all referring to a given 'struct main_type' object. You could > just stick both kinds of pointers in the type hash, at the cost of > some static typing. We could do this. It makes the initialization rather more complicated; we have to bypass alloc_type in some cases, and we can't use alloc_type_instance because it expects the old type (there's no pointer from the main_type back to the chain). Do you think this is worthwhile? If so I'll try to do it. > And it doesn't preserve 'pointer_type', > 'reference_type', or 'chain' groupings. That was a deliberate choice, to (in this context) reduce memory usage. The set of convenience variables will be relatively small. There's no point in eagerly copying either the cv-chains, or the pointer-to/reference-to chains, if they aren't used. Most of the time I expect that they won't be; do you expect otherwise? There's no straightforward way to non-eagerly preserve the chains. > Go ahead and expand the comment in copy_type_recursive to spell out > why it is *necessary* to add the type pair to the hash table before > the type is completely constructed. Don't just point out that we do > it where we do it. This is standard for any hash-table based tree walker... > preserve_values also needs a comment indicating that it's meant to be > called only when we're about to free the given objfile, which ensures > that we never make more than one copy of a given type. That's unnecessary; you could call it multiple times if you wanted to and it would do the right thing (i.e. nothing). It only looks for types associated with the objfile to be discarded, and it replaces them all. -- Daniel Jacobowitz CodeSourcery, LLC ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 8:24 ` Daniel Jacobowitz @ 2005-12-10 22:20 ` Jim Blandy 2005-12-11 18:12 ` Eli Zaretskii 0 siblings, 1 reply; 32+ messages in thread From: Jim Blandy @ 2005-12-10 22:20 UTC (permalink / raw) To: Jim Blandy, Andrew STUBBS, gdb-patches On 12/9/05, Daniel Jacobowitz <drow@false.org> wrote: > Meta-issue: When you're reviewing a patch, especially from a peer, > please keep courtesy in mind. I value your comments and will honor any > suggestions or objections; that doesn't mean that I need your > permission to do things, or that I want to be instructed to fix > problems with pre-existing code. Whoa. Being bossy wasn't on my mind. I did write imperative sentences, but only because it was terser than writing "I think", "How about", etc. each time. If it came across as something other than collegial, I'll certainly avoid that in the future. > Especially true when I don't agree that the usage is problematic (e.g. > the use of a macro for TYPE_ZALLOC, paralleling TYPE_ALLOC). Well, you're adding a new usage. I think macros are fine for things where they offer a brevity that you can't get with other kinds of abstractions, but this seems like a case where there isn't much advantage; and you lose type checking, stepping ability, etc. > On Fri, Dec 09, 2005 at 03:13:29PM -0800, Jim Blandy wrote: > > I don't understand what the tracepoint.c change has to do with the > > rest of the patch. > > Take another look at the experiment in my message. With the patch > applied, the initially created convenience variables are no longer > discarded by loading the initial objfile. Either tracepoint.c > has to change, or the output of "show convenience" in default.exp does. Okay, I see. I think default.exp should be changed; preserving the values of those variables seems like a feature, not a bug. > > copy_type_recursive undoes the sharing given a bunch of 'struct type' > > objects all referring to a given 'struct main_type' object. You could > > just stick both kinds of pointers in the type hash, at the cost of > > some static typing. > > We could do this. It makes the initialization rather more complicated; > we have to bypass alloc_type in some cases, and we can't use > alloc_type_instance because it expects the old type (there's no pointer > from the main_type back to the chain). > > Do you think this is worthwhile? If so I'll try to do it. ... I'm not sure it's worthwhile. > > And it doesn't preserve 'pointer_type', > > 'reference_type', or 'chain' groupings. > > That was a deliberate choice, to (in this context) reduce memory usage. > The set of convenience variables will be relatively small. There's no > point in eagerly copying either the cv-chains, or the > pointer-to/reference-to chains, if they aren't used. Most of the time > I expect that they won't be; do you expect otherwise? There's no > straightforward way to non-eagerly preserve the chains. No, I see what you mean with this. > > > Go ahead and expand the comment in copy_type_recursive to spell out > > why it is *necessary* to add the type pair to the hash table before > > the type is completely constructed. Don't just point out that we do > > it where we do it. > > This is standard for any hash-table based tree walker... Yes, but the code will also be read by people trying to learn the algorithm, not just by people who already know it. If you like, I'll write the comment. > > preserve_values also needs a comment indicating that it's meant to be > > called only when we're about to free the given objfile, which ensures > > that we never make more than one copy of a given type. > > That's unnecessary; you could call it multiple times if you wanted to > and it would do the right thing (i.e. nothing). It only looks for > types associated with the objfile to be discarded, and it replaces them > all. Maybe I'm misunderstanding. If you call it multiple times on the same objfile, and there are new convenience variables whose types come from that objfile each time, it will make fresh copies of all the new variables' types, rather than sharing them with those of values preserved by prior calls, because we allocate a fresh hash table each time preserve_values is called. Isn't that right? I'm not sure this is important enough to worry about; it's just a facet of the behavior that I thought was worth explaining. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 22:20 ` Jim Blandy @ 2005-12-11 18:12 ` Eli Zaretskii 0 siblings, 0 replies; 32+ messages in thread From: Eli Zaretskii @ 2005-12-11 18:12 UTC (permalink / raw) To: Jim Blandy; +Cc: gdb-patches > Date: Fri, 9 Dec 2005 18:10:36 -0800 > From: Jim Blandy <jimb@red-bean.com> > > Whoa. Being bossy wasn't on my mind. I did write imperative > sentences, but only because it was terser than writing "I think", "How > about", etc. each time. Sometimes, one can be in the mood whereby reading a too imperative language would annoy a lot. FWIW, I generally find that adding "I think", "How about", "IMO", and similar phrases, while it wastes a little bandwidth, is worth that waste in gold in terms of making the discussion more pleasant and constructive. So much so that I now re-read every message before sending and add such reservations and qualifications in every sentence that isn't saying something that is 200% obvious. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 4:46 ` [RFC] Alternate approach to keeping convenience variables Daniel Jacobowitz 2005-12-10 5:07 ` Jim Blandy @ 2005-12-11 19:17 ` Eli Zaretskii 2006-01-23 22:29 ` Jim Blandy 2006-01-04 12:17 ` Andrew STUBBS 2006-01-22 21:31 ` Daniel Jacobowitz 3 siblings, 1 reply; 32+ messages in thread From: Eli Zaretskii @ 2005-12-11 19:17 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew STUBBS > Date: Fri, 9 Dec 2005 15:59:23 -0500 > From: Daniel Jacobowitz <drow@false.org> > Cc: Jim Blandy <jimb@red-bean.com>, gdb-patches@sources.redhat.com > > The tracepoint.c change is necessary for default.exp. To see why, try > this: > $ gdb > (gdb) show convenience > $trace_frame = -1 > $tpnum = 0 > > vs. > $ gdb /bin/ls > (gdb) show convenience > No debugger convenience variables now defined. > Convenience variables have names starting with "$"; > use "set" as in "set $foo = 5" to define them. > > If you want the inconsistency resolve the other way round, please > holler. I understand that, with your change, GDB will behave as in the second example, even if no executable file was loaded yet, yes? If so, I'm for this change: I think GDB shouldn't show tracepoint-related variables unless tracepoints are actually being used. > How does this look? Looks good to me, thanks. A few minor comments: . copy_type_recursive looks very much like a kind of deep copy. Is it? If so, perhaps we should call the function accordingly. . Do you think we need to add or change anything in the manual as result of this patch? . How about adding a few words in gdbint.texinfo to describe the strategy of your solution? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-11 19:17 ` Eli Zaretskii @ 2006-01-23 22:29 ` Jim Blandy 2006-01-24 11:44 ` Andrew STUBBS 0 siblings, 1 reply; 32+ messages in thread From: Jim Blandy @ 2006-01-23 22:29 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches, Andrew STUBBS On 12/10/05, Eli Zaretskii <eliz@gnu.org> wrote: > > Date: Fri, 9 Dec 2005 15:59:23 -0500 > > From: Daniel Jacobowitz <drow@false.org> > > Cc: Jim Blandy <jimb@red-bean.com>, gdb-patches@sources.redhat.com > > > > The tracepoint.c change is necessary for default.exp. To see why, try > > this: > > $ gdb > > (gdb) show convenience > > $trace_frame = -1 > > $tpnum = 0 > > > > vs. > > $ gdb /bin/ls > > (gdb) show convenience > > No debugger convenience variables now defined. > > Convenience variables have names starting with "$"; > > use "set" as in "set $foo = 5" to define them. > > > > If you want the inconsistency resolve the other way round, please > > holler. > > I understand that, with your change, GDB will behave as in the second > example, even if no executable file was loaded yet, yes? If so, I'm > for this change: I think GDB shouldn't show tracepoint-related > variables unless tracepoints are actually being used. [leaving lots of context in, since it's been a while] The $trace_frame variable is set to -1 when GDB starts, which indicates that there's no trace frame selected. I think it would be a little more helpful for the variables to always exist: you can write user-defined commands that give a reasonable error message, for example. GDB doesn't have any way (?) to check whether a convenience variable has been initialized yet. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-23 22:29 ` Jim Blandy @ 2006-01-24 11:44 ` Andrew STUBBS 2006-01-24 18:41 ` Jim Blandy 0 siblings, 1 reply; 32+ messages in thread From: Andrew STUBBS @ 2006-01-24 11:44 UTC (permalink / raw) To: Jim Blandy; +Cc: Eli Zaretskii, gdb-patches Jim Blandy wrote: > The $trace_frame variable is set to -1 when GDB starts, which > indicates that there's no trace frame selected. I think it would be a > little more helpful for the variables to always exist: you can write > user-defined commands that give a reasonable error message, for > example. GDB doesn't have any way (?) to check whether a convenience > variable has been initialized yet. A newly created convenience variable has the value 'void'. (A variable is created the first time it is referenced, even as an rvalue.) Therefore, if it is 'void' it can be considered uninitialised. This is how the 'init-if-undefined' command works. The only way (I know of) to set it back to 'void', once assigned another value, is to assign the value of another uninitialised variable, which I would consider as copying the undefined status as well as the value 'void'. So an example GDB script fragment to check a variable might look like this: set $temp = $trace_frame init-if-undefined $temp = 99999 if $temp == 99999 Copying the value to $temp ensures that the original variable $trace_frame remains undamaged. HTH Andrew Stubbs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-24 11:44 ` Andrew STUBBS @ 2006-01-24 18:41 ` Jim Blandy 2006-01-24 18:43 ` Daniel Jacobowitz 0 siblings, 1 reply; 32+ messages in thread From: Jim Blandy @ 2006-01-24 18:41 UTC (permalink / raw) To: Andrew STUBBS; +Cc: Eli Zaretskii, gdb-patches On 1/24/06, Andrew STUBBS <andrew.stubbs@st.com> wrote: > Jim Blandy wrote: > > The $trace_frame variable is set to -1 when GDB starts, which > > indicates that there's no trace frame selected. I think it would be a > > little more helpful for the variables to always exist: you can write > > user-defined commands that give a reasonable error message, for > > example. GDB doesn't have any way (?) to check whether a convenience > > variable has been initialized yet. > > A newly created convenience variable has the value 'void'. (A variable > is created the first time it is referenced, even as an rvalue.) > Therefore, if it is 'void' it can be considered uninitialised. Right, but there's no way to test for that in the scripting language. Your 'init-if-undefined' command has to be a primitive, implemented in C. My argument was that having the variables always be present is more convenient for user-defined commands. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-24 18:41 ` Jim Blandy @ 2006-01-24 18:43 ` Daniel Jacobowitz 2006-01-24 19:16 ` Jim Blandy 0 siblings, 1 reply; 32+ messages in thread From: Daniel Jacobowitz @ 2006-01-24 18:43 UTC (permalink / raw) To: Jim Blandy; +Cc: Andrew STUBBS, Eli Zaretskii, gdb-patches On Tue, Jan 24, 2006 at 10:40:58AM -0800, Jim Blandy wrote: > On 1/24/06, Andrew STUBBS <andrew.stubbs@st.com> wrote: > > Jim Blandy wrote: > > > The $trace_frame variable is set to -1 when GDB starts, which > > > indicates that there's no trace frame selected. I think it would be a > > > little more helpful for the variables to always exist: you can write > > > user-defined commands that give a reasonable error message, for > > > example. GDB doesn't have any way (?) to check whether a convenience > > > variable has been initialized yet. > > > > A newly created convenience variable has the value 'void'. (A variable > > is created the first time it is referenced, even as an rvalue.) > > Therefore, if it is 'void' it can be considered uninitialised. > > Right, but there's no way to test for that in the scripting language. > Your 'init-if-undefined' command has to be a primitive, implemented in > C. My argument was that having the variables always be present is > more convenient for user-defined commands. Andrew's point is that such a primitive was recently committed :-) -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-24 18:43 ` Daniel Jacobowitz @ 2006-01-24 19:16 ` Jim Blandy 2006-01-24 19:24 ` Daniel Jacobowitz 2006-01-24 19:45 ` Andrew STUBBS 0 siblings, 2 replies; 32+ messages in thread From: Jim Blandy @ 2006-01-24 19:16 UTC (permalink / raw) To: Jim Blandy, Andrew STUBBS, Eli Zaretskii, gdb-patches On 1/24/06, Daniel Jacobowitz <drow@false.org> wrote: > > Right, but there's no way to test for that in the scripting language. > > Your 'init-if-undefined' command has to be a primitive, implemented in > > C. My argument was that having the variables always be present is > > more convenient for user-defined commands. > > Andrew's point is that such a primitive was recently committed :-) If you want your user-defined command to print a helpful error mesage, you can't use that in an 'if'. If we had some operator like $defined($foo), then that'd be different. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-24 19:16 ` Jim Blandy @ 2006-01-24 19:24 ` Daniel Jacobowitz 2006-01-25 0:13 ` Jim Blandy 2006-01-24 19:45 ` Andrew STUBBS 1 sibling, 1 reply; 32+ messages in thread From: Daniel Jacobowitz @ 2006-01-24 19:24 UTC (permalink / raw) To: gdb-patches On Tue, Jan 24, 2006 at 11:16:38AM -0800, Jim Blandy wrote: > On 1/24/06, Daniel Jacobowitz <drow@false.org> wrote: > > > Right, but there's no way to test for that in the scripting language. > > > Your 'init-if-undefined' command has to be a primitive, implemented in > > > C. My argument was that having the variables always be present is > > > more convenient for user-defined commands. > > > > Andrew's point is that such a primitive was recently committed :-) > > If you want your user-defined command to print a helpful error mesage, > you can't use that in an 'if'. If we had some operator like > $defined($foo), then that'd be different. At this point we've gotten pretty far afield from the original question. How does defined($foo) help relative to having the variables always present? To refresh, here's what an unpatched GDB without a symbol file shows: (gdb) show convenience $trace_frame = -1 $tpnum = 0 With a symbol file: (gdb) show convenience No debugger convenience variables now defined. Convenience variables have names starting with "$"; use "set" as in "set $foo = 5" to define them. With a tracepoint: (gdb) show convenience $tpnum = 1 And, presumably, $trace_frame is set when you're actually using tracepoints (which I can't test). I think that the "without symbol file" output should be replaced by the "with symbol file" output. I don't think it likely that there are any existing scripts that this would break. If there were, they could add "init-if-undefined $tpnum = 0" and "init-if-undefined $trace_frame = -1", and be back where they started from. For those not using tracepoints, or huge .gdbinit scripts which know about tracepoints, it's nice to hide the tracepoint variables. Can you think of any example that would break? Since GDB is normally used with a symbol file, it seems unlikely. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-24 19:24 ` Daniel Jacobowitz @ 2006-01-25 0:13 ` Jim Blandy 0 siblings, 0 replies; 32+ messages in thread From: Jim Blandy @ 2006-01-25 0:13 UTC (permalink / raw) To: gdb-patches My point was simply that life is easier when you have invariants that hold over wider regions. I see that someone who has the whole picture in mind can get the effect they want, but it's simpler to have only one case to handle in the first place (is $trace_frame -1?) than two (is $trace_frame even set? what is it set to?). But I think you all understand what I've been trying to say. All the persuading that's going to happen has already happened. :) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-24 19:16 ` Jim Blandy 2006-01-24 19:24 ` Daniel Jacobowitz @ 2006-01-24 19:45 ` Andrew STUBBS 1 sibling, 0 replies; 32+ messages in thread From: Andrew STUBBS @ 2006-01-24 19:45 UTC (permalink / raw) To: Jim Blandy; +Cc: Eli Zaretskii, gdb-patches Jim Blandy wrote: > On 1/24/06, Daniel Jacobowitz <drow@false.org> wrote: >>> Right, but there's no way to test for that in the scripting language. >>> Your 'init-if-undefined' command has to be a primitive, implemented in >>> C. My argument was that having the variables always be present is >>> more convenient for user-defined commands. >> Andrew's point is that such a primitive was recently committed :-) > > If you want your user-defined command to print a helpful error mesage, > you can't use that in an 'if'. If we had some operator like > $defined($foo), then that'd be different. I'm not totally sure what you mean by this. I put the following into the wavefront GDB and it works as I expected: define defined set $temp = $arg0 init-if-undefined $temp = 99999 if $temp == 99999 echo $arg0 not defined\n end end (gdb) defined $a $a not defined (gdb) defined $tpnum (gdb) It has its limitations, of course, but it does work. Your suggestion would still be better. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 4:46 ` [RFC] Alternate approach to keeping convenience variables Daniel Jacobowitz 2005-12-10 5:07 ` Jim Blandy 2005-12-11 19:17 ` Eli Zaretskii @ 2006-01-04 12:17 ` Andrew STUBBS 2006-01-04 17:00 ` Jim Blandy 2006-01-22 21:31 ` Daniel Jacobowitz 3 siblings, 1 reply; 32+ messages in thread From: Andrew STUBBS @ 2006-01-04 12:17 UTC (permalink / raw) To: Daniel Jacobowitz; +Cc: Jim Blandy, gdb-patches Hi, I'm back! Thanks for this work. I do not follow exactly how it works (I suppose that's the difference between somebody who knows GDB and somebody who does not) but it does appear to solve the problem at hand. One point though - why is it ok to leak the memory? This seems like bad practice to me - I mean, types can be arbitrarily large and, even if they are typically small, the variables may be rewritten many many times (particularly by scripts with loops). Are you saying that they will be actually leaked or just left for some sort of garbage collection? Otherwise excellent - just what I need. I'll be very happy when this goes in. Thanks again Andrew Stubbs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-04 12:17 ` Andrew STUBBS @ 2006-01-04 17:00 ` Jim Blandy 2006-01-04 17:48 ` Andrew STUBBS 0 siblings, 1 reply; 32+ messages in thread From: Jim Blandy @ 2006-01-04 17:00 UTC (permalink / raw) To: Andrew STUBBS; +Cc: Daniel Jacobowitz, gdb-patches On 1/4/06, Andrew STUBBS <andrew.stubbs@st.com> wrote: > One point though - why is it ok to leak the memory? This seems like bad > practice to me - I mean, types can be arbitrarily large and, even if > they are typically small, the variables may be rewritten many many times > (particularly by scripts with loops). Are you saying that they will be > actually leaked or just left for some sort of garbage collection? No, it'll really be leaked. But the leakage only occurs when symbol files get unloaded from memory, so it would only affect scripts that load and unload symbol files in a loop. I agree it's not optimal, but I think the alternative is GC. Maybe we should think about that. Has anyone ever tried running GDB using the Boehm collector's replacements for malloc and free? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-04 17:00 ` Jim Blandy @ 2006-01-04 17:48 ` Andrew STUBBS 2006-01-04 18:37 ` Jim Blandy 0 siblings, 1 reply; 32+ messages in thread From: Andrew STUBBS @ 2006-01-04 17:48 UTC (permalink / raw) To: Jim Blandy; +Cc: Daniel Jacobowitz, gdb-patches Jim Blandy wrote: > On 1/4/06, Andrew STUBBS <andrew.stubbs@st.com> wrote: > >>One point though - why is it ok to leak the memory? This seems like bad >>practice to me - I mean, types can be arbitrarily large and, even if >>they are typically small, the variables may be rewritten many many times >>(particularly by scripts with loops). Are you saying that they will be >>actually leaked or just left for some sort of garbage collection? > > > No, it'll really be leaked. But the leakage only occurs when symbol > files get unloaded from memory, so it would only affect scripts that > load and unload symbol files in a loop. I agree it's not optimal, but > I think the alternative is GC. > > Maybe we should think about that. Has anyone ever tried running GDB > using the Boehm collector's replacements for malloc and free? > Perhaps you could modify copy_type_recursive such that it creates a cleanup chain specific to the internal variable. Then call do cleanups in set_internal_var. Might be tricky with shared type copies though. Just a thought. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-04 17:48 ` Andrew STUBBS @ 2006-01-04 18:37 ` Jim Blandy 2006-01-22 21:04 ` Daniel Jacobowitz 0 siblings, 1 reply; 32+ messages in thread From: Jim Blandy @ 2006-01-04 18:37 UTC (permalink / raw) To: Andrew STUBBS; +Cc: Daniel Jacobowitz, gdb-patches On 1/4/06, Andrew STUBBS <andrew.stubbs@st.com> wrote: > Perhaps you could modify copy_type_recursive such that it creates a > cleanup chain specific to the internal variable. Then call do cleanups > in set_internal_var. > > Might be tricky with shared type copies though. Just a thought. Exactly. You can keep throwing complexity into the code to deal with this stuff until it buries you. And it'll never be a full solution. Or it'll be complete until someone wants to add some perfectly reasonable feature (just as is happening with "let's preserve the values of convenience variables across symfile loads" right now). At some point, we have to take a step back and think about a complete solution. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-04 18:37 ` Jim Blandy @ 2006-01-22 21:04 ` Daniel Jacobowitz 0 siblings, 0 replies; 32+ messages in thread From: Daniel Jacobowitz @ 2006-01-22 21:04 UTC (permalink / raw) To: Jim Blandy; +Cc: Andrew STUBBS, gdb-patches On Wed, Jan 04, 2006 at 10:37:06AM -0800, Jim Blandy wrote: > On 1/4/06, Andrew STUBBS <andrew.stubbs@st.com> wrote: > > Perhaps you could modify copy_type_recursive such that it creates a > > cleanup chain specific to the internal variable. Then call do cleanups > > in set_internal_var. > > > > Might be tricky with shared type copies though. Just a thought. > > Exactly. > > You can keep throwing complexity into the code to deal with this stuff > until it buries you. And it'll never be a full solution. Or it'll be > complete until someone wants to add some perfectly reasonable feature > (just as is happening with "let's preserve the values of convenience > variables across symfile loads" right now). At some point, we have to > take a step back and think about a complete solution. Right. For now, I'm OK with saying "this is a small memory leak when symbol files are unloaded". If someone wants to investigate adding boehm-gc to GDB and using that, instead, then I'm OK with that too :-) Every once in a while I do get the urge to go through GDB with a leak checker. But not often enough to jump through huge, rarely tested hoops for that case. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2005-12-10 4:46 ` [RFC] Alternate approach to keeping convenience variables Daniel Jacobowitz ` (2 preceding siblings ...) 2006-01-04 12:17 ` Andrew STUBBS @ 2006-01-22 21:31 ` Daniel Jacobowitz 2006-01-23 22:46 ` Jim Blandy ` (2 more replies) 3 siblings, 3 replies; 32+ messages in thread From: Daniel Jacobowitz @ 2006-01-22 21:31 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew STUBBS, Jim Blandy This version of the patch addresses all the comments I've received, except for: - TYPE_ZALLOC. I think this is more appropriate as a macro, personally, though I wouldn't argue too loudly if someone wanted to functionify it at the same time as TYPE_ALLOC. GDB developers seem to have developed an allergy to C macros that I just don't understand. They're not _inherently_ obfuscating or evil! They can be both useful and elegant. - gdbint; I couldn't find a vaguely appropriate place to add a description of this, and I don't think it's such a fascinating approach that it's worth creating a new section for. - $tpnum. Eli was in favor of the change I'd made, Jim preferred the opposite. We don't set $bpnum until the first breakpoint is set; Jim, is that a convincing argument for not setting $tpnum until the first tracepoint is set? I noticed in the course of updating gdb.texinfo that there are still a whole lot of references to VxWorks. GDB no longer supports VxWorks, and current versions of VxWorks don't speak a compatible protocol to the one that GDB used to support, anyway. Ripping this out will be a little more work than I have time for at the moment though. -- Daniel Jacobowitz CodeSourcery 2006-01-22 Daniel Jacobowitz <dan@codesourcery.com> * Makefile.in (gdbtypes_h, gdbtypes.o, utils.o): Update. * defs.h (hashtab_obstack_allocate, dummy_obstack_deallocate): Add prototypes. * dwarf2read.c (read_subroutine_type): Use TYPE_ZALLOC. (hashtab_obstack_allocate, dummy_obstack_deallocate): Moved to... * utils.c (hashtab_obstack_allocate, dummy_obstack_deallocate): ...here. * gdbtypes.c: Include "hashtab.h". (build_gdbtypes): Remove extra prototype. (struct type_pair, type_pair_hash, type_pair_eq) (create_copied_types_hash, copy_type_recursive): New. * gdbtypes.h: Include "hashtab.h". (TYPE_ZALLOC): New. (create_copied_types_hash, copy_type_recursive): New prototypes. * objfiles.c (free_objfile): Call preserve_values. * symfile.c (reread_symbols): Likewise. (clear_symtab_users): Remove calls to clear_value_history and clear_internalvars. * value.c (clear_value_history, clear_internalvars): Removed. (preserve_one_value, preserve_values): New functions. * value.h (clear_value_history, clear_internalvars): Removed. (preserve_values): New prototype. * tracepoint.c (_initialize_tracepoint): Do not initialize convenience variables here. * doc/gdb.texinfo (Files): Remove obsolete bits from the description of "symbol-file". Index: src/gdb/Makefile.in =================================================================== --- src.orig/gdb/Makefile.in 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/Makefile.in 2006-01-22 16:08:23.000000000 -0500 @@ -696,7 +696,7 @@ gdb_stat_h = gdb_stat.h gdb_string_h = gdb_string.h gdb_thread_db_h = gdb_thread_db.h gdbthread_h = gdbthread.h $(breakpoint_h) $(frame_h) -gdbtypes_h = gdbtypes.h +gdbtypes_h = gdbtypes.h $(hashtab_h) gdb_vfork_h = gdb_vfork.h gdb_wait_h = gdb_wait.h glibc_tdep_h = glibc-tdep.h @@ -1979,7 +1979,7 @@ gdb-events.o: gdb-events.c $(defs_h) $(g gdbtypes.o: gdbtypes.c $(defs_h) $(gdb_string_h) $(bfd_h) $(symtab_h) \ $(symfile_h) $(objfiles_h) $(gdbtypes_h) $(expression_h) \ $(language_h) $(target_h) $(value_h) $(demangle_h) $(complaints_h) \ - $(gdbcmd_h) $(wrapper_h) $(cp_abi_h) $(gdb_assert_h) + $(gdbcmd_h) $(wrapper_h) $(cp_abi_h) $(gdb_assert_h) $(hashtab_h) glibc-tdep.o: glibc-tdep.c $(defs_h) $(frame_h) $(symtab_h) $(symfile_h) \ $(objfiles_h) $(glibc_tdep_h) gnu-nat.o: gnu-nat.c $(gdb_string_h) $(defs_h) $(inferior_h) $(symtab_h) \ @@ -2714,7 +2714,7 @@ utils.o: utils.c $(defs_h) $(gdb_assert_ $(exceptions_h) $(tui_h) $(gdbcmd_h) $(serial_h) $(bfd_h) \ $(target_h) $(demangle_h) $(expression_h) $(language_h) $(charset_h) \ $(annotate_h) $(filenames_h) $(symfile_h) $(inferior_h) \ - $(gdb_curses_h) $(readline_h) + $(gdb_curses_h) $(readline_h) $(gdb_obstack_h) uw-thread.o: uw-thread.c $(defs_h) $(gdbthread_h) $(target_h) $(inferior_h) \ $(regcache_h) $(gregset_h) v850-tdep.o: v850-tdep.c $(defs_h) $(frame_h) $(frame_base_h) $(trad_frame_h) \ Index: src/gdb/defs.h =================================================================== --- src.orig/gdb/defs.h 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/defs.h 2006-01-22 16:08:23.000000000 -0500 @@ -1217,4 +1217,9 @@ extern int use_windows; extern ULONGEST align_up (ULONGEST v, int n); extern ULONGEST align_down (ULONGEST v, int n); +/* Allocation and deallocation functions for the libiberty hash table + which use obstacks. */ +void *hashtab_obstack_allocate (void *data, size_t size, size_t count); +void dummy_obstack_deallocate (void *object, void *data); + #endif /* #ifndef DEFS_H */ Index: src/gdb/dwarf2read.c =================================================================== --- src.orig/gdb/dwarf2read.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/dwarf2read.c 2006-01-22 16:08:23.000000000 -0500 @@ -1026,10 +1026,6 @@ static char *skip_one_die (char *info_pt static void free_stack_comp_unit (void *); -static void *hashtab_obstack_allocate (void *data, size_t size, size_t count); - -static void dummy_obstack_deallocate (void *object, void *data); - static hashval_t partial_die_hash (const void *item); static int partial_die_eq (const void *item_lhs, const void *item_rhs); @@ -4622,7 +4618,7 @@ read_subroutine_type (struct die_info *d /* Allocate storage for parameters and fill them in. */ TYPE_NFIELDS (ftype) = nparams; TYPE_FIELDS (ftype) = (struct field *) - TYPE_ALLOC (ftype, nparams * sizeof (struct field)); + TYPE_ZALLOC (ftype, nparams * sizeof (struct field)); child_die = die->child; while (child_die && child_die->tag) @@ -9617,28 +9613,6 @@ dwarf2_clear_marks (struct dwarf2_per_cu } } -/* Allocation function for the libiberty hash table which uses an - obstack. */ - -static void * -hashtab_obstack_allocate (void *data, size_t size, size_t count) -{ - unsigned int total = size * count; - void *ptr = obstack_alloc ((struct obstack *) data, total); - memset (ptr, 0, total); - return ptr; -} - -/* Trivial deallocation function for the libiberty splay tree and hash - table - don't deallocate anything. Rely on later deletion of the - obstack. */ - -static void -dummy_obstack_deallocate (void *object, void *data) -{ - return; -} - /* Trivial hash function for partial_die_info: the hash value of a DIE is its offset in .debug_info for this objfile. */ Index: src/gdb/gdbtypes.c =================================================================== --- src.orig/gdb/gdbtypes.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/gdbtypes.c 2006-01-22 16:08:40.000000000 -0500 @@ -1,6 +1,6 @@ /* Support routines for manipulating internal types for GDB. Copyright 1992, 1993, 1994, 1995, 1996, 1998, 1999, 2000, 2001, 2002, 2003, - 2004 Free Software Foundation, Inc. + 2004, 2005 Free Software Foundation, Inc. Contributed by Cygnus Support, using pieces from other GDB modules. This file is part of GDB. @@ -37,6 +37,7 @@ #include "wrapper.h" #include "cp-abi.h" #include "gdb_assert.h" +#include "hashtab.h" /* These variables point to the objects representing the predefined C data types. */ @@ -3111,7 +3112,147 @@ recursive_dump_type (struct type *type, obstack_free (&dont_print_type_obstack, NULL); } -static void build_gdbtypes (void); +/* Trivial helpers for the libiberty hash table, for mapping one + type to another. */ + +struct type_pair +{ + struct type *old, *new; +}; + +static hashval_t +type_pair_hash (const void *item) +{ + const struct type_pair *pair = item; + return htab_hash_pointer (pair->old); +} + +static int +type_pair_eq (const void *item_lhs, const void *item_rhs) +{ + const struct type_pair *lhs = item_lhs, *rhs = item_rhs; + return lhs->old == rhs->old; +} + +/* Allocate the hash table used by copy_type_recursive to walk + types without duplicates. We use OBJFILE's obstack, because + OBJFILE is about to be deleted. */ + +htab_t +create_copied_types_hash (struct objfile *objfile) +{ + return htab_create_alloc_ex (1, type_pair_hash, type_pair_eq, + NULL, &objfile->objfile_obstack, + hashtab_obstack_allocate, + dummy_obstack_deallocate); +} + +/* Recursively copy (deep copy) TYPE, if it is associated with OBJFILE. + Return a new type allocated using malloc, a saved type if we have already + visited TYPE (using COPIED_TYPES), or TYPE if it is not associated with + OBJFILE. */ + +struct type * +copy_type_recursive (struct objfile *objfile, struct type *type, + htab_t copied_types) +{ + struct type_pair *stored, pair; + void **slot; + struct type *new_type; + + if (TYPE_OBJFILE (type) == NULL) + return type; + + /* This type shouldn't be pointing to any types in other objfiles; if + it did, the type might disappear unexpectedly. */ + gdb_assert (TYPE_OBJFILE (type) == objfile); + + pair.old = type; + slot = htab_find_slot (copied_types, &pair, INSERT); + if (*slot != NULL) + return ((struct type_pair *) *slot)->new; + + new_type = alloc_type (NULL); + + /* We must add the new type to the hash table immediately, in case + we encounter this type again during a recursive call below. */ + stored = xmalloc (sizeof (struct type_pair)); + stored->old = type; + stored->new = new_type; + *slot = stored; + + /* Copy the common fields of types. */ + TYPE_CODE (new_type) = TYPE_CODE (type); + TYPE_ARRAY_UPPER_BOUND_TYPE (new_type) = TYPE_ARRAY_UPPER_BOUND_TYPE (type); + TYPE_ARRAY_LOWER_BOUND_TYPE (new_type) = TYPE_ARRAY_LOWER_BOUND_TYPE (type); + if (TYPE_NAME (type)) + TYPE_NAME (new_type) = xstrdup (TYPE_NAME (type)); + if (TYPE_TAG_NAME (type)) + TYPE_TAG_NAME (new_type) = xstrdup (TYPE_TAG_NAME (type)); + TYPE_FLAGS (new_type) = TYPE_FLAGS (type); + TYPE_VPTR_FIELDNO (new_type) = TYPE_VPTR_FIELDNO (type); + + TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type); + TYPE_LENGTH (new_type) = TYPE_LENGTH (type); + + /* Copy the fields. */ + TYPE_NFIELDS (new_type) = TYPE_NFIELDS (type); + if (TYPE_NFIELDS (type)) + { + int i, nfields; + + nfields = TYPE_NFIELDS (type); + TYPE_FIELDS (new_type) = xmalloc (sizeof (struct field) * nfields); + for (i = 0; i < nfields; i++) + { + TYPE_FIELD_ARTIFICIAL (new_type, i) = TYPE_FIELD_ARTIFICIAL (type, i); + TYPE_FIELD_BITSIZE (new_type, i) = TYPE_FIELD_BITSIZE (type, i); + if (TYPE_FIELD_TYPE (type, i)) + TYPE_FIELD_TYPE (new_type, i) + = copy_type_recursive (objfile, TYPE_FIELD_TYPE (type, i), + copied_types); + if (TYPE_FIELD_NAME (type, i)) + TYPE_FIELD_NAME (new_type, i) = xstrdup (TYPE_FIELD_NAME (type, i)); + if (TYPE_FIELD_STATIC_HAS_ADDR (type, i)) + SET_FIELD_PHYSADDR (TYPE_FIELD (new_type, i), + TYPE_FIELD_STATIC_PHYSADDR (type, i)); + else if (TYPE_FIELD_STATIC (type, i)) + SET_FIELD_PHYSNAME (TYPE_FIELD (new_type, i), + xstrdup (TYPE_FIELD_STATIC_PHYSNAME (type, i))); + else + { + TYPE_FIELD_BITPOS (new_type, i) = TYPE_FIELD_BITPOS (type, i); + TYPE_FIELD_STATIC_KIND (new_type, i) = 0; + } + } + } + + /* Copy pointers to other types. */ + if (TYPE_TARGET_TYPE (type)) + TYPE_TARGET_TYPE (new_type) = copy_type_recursive (objfile, + TYPE_TARGET_TYPE (type), + copied_types); + if (TYPE_VPTR_BASETYPE (type)) + TYPE_VPTR_BASETYPE (new_type) = copy_type_recursive (objfile, + TYPE_VPTR_BASETYPE (type), + copied_types); + /* Maybe copy the type_specific bits. + + NOTE drow/2005-12-09: We do not copy the C++-specific bits like + base classes and methods. There's no fundamental reason why we + can't, but at the moment it is not needed. */ + + if (TYPE_CODE (type) == TYPE_CODE_FLT) + TYPE_FLOATFORMAT (new_type) == TYPE_FLOATFORMAT (type); + else if (TYPE_CODE (type) == TYPE_CODE_STRUCT + || TYPE_CODE (type) == TYPE_CODE_UNION + || TYPE_CODE (type) == TYPE_CODE_TEMPLATE + || TYPE_CODE (type) == TYPE_CODE_NAMESPACE) + INIT_CPLUS_SPECIFIC (new_type); + + return new_type; +} + static void build_gdbtypes (void) { Index: src/gdb/gdbtypes.h =================================================================== --- src.orig/gdb/gdbtypes.h 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/gdbtypes.h 2006-01-22 16:08:23.000000000 -0500 @@ -25,6 +25,8 @@ #if !defined (GDBTYPES_H) #define GDBTYPES_H 1 +#include "hashtab.h" + /* Forward declarations for prototypes. */ struct field; struct block; @@ -1175,6 +1177,12 @@ extern struct type *builtin_type_f_void; ? obstack_alloc (&TYPE_OBJFILE (t) -> objfile_obstack, size) \ : xmalloc (size)) +#define TYPE_ZALLOC(t,size) \ + (TYPE_OBJFILE (t) != NULL \ + ? memset (obstack_alloc (&TYPE_OBJFILE (t)->objfile_obstack, size), \ + 0, size) \ + : xzalloc (size)) + extern struct type *alloc_type (struct objfile *); extern struct type *init_type (enum type_code, int, int, char *, @@ -1364,4 +1372,10 @@ extern int is_integral_type (struct type extern void maintenance_print_type (char *, int); +extern htab_t create_copied_types_hash (struct objfile *objfile); + +extern struct type *copy_type_recursive (struct objfile *objfile, + struct type *type, + htab_t copied_types); + #endif /* GDBTYPES_H */ Index: src/gdb/objfiles.c =================================================================== --- src.orig/gdb/objfiles.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/objfiles.c 2006-01-22 16:08:23.000000000 -0500 @@ -392,6 +392,10 @@ free_objfile (struct objfile *objfile) objfile->separate_debug_objfile_backlink->separate_debug_objfile = NULL; } + /* Remove any references to this objfile in the global value + lists. */ + preserve_values (objfile); + /* First do any symbol file specific actions required when we are finished with a particular symbol file. Note that if the objfile is using reusable symbol information (via mmalloc) then each of Index: src/gdb/symfile.c =================================================================== --- src.orig/gdb/symfile.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/symfile.c 2006-01-22 16:08:23.000000000 -0500 @@ -2012,6 +2012,10 @@ reread_symbols (void) memcpy (offsets, objfile->section_offsets, SIZEOF_N_SECTION_OFFSETS (num_offsets)); + /* Remove any references to this objfile in the global + value lists. */ + preserve_values (objfile); + /* Nuke all the state that we will re-read. Much of the following code which sets things to NULL really is necessary to tell other parts of GDB that there is nothing currently there. */ @@ -2484,9 +2488,7 @@ clear_symtab_users (void) breakpoint_re_set may try to access the current symtab. */ clear_current_source_symtab_and_line (); - clear_value_history (); clear_displays (); - clear_internalvars (); breakpoint_re_set (); set_default_breakpoint (0, 0, 0, 0); clear_pc_function_cache (); Index: src/gdb/tracepoint.c =================================================================== --- src.orig/gdb/tracepoint.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/tracepoint.c 2006-01-22 16:08:23.000000000 -0500 @@ -2694,11 +2694,6 @@ _initialize_tracepoint (void) traceframe_number = -1; tracepoint_number = -1; - set_internalvar (lookup_internalvar ("tpnum"), - value_from_longest (builtin_type_int, (LONGEST) 0)); - set_internalvar (lookup_internalvar ("trace_frame"), - value_from_longest (builtin_type_int, (LONGEST) - 1)); - if (tracepoint_list.list == NULL) { tracepoint_list.listsize = 128; Index: src/gdb/utils.c =================================================================== --- src.orig/gdb/utils.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/utils.c 2006-01-22 16:08:23.000000000 -0500 @@ -53,6 +53,7 @@ #include "annotate.h" #include "filenames.h" #include "symfile.h" +#include "gdb_obstack.h" #include "inferior.h" /* for signed_pointer_to_address */ @@ -3124,3 +3125,26 @@ align_down (ULONGEST v, int n) gdb_assert (n && (n & (n-1)) == 0); return (v & -n); } + +/* Allocation function for the libiberty hash table which uses an + obstack. The obstack is passed as DATA. */ + +void * +hashtab_obstack_allocate (void *data, size_t size, size_t count) +{ + unsigned int total = size * count; + void *ptr = obstack_alloc ((struct obstack *) data, total); + memset (ptr, 0, total); + return ptr; +} + +/* Trivial deallocation function for the libiberty splay tree and hash + table - don't deallocate anything. Rely on later deletion of the + obstack. DATA will be the obstack, although it is not needed + here. */ + +void +dummy_obstack_deallocate (void *object, void *data) +{ + return; +} Index: src/gdb/value.c =================================================================== --- src.orig/gdb/value.c 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/value.c 2006-01-22 16:11:55.000000000 -0500 @@ -653,29 +653,6 @@ access_value_history (int num) return value_copy (chunk->values[absnum % VALUE_HISTORY_CHUNK]); } -/* Clear the value history entirely. - Must be done when new symbol tables are loaded, - because the type pointers become invalid. */ - -void -clear_value_history (void) -{ - struct value_history_chunk *next; - int i; - struct value *val; - - while (value_history_chain) - { - for (i = 0; i < VALUE_HISTORY_CHUNK; i++) - if ((val = value_history_chain->values[i]) != NULL) - xfree (val); - next = value_history_chain->next; - xfree (value_history_chain); - value_history_chain = next; - } - value_history_count = 0; -} - static void show_values (char *num_exp, int from_tty) { @@ -842,22 +819,49 @@ internalvar_name (struct internalvar *va return var->name; } -/* Free all internalvars. Done when new symtabs are loaded, - because that makes the values invalid. */ +/* Update VALUE before discarding OBJFILE. COPIED_TYPES is used to + prevent cycles / duplicates. */ + +static void +preserve_one_value (struct value *value, struct objfile *objfile, + htab_t copied_types) +{ + if (TYPE_OBJFILE (value->type) == objfile) + value->type = copy_type_recursive (objfile, value->type, copied_types); + + if (TYPE_OBJFILE (value->enclosing_type) == objfile) + value->enclosing_type = copy_type_recursive (objfile, + value->enclosing_type, + copied_types); +} + +/* Update the internal variables and value history when OBJFILE is + discarded; we must copy the types out of the objfile. New global types + will be created for every convenience variable which currently points to + this objfile's types, and the convenience variables will be adjusted to + use the new global types. */ void -clear_internalvars (void) +preserve_values (struct objfile *objfile) { + htab_t copied_types; + struct value_history_chunk *cur; struct internalvar *var; + int i; - while (internalvars) - { - var = internalvars; - internalvars = var->next; - xfree (var->name); - xfree (var->value); - xfree (var); - } + /* Create the hash table. We allocate on the objfile's obstack, since + 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 (var = internalvars; var; var = var->next) + preserve_one_value (var->value, objfile, copied_types); + + htab_delete (copied_types); } static void Index: src/gdb/value.h =================================================================== --- src.orig/gdb/value.h 2006-01-22 16:06:58.000000000 -0500 +++ src/gdb/value.h 2006-01-22 16:08:23.000000000 -0500 @@ -502,9 +502,7 @@ extern void typedef_print (struct type * extern char *internalvar_name (struct internalvar *var); -extern void clear_value_history (void); - -extern void clear_internalvars (void); +extern void preserve_values (struct objfile *); /* From values.c */ Index: src/gdb/doc/gdb.texinfo =================================================================== --- src.orig/gdb/doc/gdb.texinfo 2005-12-03 07:44:31.000000000 -0500 +++ src/gdb/doc/gdb.texinfo 2006-01-22 16:20:46.000000000 -0500 @@ -10821,11 +10821,11 @@ table and program to run from the same f @code{symbol-file} with no argument clears out @value{GDBN} information on your program's symbol table. -The @code{symbol-file} command causes @value{GDBN} to forget the contents -of its convenience variables, the value history, and all breakpoints and -auto-display expressions. This is because they may contain pointers to -the internal data recording symbols and data types, which are part of -the old symbol table data being discarded inside @value{GDBN}. +The @code{symbol-file} command causes @value{GDBN} to forget the contents of +some breakpoints and auto-display expressions. This is because they may +contain pointers to the internal data recording symbols and data types, +which are part of the old symbol table data being discarded inside +@value{GDBN}. @code{symbol-file} does not repeat if you press @key{RET} again after executing it once. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-22 21:31 ` Daniel Jacobowitz @ 2006-01-23 22:46 ` Jim Blandy 2006-01-27 17:53 ` Eli Zaretskii 2006-02-01 23:14 ` Daniel Jacobowitz 2 siblings, 0 replies; 32+ messages in thread From: Jim Blandy @ 2006-01-23 22:46 UTC (permalink / raw) To: gdb-patches, Andrew STUBBS, Jim Blandy On 1/22/06, Daniel Jacobowitz <drow@false.org> wrote: > This version of the patch addresses all the comments I've received, > except for: > > - TYPE_ZALLOC. I think this is more appropriate as a macro, > personally, though I wouldn't argue too loudly if someone wanted to > functionify it at the same time as TYPE_ALLOC. That's fine. I'll have to put my typing where my mouth is. > GDB developers seem to have developed an allergy to C macros that > I just don't understand. They're not _inherently_ obfuscating > or evil! They can be both useful and elegant. Nobody commented on my creative use of macros in m32c-tdep.c. If I were allergic, that'd definitely put me in anaphylactic shock. I don't like TYPE_ZALLOC and TYPE_ALLOC because they don't get anything special from being macros. These aren't time-critical bits of code. They don't construct names. They don't take a type as an argument (err, a compile-time type, that is). They don't expand to statements. They don't export an interface whose presence you'd like to be able to test for easily at compile-time. So why not get the nicer syntax, type checking, and debuggability that functions provide? De gustibus non est disputandum. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-22 21:31 ` Daniel Jacobowitz 2006-01-23 22:46 ` Jim Blandy @ 2006-01-27 17:53 ` Eli Zaretskii 2006-01-27 18:12 ` Jim Blandy 2006-01-27 18:41 ` Joel Brobecker 2006-02-01 23:14 ` Daniel Jacobowitz 2 siblings, 2 replies; 32+ messages in thread From: Eli Zaretskii @ 2006-01-27 17:53 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew STUBBS, Jim Blandy > Date: Sun, 22 Jan 2006 16:31:18 -0500 > From: Daniel Jacobowitz <drow@false.org> > Cc: Andrew STUBBS <andrew.stubbs@st.com>, Jim Blandy <jimb@red-bean.com> > > This version of the patch addresses all the comments I've received, > except for: > > - TYPE_ZALLOC. I think this is more appropriate as a macro, > personally, though I wouldn't argue too loudly if someone wanted to > functionify it at the same time as TYPE_ALLOC. > > GDB developers seem to have developed an allergy to C macros that > I just don't understand. They're not _inherently_ obfuscating > or evil! They can be both useful and elegant. FWIW, I never understood the ``macros are bad, m'kay?'' policy, either. > - gdbint; I couldn't find a vaguely appropriate place to add a > description of this, and I don't think it's such a fascinating > approach that it's worth creating a new section for. At this point, I wouldn't bother about the structure of gdbint.texinfo, just about getting the information in there. The structure is a mess anyhow; in particular, there are too few nodes and too many sections that lack a node. But it is useless to try to fix structure when the full extent of the information is not known even approximately. So, if you have time, just add a section somewhere, even if it is short. I think it's important to document this piece of information. > I noticed in the course of updating gdb.texinfo that there are still a > whole lot of references to VxWorks. GDB no longer supports VxWorks, > and current versions of VxWorks don't speak a compatible protocol to > the one that GDB used to support, anyway. Ripping this out will be a > little more work than I have time for at the moment though. Thanks for the heads-up, I will try to remember to do that when I have my next Rainy Day(tm). > Index: src/gdb/doc/gdb.texinfo > =================================================================== > --- src.orig/gdb/doc/gdb.texinfo 2005-12-03 07:44:31.000000000 -0500 > +++ src/gdb/doc/gdb.texinfo 2006-01-22 16:20:46.000000000 -0500 > @@ -10821,11 +10821,11 @@ table and program to run from the same f > @code{symbol-file} with no argument clears out @value{GDBN} information on your > program's symbol table. > > -The @code{symbol-file} command causes @value{GDBN} to forget the contents > -of its convenience variables, the value history, and all breakpoints and > -auto-display expressions. This is because they may contain pointers to > -the internal data recording symbols and data types, which are part of > -the old symbol table data being discarded inside @value{GDBN}. > +The @code{symbol-file} command causes @value{GDBN} to forget the contents of > +some breakpoints and auto-display expressions. This is because they may > +contain pointers to the internal data recording symbols and data types, > +which are part of the old symbol table data being discarded inside > +@value{GDBN}. > > @code{symbol-file} does not repeat if you press @key{RET} again after > executing it once. This part is approved. I also skimmed the other parts of the patch and didn't see anything I would disapprove. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-27 17:53 ` Eli Zaretskii @ 2006-01-27 18:12 ` Jim Blandy 2006-01-27 18:29 ` Eli Zaretskii 2006-01-27 18:41 ` Joel Brobecker 1 sibling, 1 reply; 32+ messages in thread From: Jim Blandy @ 2006-01-27 18:12 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches, Andrew STUBBS On 1/27/06, Eli Zaretskii <eliz@gnu.org> wrote: > FWIW, I never understood the ``macros are bad, m'kay?'' policy, > either. To be clear, I don't like that policy either. It just asserts that something is better (in a condescending/dopey way, no less), without justification or context. I hope I did better than that when I explained why I think TYPE_ALLOC and TYPE_ZALLOC should be functions. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-27 18:12 ` Jim Blandy @ 2006-01-27 18:29 ` Eli Zaretskii 0 siblings, 0 replies; 32+ messages in thread From: Eli Zaretskii @ 2006-01-27 18:29 UTC (permalink / raw) To: Jim Blandy; +Cc: gdb-patches, andrew.stubbs > Date: Fri, 27 Jan 2006 10:12:36 -0800 > From: Jim Blandy <jimb@red-bean.com> > Cc: gdb-patches@sourceware.org, Andrew STUBBS <andrew.stubbs@st.com> > > I hope I did better than that when I explained why I think > TYPE_ALLOC and TYPE_ZALLOC should be functions. You did. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-27 17:53 ` Eli Zaretskii 2006-01-27 18:12 ` Jim Blandy @ 2006-01-27 18:41 ` Joel Brobecker 1 sibling, 0 replies; 32+ messages in thread From: Joel Brobecker @ 2006-01-27 18:41 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches, Andrew STUBBS, Jim Blandy > > GDB developers seem to have developed an allergy to C macros that > > I just don't understand. They're not _inherently_ obfuscating > > or evil! They can be both useful and elegant. > > FWIW, I never understood the ``macros are bad, m'kay?'' policy, > either. Well, it is clear to me that a unilateral decision against macros might be a bit too extreme. I think that some of the macro usage in GDB could be advantageously replaced with something better, though. I don't have the luxury to always debug on platforms where dwarf2 is available. So I can't print the value of a macro in the debugger. When I debug some code and see a function call in the form of the macro, I can't inspect a function pointer variable, I have to step into the code to follow it. Same for contant macros. You can't take the address of a macro either. Even with dwarf2, I don't know how much people use this new feature that allows us to access macros from the debugger. There are cases where macros a the best solution too. I found the new try/catch mechanism a huge improvement over the catch_error abstraction. A nice additional layer to make programing easier. Same for the ALL_OBJFILE-like macros. They make the code simpler to write without propagating too much the details about the structure. So I welcome the usage of macros. But not without discrimination. -- Joel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-01-22 21:31 ` Daniel Jacobowitz 2006-01-23 22:46 ` Jim Blandy 2006-01-27 17:53 ` Eli Zaretskii @ 2006-02-01 23:14 ` Daniel Jacobowitz 2006-02-02 4:18 ` Eli Zaretskii 2 siblings, 1 reply; 32+ messages in thread From: Daniel Jacobowitz @ 2006-02-01 23:14 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew STUBBS, Jim Blandy On Sun, Jan 22, 2006 at 04:31:18PM -0500, Daniel Jacobowitz wrote: > - gdbint; I couldn't find a vaguely appropriate place to add a > description of this, and I don't think it's such a fascinating > approach that it's worth creating a new section for. Eli convinced me otherwise. Eli, how does the below look? > 2006-01-22 Daniel Jacobowitz <dan@codesourcery.com> > > * Makefile.in (gdbtypes_h, gdbtypes.o, utils.o): Update. > * defs.h (hashtab_obstack_allocate, dummy_obstack_deallocate): Add > prototypes. > * dwarf2read.c (read_subroutine_type): Use TYPE_ZALLOC. > (hashtab_obstack_allocate, dummy_obstack_deallocate): Moved to... > * utils.c (hashtab_obstack_allocate, dummy_obstack_deallocate): > ...here. > * gdbtypes.c: Include "hashtab.h". > (build_gdbtypes): Remove extra prototype. > (struct type_pair, type_pair_hash, type_pair_eq) > (create_copied_types_hash, copy_type_recursive): New. > * gdbtypes.h: Include "hashtab.h". > (TYPE_ZALLOC): New. > (create_copied_types_hash, copy_type_recursive): New prototypes. > * objfiles.c (free_objfile): Call preserve_values. > * symfile.c (reread_symbols): Likewise. > (clear_symtab_users): Remove calls to clear_value_history and > clear_internalvars. > * value.c (clear_value_history, clear_internalvars): Removed. > (preserve_one_value, preserve_values): New functions. > * value.h (clear_value_history, clear_internalvars): Removed. > (preserve_values): New prototype. > > * tracepoint.c (_initialize_tracepoint): Do not initialize convenience > variables here. > > * doc/gdb.texinfo (Files): Remove obsolete bits from the description > of "symbol-file". I have committed this, after retesting it on x86_64-pc-linux-gnu. -- Daniel Jacobowitz CodeSourcery 2006-02-01 Daniel Jacobowitz <dan@codesourcery.com> * gdbint.texinfo (Symbol Handling): Add a section on memory management. Index: doc/gdbint.texinfo =================================================================== RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v retrieving revision 1.237 diff -u -p -r1.237 gdbint.texinfo --- doc/gdbint.texinfo 25 Jan 2006 21:15:42 -0000 1.237 +++ doc/gdbint.texinfo 1 Feb 2006 22:53:51 -0000 @@ -9,7 +9,7 @@ @ifinfo This file documents the internals of the GNU debugger @value{GDBN}. Copyright (C) 1990, 1991, 1992, 1993, 1994, 1996, 1998, 1999, 2000, 2001, - 2002, 2003, 2004, 2005 + 2002, 2003, 2004, 2005, 2006 Free Software Foundation, Inc. Contributed by Cygnus Solutions. Written by John Gilmore. Second Edition by Stan Shebs. @@ -49,7 +49,7 @@ Free Documentation License''. @vskip 0pt plus 1filll Copyright @copyright{} 1990,1991,1992,1993,1994,1996,1998,1999,2000,2001, - 2002, 2003, 2004, 2005 Free Software Foundation, Inc. + 2002, 2003, 2004, 2005, 2006 Free Software Foundation, Inc. Permission is granted to copy, distribute and/or modify this document under the terms of the GNU Free Documentation License, Version 1.1 or @@ -1985,6 +1985,22 @@ will only ever be implemented by one obj directly. This interface should be described in a file @file{bfd/lib@var{xyz}.h}, which is included by @value{GDBN}. +@section Memory Management for Symbol Files + +Most memory associated with a loaded symbol file is stored on +its @code{objfile_obstack}. This includes symbols, types, +namespace data, and other information produced by the symbol readers. + +Because this data lives on the objfile's obstack, it is automatically +released when the objfile is unloaded or reloaded. Therefore one +objfile must not reference symbol or type data from another objfile; +they could be unloaded at different times. + +User convenience variables, et cetera, have associated types. Normally +these types live in the associated objfile. However, when the objfile +is unloaded, those types are deep copied to global memory, so that +the values of the user variables and history items are not lost. + @node Language Support ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-02-01 23:14 ` Daniel Jacobowitz @ 2006-02-02 4:18 ` Eli Zaretskii 2006-02-06 22:14 ` Daniel Jacobowitz 0 siblings, 1 reply; 32+ messages in thread From: Eli Zaretskii @ 2006-02-02 4:18 UTC (permalink / raw) To: gdb-patches > Date: Wed, 1 Feb 2006 18:14:22 -0500 > From: Daniel Jacobowitz <drow@false.org> > Cc: Andrew STUBBS <andrew.stubbs@st.com>, Jim Blandy <jimb@red-bean.com> > > On Sun, Jan 22, 2006 at 04:31:18PM -0500, Daniel Jacobowitz wrote: > > - gdbint; I couldn't find a vaguely appropriate place to add a > > description of this, and I don't think it's such a fascinating > > approach that it's worth creating a new section for. > > Eli convinced me otherwise. Eli, how does the below look? It's fine. Thanks! ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC] Alternate approach to keeping convenience variables 2006-02-02 4:18 ` Eli Zaretskii @ 2006-02-06 22:14 ` Daniel Jacobowitz 0 siblings, 0 replies; 32+ messages in thread From: Daniel Jacobowitz @ 2006-02-06 22:14 UTC (permalink / raw) To: gdb-patches On Thu, Feb 02, 2006 at 06:18:13AM +0200, Eli Zaretskii wrote: > > Date: Wed, 1 Feb 2006 18:14:22 -0500 > > From: Daniel Jacobowitz <drow@false.org> > > Cc: Andrew STUBBS <andrew.stubbs@st.com>, Jim Blandy <jimb@red-bean.com> > > > > On Sun, Jan 22, 2006 at 04:31:18PM -0500, Daniel Jacobowitz wrote: > > > - gdbint; I couldn't find a vaguely appropriate place to add a > > > description of this, and I don't think it's such a fascinating > > > approach that it's worth creating a new section for. > > > > Eli convinced me otherwise. Eli, how does the below look? > > It's fine. Thanks! Thanks, I've checked it in. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2006-02-06 22:14 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2005-11-21 22:05 [PATCH] keeping convenience variables (take 2) Andrew STUBBS 2005-11-22 4:58 ` Eli Zaretskii 2005-11-22 8:43 ` Jim Blandy 2005-11-22 19:27 ` Andrew STUBBS 2005-12-10 4:46 ` [RFC] Alternate approach to keeping convenience variables Daniel Jacobowitz 2005-12-10 5:07 ` Jim Blandy 2005-12-10 8:24 ` Daniel Jacobowitz 2005-12-10 22:20 ` Jim Blandy 2005-12-11 18:12 ` Eli Zaretskii 2005-12-11 19:17 ` Eli Zaretskii 2006-01-23 22:29 ` Jim Blandy 2006-01-24 11:44 ` Andrew STUBBS 2006-01-24 18:41 ` Jim Blandy 2006-01-24 18:43 ` Daniel Jacobowitz 2006-01-24 19:16 ` Jim Blandy 2006-01-24 19:24 ` Daniel Jacobowitz 2006-01-25 0:13 ` Jim Blandy 2006-01-24 19:45 ` Andrew STUBBS 2006-01-04 12:17 ` Andrew STUBBS 2006-01-04 17:00 ` Jim Blandy 2006-01-04 17:48 ` Andrew STUBBS 2006-01-04 18:37 ` Jim Blandy 2006-01-22 21:04 ` Daniel Jacobowitz 2006-01-22 21:31 ` Daniel Jacobowitz 2006-01-23 22:46 ` Jim Blandy 2006-01-27 17:53 ` Eli Zaretskii 2006-01-27 18:12 ` Jim Blandy 2006-01-27 18:29 ` Eli Zaretskii 2006-01-27 18:41 ` Joel Brobecker 2006-02-01 23:14 ` Daniel Jacobowitz 2006-02-02 4:18 ` Eli Zaretskii 2006-02-06 22:14 ` Daniel Jacobowitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox