[RFA 1/5] Explicit linespecs - refactor linespec parsing
Jan Kratochvil
jan.kratochvil@redhat.com
Mon Jul 30 04:45:00 GMT 2012
Hi Keith,
I do not see a reason for the virtualization via 'parse_it' field. It is
called only by do_decode_line and it is the first thing do_decode_line does.
Also the direct callers of do_decode_line know the virtualized function
instance. Therefore the callers of do_decode_line can just pass the result
from the virtualization function, without any virtualization at all.
That means 'struct ls_parser' is "parser state", not "parser definition".
I have attached such simplification proof-of-concept.
On Fri, 27 Jul 2012 05:46:29 +0200, Keith Seitz wrote:
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -265,6 +265,12 @@ typedef struct ls_token linespec_token;
> #define LS_TOKEN_STOKEN(TOK) (TOK).data.string
> #define LS_TOKEN_KEYWORD(TOK) (TOK).data.keyword
>
> +/* A parser function definition used by do_decode_line. */
> +
> +struct ls_parser;
> +typedef struct symtabs_and_lines (*linespec_parse_func) (struct ls_parser *,
> + void *client_data);
GDB uses more *_ftype. And it is function type, not pointer to function type.
> +
> /* An instance of the linespec parser. */
>
> struct ls_parser
> @@ -293,6 +299,9 @@ struct ls_parser
> /* The result of the parse. */
> struct linespec result;
> #define PARSER_RESULT(PPTR) (&(PPTR)->result)
> +
> + /* The function to actually do the parsing. */
> + linespec_parse_func parse_it;
> };
> typedef struct ls_parser linespec_parser;
>
> @@ -1433,6 +1442,29 @@ unexpected_linespec_error (linespec_parser *parser)
> token_type_strings[token.type]);
> }
>
> +/* Throw an undefined label error. */
> +
> +static void ATTRIBUTE_NORETURN
> +undefined_label_error (const char *function, const char *label)
> +{
> + if (function != NULL)
> + throw_error (NOT_FOUND_ERROR,
> + _("No label \"%s\" defined in function \"%s\"."),
> + label, function);
Use tabs for indentation, not spaces. On more places in this patch. Also the
indentation with spaces is missing one space so there probably formerly were
tabs.
> + else
> + throw_error (NOT_FOUND_ERROR,
> + _("No label \"%s\" defined in current function."), label);
> +}
> +
> +/* Throw a source file not found error. */
> +
> +static void ATTRIBUTE_NORETURN
> +source_file_not_found_error (const char *name)
> +{
> + throw_error (NOT_FOUND_ERROR,
> + _("No source file named %s."), name);
\"%s\", not %s.
> +}
> +
> /* Parse and return a line offset in STRING. */
>
> static struct line_offset
> @@ -1587,9 +1619,8 @@ linespec_parse_basic (linespec_parser *parser)
> else
> {
> /* We don't know what it was, but it isn't a label. */
> - throw_error (NOT_FOUND_ERROR,
> - _("No label \"%s\" defined in function \"%s\"."),
> - name, PARSER_RESULT (parser)->function_name);
> + undefined_label_error (PARSER_RESULT (parser)->function_name,
> + name);
> }
>
> /* Check for a line offset. */
> @@ -1995,15 +2026,16 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
> if no file is validly specified. Callers must check that.
> Also, the line number returned may be invalid. */
>
> -/* Parse the linespec in ARGPTR. */
> +/* Parse the linespec in DATA. */
>
> static struct symtabs_and_lines
> -parse_linespec (linespec_parser *parser, char **argptr)
> +parse_linespec (linespec_parser *parser, void *data)
> {
> linespec_token token;
> struct symtabs_and_lines values;
> volatile struct gdb_exception file_exception;
> struct cleanup *cleanup;
> + char **argptr = (char **) data;
const?
>
> /* A special case to start. It has become quite popular for
> IDEs to work around bugs in the previous parser by quoting
> @@ -2210,7 +2242,7 @@ linespec_state_constructor (struct linespec_state *self,
> /* Initialize a new linespec parser. */
>
> static void
> -linespec_parser_new (linespec_parser *parser,
> +linespec_parser_new (linespec_parser *parser, linespec_parse_func func,
> int flags, const struct language_defn *language,
> struct symtab *default_symtab,
> int default_line,
> @@ -2219,6 +2251,7 @@ linespec_parser_new (linespec_parser *parser,
> parser->lexer.current.type = LSTOKEN_CONSUMED;
> memset (PARSER_RESULT (parser), 0, sizeof (struct linespec));
> PARSER_RESULT (parser)->line_offset.sign = LINE_OFFSET_UNKNOWN;
> + parser->parse_it = func;
> linespec_state_constructor (PARSER_STATE (parser), flags, language,
> default_symtab, default_line, canonical);
> }
> @@ -2261,42 +2294,35 @@ linespec_parser_delete (void *arg)
> linespec_state_destructor (PARSER_STATE (parser));
> }
>
> -/* See linespec.h. */
> +/* Decode the linespec given in DATA with the PARSER.
> + SELECT_MODE and FILTER are from decode_line_full. */
>
> -void
> -decode_line_full (char **argptr, int flags,
> - struct symtab *default_symtab,
> - int default_line, struct linespec_result *canonical,
> - const char *select_mode,
> - const char *filter)
> +static void
> +do_decode_line (linespec_parser *parser, void *data,
> + const char *select_mode, const char *filter)
> {
> struct symtabs_and_lines result;
> struct cleanup *cleanups;
> - char *arg_start = *argptr;
> VEC (const_char_ptr) *filters = NULL;
> - linespec_parser parser;
> struct linespec_state *state;
>
> - gdb_assert (canonical != NULL);
> /* The filter only makes sense for 'all'. */
> gdb_assert (filter == NULL || select_mode == multiple_symbols_all);
> gdb_assert (select_mode == NULL
> || select_mode == multiple_symbols_all
> || select_mode == multiple_symbols_ask
> || select_mode == multiple_symbols_cancel);
> - gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0);
>
> - linespec_parser_new (&parser, flags, current_language, default_symtab,
> - default_line, canonical);
> - cleanups = make_cleanup (linespec_parser_delete, &parser);
> + cleanups = make_cleanup (null_cleanup, NULL);
> save_current_program_space ();
You can do:
cleanups = save_current_program_space ();
>
> - result = parse_linespec (&parser, argptr);
> - state = PARSER_STATE (&parser);
> + result = (*parser->parse_it) (parser, data);
> + state = PARSER_STATE (parser);
>
> - gdb_assert (result.nelts == 1 || canonical->pre_expanded);
> - gdb_assert (canonical->addr_string != NULL);
> - canonical->pre_expanded = 1;
> + gdb_assert (result.nelts == 1 || state->canonical->pre_expanded);
> + gdb_assert (state->canonical->addr_string != NULL);
> + gdb_assert (result.nelts == 0 || state->canonical_names != NULL);
> + state->canonical->pre_expanded = 1;
>
> /* Arrange for allocated canonical names to be freed. */
> if (result.nelts > 0)
> @@ -2336,6 +2362,29 @@ decode_line_full (char **argptr, int flags,
> do_cleanups (cleanups);
> }
>
> +/* See linespec.h.
> + All of the real work is done in do_decode_line. */
The split is incomplete, for example in this part there is nothing in
linespec.h, such way one has to check the whole patchset to review just this
part.
> +
> +void
> +decode_line_full (char **argptr, int flags,
> + struct symtab *default_symtab,
> + int default_line, struct linespec_result *canonical,
> + const char *select_mode,
> + const char *filter)
> +{
> + linespec_parser parser;
> + struct cleanup *cleanups;
> +
> + gdb_assert (canonical != NULL);
> + gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0);
> +
> + linespec_parser_new (&parser, parse_linespec, flags, current_language,
> + default_symtab, default_line, canonical);
> + cleanups = make_cleanup (linespec_parser_delete, &parser);
> + do_decode_line (&parser, argptr, select_mode, filter);
> + do_cleanups (cleanups);
> +}
> +
> /* See linespec.h. */
>
> struct symtabs_and_lines
> @@ -2347,12 +2396,12 @@ decode_line_1 (char **argptr, int flags,
> linespec_parser parser;
> struct cleanup *cleanups;
>
> - linespec_parser_new (&parser, flags, current_language, default_symtab,
> - default_line, NULL);
> + linespec_parser_new (&parser, parse_linespec, flags, current_language,
> + default_symtab, default_line, NULL);
> cleanups = make_cleanup (linespec_parser_delete, &parser);
> save_current_program_space ();
>
> - result = parse_linespec (&parser, argptr);
> + result = (*parser.parse_it) (&parser, argptr);
>
> do_cleanups (cleanups);
> return result;
> @@ -2879,7 +2928,7 @@ symtabs_from_filename (const char *filename)
> throw_error (NOT_FOUND_ERROR,
> _("No symbol table is loaded. "
> "Use the \"file\" command."));
> - throw_error (NOT_FOUND_ERROR, _("No source file named %s."), filename);
> + source_file_not_found_error (filename);
> }
>
> return result;
Thanks,
Jan
-------------- next part --------------
diff --git a/gdb/linespec.c b/gdb/linespec.c
index fcbf3dd..5539908 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -265,12 +265,6 @@ typedef struct ls_token linespec_token;
#define LS_TOKEN_STOKEN(TOK) (TOK).data.string
#define LS_TOKEN_KEYWORD(TOK) (TOK).data.keyword
-/* A parser function definition used by do_decode_line. */
-
-struct ls_parser;
-typedef struct symtabs_and_lines (*linespec_parse_func) (struct ls_parser *,
- void *client_data);
-
/* An instance of the linespec parser. */
struct ls_parser
@@ -299,9 +293,6 @@ struct ls_parser
/* The result of the parse. */
struct linespec result;
#define PARSER_RESULT(PPTR) (&(PPTR)->result)
-
- /* The function to actually do the parsing. */
- linespec_parse_func parse_it;
};
typedef struct ls_parser linespec_parser;
@@ -2041,13 +2032,12 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
/* Parse the linespec in DATA. */
static struct symtabs_and_lines
-parse_linespec (linespec_parser *parser, void *data)
+parse_linespec (linespec_parser *parser, char **argptr)
{
linespec_token token;
struct symtabs_and_lines values;
volatile struct gdb_exception file_exception;
struct cleanup *cleanup;
- char **argptr = (char **) data;
/* A special case to start. It has become quite popular for
IDEs to work around bugs in the previous parser by quoting
@@ -2254,7 +2244,7 @@ linespec_state_constructor (struct linespec_state *self,
/* Initialize a new linespec parser. */
static void
-linespec_parser_new (linespec_parser *parser, linespec_parse_func func,
+linespec_parser_new (linespec_parser *parser,
int flags, const struct language_defn *language,
struct symtab *default_symtab,
int default_line,
@@ -2263,7 +2253,6 @@ linespec_parser_new (linespec_parser *parser, linespec_parse_func func,
parser->lexer.current.type = LSTOKEN_CONSUMED;
memset (PARSER_RESULT (parser), 0, sizeof (struct linespec));
PARSER_RESULT (parser)->line_offset.sign = LINE_OFFSET_UNKNOWN;
- parser->parse_it = func;
linespec_state_constructor (PARSER_STATE (parser), flags, language,
default_symtab, default_line, canonical);
}
@@ -2310,10 +2299,9 @@ linespec_parser_delete (void *arg)
SELECT_MODE and FILTER are from decode_line_full. */
static void
-do_decode_line (linespec_parser *parser, void *data,
+do_decode_line (linespec_parser *parser, struct symtabs_and_lines result,
const char *select_mode, const char *filter)
{
- struct symtabs_and_lines result;
struct cleanup *cleanups;
VEC (const_char_ptr) *filters = NULL;
struct linespec_state *state;
@@ -2328,7 +2316,6 @@ do_decode_line (linespec_parser *parser, void *data,
cleanups = make_cleanup (null_cleanup, NULL);
save_current_program_space ();
- result = (*parser->parse_it) (parser, data);
state = PARSER_STATE (parser);
gdb_assert (result.nelts == 1 || state->canonical->pre_expanded);
@@ -2386,25 +2373,26 @@ decode_line_full (char **argptr, int flags,
{
linespec_parser parser;
struct cleanup *cleanups;
+ struct symtabs_and_lines sal;
gdb_assert (canonical != NULL);
gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0);
- linespec_parser_new (&parser, parse_linespec, flags, current_language,
+ linespec_parser_new (&parser, flags, current_language,
default_symtab, default_line, canonical);
cleanups = make_cleanup (linespec_parser_delete, &parser);
- do_decode_line (&parser, argptr, select_mode, filter);
+ sal = parse_linespec (&parser, argptr);
+ do_decode_line (&parser, sal, select_mode, filter);
do_cleanups (cleanups);
}
/* A parsing function for explicit linespecs. */
static struct symtabs_and_lines
-parse_explicit_linespec (linespec_parser *parser, void *data)
+parse_explicit_linespec (linespec_parser *parser, explicit_linespec *els)
{
VEC (symbolp) *symbols, *labels;
VEC (minsym_and_objfile_d) *minimal_symbols;
- explicit_linespec *els = (explicit_linespec *) data;
if (els->expression != NULL)
{
@@ -2504,15 +2492,17 @@ decode_explicit_linespec (explicit_linespec *els, int flags,
{
linespec_parser parser;
struct cleanup *cleanups;
+ struct symtabs_and_lines sal;
gdb_assert (canonical != NULL);
gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0);
- linespec_parser_new (&parser, parse_explicit_linespec, flags,
+ linespec_parser_new (&parser, flags,
current_language, default_symtab,
default_line, canonical);
cleanups = make_cleanup (linespec_parser_delete, &parser);
- do_decode_line (&parser, els, select_mode, filter);
+ sal = parse_explicit_linespec (&parser, els);
+ do_decode_line (&parser, sal, select_mode, filter);
do_cleanups (cleanups);
}
@@ -2527,12 +2517,12 @@ decode_line_1 (char **argptr, int flags,
linespec_parser parser;
struct cleanup *cleanups;
- linespec_parser_new (&parser, parse_linespec, flags, current_language,
+ linespec_parser_new (&parser, flags, current_language,
default_symtab, default_line, NULL);
cleanups = make_cleanup (linespec_parser_delete, &parser);
save_current_program_space ();
- result = (*parser.parse_it) (&parser, argptr);
+ result = parse_linespec (&parser, argptr);
do_cleanups (cleanups);
return result;
More information about the Gdb-patches
mailing list