[RFA,21/42] Move the context stack to buildsym_compunit
Commit Message
This moves the context stack globals to be members of
buildsym_compunit, changing the type to a std::vector in the process.
Note that the context stack works in a somewhat odd way, where callers
can pop the stack and expect the returned pointer to remain valid
until the next pop. In the new code we handle this by having a
"popped context" object that remains valid in this way.
gdb/ChangeLog
2018-05-22 Tom Tromey <tom@tromey.com>
* dwarf2read.c (new_symbol): Update.
* dbxread.c (process_one_symbol): Update.
* buildsym.h (context_stack, context_stack_depth): Don't declare.
(outermost_context_p): Remove macro.
(outermost_context_p, get_current_context_stack)
(get_context_stack_depth): Declare.
* buildsym.c (struct buildsym_compunit) <m_context_stack,
m_popped_context>: New members.
(context_stack_size): Remove.
(INITIAL_CONTEXT_STACK_SIZE): Remove.
(prepare_for_building, end_symtab_get_static_block)
(augment_type_symtab, push_context, pop_context): Update.
(outermost_context_p, get_current_context_stack)
(get_context_stack_depth): New functions.
(buildsym_init): Update.
---
gdb/ChangeLog | 18 +++++++++++++
gdb/buildsym.c | 82 ++++++++++++++++++++++++++++++++------------------------
gdb/buildsym.h | 22 ++++++++-------
gdb/dbxread.c | 4 +--
gdb/dwarf2read.c | 40 ++++++++++++++-------------
5 files changed, 101 insertions(+), 65 deletions(-)
Comments
On 05/22/2018 09:58 PM, Tom Tromey wrote:
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index 4e3ca5e4b1..54592b7795 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -213,6 +213,9 @@ struct buildsym_compunit
> /* global "using" directives. */
>
> struct using_direct *m_global_using_directives = nullptr;
> +
> + std::vector<struct context_stack> m_context_stack;
> + struct context_stack m_popped_context {};
> };
Explanatory comments -- especially for m_popped_context? Had I not read the commit log, I would not have (as easily) understood why this was necessary.
> /* The work-in-progress of the compunit we are building.
> @@ -1583,12 +1579,9 @@ augment_type_symtab (void)
> struct compunit_symtab *cust = buildsym_compunit->compunit_symtab;
> const struct blockvector *blockvector = COMPUNIT_BLOCKVECTOR (cust);
>
> - if (context_stack_depth > 0)
> - {
> - complaint (&symfile_complaints,
> - _("Context stack not empty in augment_type_symtab"));
> - context_stack_depth = 0;
> - }
> + if (!buildsym_compunit->m_context_stack.empty ())
> + complaint (&symfile_complaints,
> + _("Context stack not empty in augment_type_symtab"));
> if (pending_blocks != NULL)
> complaint (&symfile_complaints, _("Blocks in a type symtab"));
> if (buildsym_compunit->m_pending_macros != NULL)
Warning: This patch (and one or two others) need rebasing. symfile_complaints was removed in May.
> @@ -1745,6 +1736,35 @@ get_global_using_directives ()
> return &buildsym_compunit->m_global_using_directives;
> }
>
> +/* See buildsym.h. */
> +
> +bool
> +outermost_context_p ()
> +{
> + gdb_assert (buildsym_compunit != nullptr);
> + return buildsym_compunit->m_context_stack.empty ();
> +}
> +
> +/* See buildsym.h. */
> +
> +struct context_stack *
> +get_current_context_stack ()
> +{
> + gdb_assert (buildsym_compunit != nullptr);
> + if (buildsym_compunit->m_context_stack.empty ())
> + return nullptr;
> + return &buildsym_compunit->m_context_stack.back ();
> +}
> +
> +/* See buildsym.h. */
> +
> +int
> +get_context_stack_depth ()
> +{
> + gdb_assert (buildsym_compunit != nullptr);
> + return buildsym_compunit->m_context_stack.size ();
> +}
> +
I can't help but think these function names, which could appear in several source files, seem a little vague or collision-prone (conceptually)... Would it be worth it for these (and perhaps all similar buildsym.h get_* functions) to be namespaced or further prefixed, e.g., buildsym_[get_]current_context_stack or buildsym::get_current_context_stack? I'm not going to suggest [Dare I say, "require?"] any changes -- I'm just wondering "aloud."
> /* Initialize anything that needs initializing when starting to read a
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index efb35c907b..fe158d183c 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -270,6 +261,19 @@ extern void set_local_using_directives (struct using_direct *new_local);
>
> extern struct using_direct **get_global_using_directives ();
>
> +/* Non-zero if the context stack is empty. */
^^^^^^^^
> +
> +extern bool outermost_context_p ();
Since this (now) returns bool, please use boolean in comment. [I realize that's a cut-n-paste-o.]
> +
> +/* Return the top of the context stack, or nullptr if there is
> + entry. */
> +
> +extern struct context_stack *get_current_context_stack ();
> +
Typo in "if there is entry."
> +/* Return the context stack depth. */
> +
> +extern int get_context_stack_depth ();
> +
> #undef EXTERN
>
> #endif /* defined (BUILDSYM_H) */
Thank you!
[Just a gentle reminder: IANAM.]
Keith
On 2018-07-08 01:49 PM, Simon Marchi wrote:
> On 2018-05-23 12:58 AM, Tom Tromey wrote:
>> This moves the context stack globals to be members of
>> buildsym_compunit, changing the type to a std::vector in the process.
>>
>> Note that the context stack works in a somewhat odd way, where callers
>> can pop the stack and expect the returned pointer to remain valid
>> until the next pop. In the new code we handle this by having a
>> "popped context" object that remains valid in this way.
>
> Would it work to make pop_context return the context_stack object by value
> instead?
>
>> @@ -270,6 +261,19 @@ extern void set_local_using_directives (struct using_direct *new_local);
>>
>> extern struct using_direct **get_global_using_directives ();
>>
>> +/* Non-zero if the context stack is empty. */
>> +
>> +extern bool outermost_context_p ();
>
> "True" instead of "Non-zero".
>
> Simon
Damn, failed to reply all again.
Simon
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
Keith> I can't help but think these function names, which could appear in
Keith> several source files, seem a little vague or collision-prone
Keith> (conceptually)... Would it be worth it for these (and perhaps all
Keith> similar buildsym.h get_* functions) to be namespaced or further
Keith> prefixed, e.g., buildsym_[get_]current_context_stack or
Keith> buildsym::get_current_context_stack? I'm not going to suggest [Dare I
Keith> say, "require?"] any changes -- I'm just wondering "aloud."
I think it would be a reasonable follow-up patch; though seeing that
these things are all in the legacy API, I guess I'd be inclined not to
bother.
>> +/* Non-zero if the context stack is empty. */
Keith> ^^^^^^^^
>> +
>> +extern bool outermost_context_p ();
Keith> Since this (now) returns bool, please use boolean in comment.
Fixed.
>> +
>> +/* Return the top of the context stack, or nullptr if there is
>> + entry. */
>> +
>> +extern struct context_stack *get_current_context_stack ();
>> +
Keith> Typo in "if there is entry."
Fixed.
Tom
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
Simon> Would it work to make pop_context return the context_stack object by value
Simon> instead?
Nice idea, I've made this change in the new version.
Tom
@@ -213,6 +213,9 @@ struct buildsym_compunit
/* global "using" directives. */
struct using_direct *m_global_using_directives = nullptr;
+
+ std::vector<struct context_stack> m_context_stack;
+ struct context_stack m_popped_context {};
};
/* The work-in-progress of the compunit we are building.
@@ -260,10 +263,6 @@ struct pending_block
static struct pending_block *pending_blocks;
-/* Currently allocated size of context stack. */
-
-static int context_stack_size;
-
static void free_buildsym_compunit (void);
static int compare_line_numbers (const void *ln1p, const void *ln2p);
@@ -278,7 +277,6 @@ static void free_pending_blocks ();
needed, and realloc'd down to the size actually used, when
completed. */
-#define INITIAL_CONTEXT_STACK_SIZE 10
#define INITIAL_LINE_VECTOR_LENGTH 1000
@@ -1032,8 +1030,6 @@ prepare_for_building ()
{
local_symbols = NULL;
- context_stack_depth = 0;
-
/* These should have been reset either by successful completion of building
a symtab, or by the scoped_free_pendings destructor. */
gdb_assert (file_symbols == NULL);
@@ -1222,7 +1218,7 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
/* Finish the lexical context of the last function in the file; pop
the context stack. */
- if (context_stack_depth > 0)
+ if (!buildsym_compunit->m_context_stack.empty ())
{
struct context_stack *cstk = pop_context ();
@@ -1230,7 +1226,7 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
finish_block (cstk->name, &local_symbols, cstk->old_blocks, NULL,
cstk->start_addr, end_addr);
- if (context_stack_depth > 0)
+ if (!buildsym_compunit->m_context_stack.empty ())
{
/* This is said to happen with SCO. The old coffread.c
code simply emptied the context stack, so we do the
@@ -1239,7 +1235,7 @@ end_symtab_get_static_block (CORE_ADDR end_addr, int expandable, int required)
it used to be an abort(). */
complaint (&symfile_complaints,
_("Context stack not empty in end_symtab"));
- context_stack_depth = 0;
+ buildsym_compunit->m_context_stack.clear ();
}
}
@@ -1583,12 +1579,9 @@ augment_type_symtab (void)
struct compunit_symtab *cust = buildsym_compunit->compunit_symtab;
const struct blockvector *blockvector = COMPUNIT_BLOCKVECTOR (cust);
- if (context_stack_depth > 0)
- {
- complaint (&symfile_complaints,
- _("Context stack not empty in augment_type_symtab"));
- context_stack_depth = 0;
- }
+ if (!buildsym_compunit->m_context_stack.empty ())
+ complaint (&symfile_complaints,
+ _("Context stack not empty in augment_type_symtab"));
if (pending_blocks != NULL)
complaint (&symfile_complaints, _("Blocks in a type symtab"));
if (buildsym_compunit->m_pending_macros != NULL)
@@ -1629,17 +1622,11 @@ augment_type_symtab (void)
struct context_stack *
push_context (int desc, CORE_ADDR valu)
{
- struct context_stack *newobj;
+ gdb_assert (buildsym_compunit != nullptr);
- if (context_stack_depth == context_stack_size)
- {
- context_stack_size *= 2;
- context_stack = (struct context_stack *)
- xrealloc ((char *) context_stack,
- (context_stack_size * sizeof (struct context_stack)));
- }
+ buildsym_compunit->m_context_stack.emplace_back ();
+ struct context_stack *newobj = &buildsym_compunit->m_context_stack.back ();
- newobj = &context_stack[context_stack_depth++];
newobj->depth = desc;
newobj->locals = local_symbols;
newobj->old_blocks = pending_blocks;
@@ -1660,8 +1647,12 @@ push_context (int desc, CORE_ADDR valu)
struct context_stack *
pop_context (void)
{
- gdb_assert (context_stack_depth > 0);
- return (&context_stack[--context_stack_depth]);
+ gdb_assert (buildsym_compunit != nullptr);
+ gdb_assert (!buildsym_compunit->m_context_stack.empty ());
+ buildsym_compunit->m_popped_context
+ = buildsym_compunit->m_context_stack.back ();
+ buildsym_compunit->m_context_stack.pop_back ();
+ return &buildsym_compunit->m_popped_context;
}
@@ -1745,6 +1736,35 @@ get_global_using_directives ()
return &buildsym_compunit->m_global_using_directives;
}
+/* See buildsym.h. */
+
+bool
+outermost_context_p ()
+{
+ gdb_assert (buildsym_compunit != nullptr);
+ return buildsym_compunit->m_context_stack.empty ();
+}
+
+/* See buildsym.h. */
+
+struct context_stack *
+get_current_context_stack ()
+{
+ gdb_assert (buildsym_compunit != nullptr);
+ if (buildsym_compunit->m_context_stack.empty ())
+ return nullptr;
+ return &buildsym_compunit->m_context_stack.back ();
+}
+
+/* See buildsym.h. */
+
+int
+get_context_stack_depth ()
+{
+ gdb_assert (buildsym_compunit != nullptr);
+ return buildsym_compunit->m_context_stack.size ();
+}
+
/* Initialize anything that needs initializing when starting to read a
@@ -1756,14 +1776,6 @@ buildsym_init ()
{
pending_addrmap_interesting = 0;
- /* Context stack is initially empty. Allocate first one with room
- for a few levels; reuse it forever afterward. */
- if (context_stack == NULL)
- {
- context_stack_size = INITIAL_CONTEXT_STACK_SIZE;
- context_stack = XNEWVEC (struct context_stack, context_stack_size);
- }
-
/* Ensure the scoped_free_pendings destructor was called after
the last time. */
gdb_assert (free_pendings == NULL);
@@ -133,15 +133,6 @@ struct context_stack
};
-EXTERN struct context_stack *context_stack;
-
-/* Index of first unused entry in context stack. */
-
-EXTERN int context_stack_depth;
-
-/* Non-zero if the context stack is empty. */
-#define outermost_context_p() (context_stack_depth == 0)
-
/* The type of the record_line function. */
typedef void (record_line_ftype) (struct subfile *subfile, int line,
CORE_ADDR pc);
@@ -270,6 +261,19 @@ extern void set_local_using_directives (struct using_direct *new_local);
extern struct using_direct **get_global_using_directives ();
+/* Non-zero if the context stack is empty. */
+
+extern bool outermost_context_p ();
+
+/* Return the top of the context stack, or nullptr if there is
+ entry. */
+
+extern struct context_stack *get_current_context_stack ();
+
+/* Return the context stack depth. */
+
+extern int get_context_stack_depth ();
+
#undef EXTERN
#endif /* defined (BUILDSYM_H) */
@@ -2648,7 +2648,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
}
local_symbols = newobj->locals;
- if (context_stack_depth > 1)
+ if (get_context_stack_depth () > 1)
{
/* This is not the outermost LBRAC...RBRAC pair in the
function, its local symbols preceded it, and are the ones
@@ -2942,7 +2942,7 @@ process_one_symbol (int type, int desc, CORE_ADDR valu, const char *name,
within_function = 1;
- if (context_stack_depth > 1)
+ if (get_context_stack_depth () > 1)
{
complaint (&symfile_complaints,
_("unmatched N_LBRAC before symtab pos %d"),
@@ -21467,26 +21467,28 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
}
break;
case DW_TAG_formal_parameter:
- /* If we are inside a function, mark this as an argument. If
- not, we might be looking at an argument to an inlined function
- when we do not have enough information to show inlined frames;
- pretend it's a local variable in that case so that the user can
- still see it. */
- if (!outermost_context_p ()
- && context_stack[context_stack_depth - 1].name != NULL)
- SYMBOL_IS_ARGUMENT (sym) = 1;
- attr = dwarf2_attr (die, DW_AT_location, cu);
- if (attr)
- {
- var_decode_location (attr, sym, cu);
- }
- attr = dwarf2_attr (die, DW_AT_const_value, cu);
- if (attr)
- {
- dwarf2_const_value (attr, sym, cu);
- }
+ {
+ /* If we are inside a function, mark this as an argument. If
+ not, we might be looking at an argument to an inlined function
+ when we do not have enough information to show inlined frames;
+ pretend it's a local variable in that case so that the user can
+ still see it. */
+ struct context_stack *curr = get_current_context_stack ();
+ if (curr != nullptr && curr->name != nullptr)
+ SYMBOL_IS_ARGUMENT (sym) = 1;
+ attr = dwarf2_attr (die, DW_AT_location, cu);
+ if (attr)
+ {
+ var_decode_location (attr, sym, cu);
+ }
+ attr = dwarf2_attr (die, DW_AT_const_value, cu);
+ if (attr)
+ {
+ dwarf2_const_value (attr, sym, cu);
+ }
- list_to_add = cu->list_in_scope;
+ list_to_add = cu->list_in_scope;
+ }
break;
case DW_TAG_unspecified_parameters:
/* From varargs functions; gdb doesn't seem to have any