This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Bug 17394: we cannot put a break-point at a global function for ASM file
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Mihail-Marian Nistor <mihail dot nistor at freescale dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Mon, 15 Sep 2014 16:44:28 -0400
- Subject: Re: [PATCH] Bug 17394: we cannot put a break-point at a global function for ASM file
- Authentication-results: sourceware.org; auth=none
- References: <1410790918-18986-1-git-send-email-mihail dot nistor at freescale dot com>
On Monday, September 15 2014, Mihail-Marian Nistor wrote:
> We need to cover the following test case: the user wants to do an action
> only for the function that was defined into a selected file name.
> An example: the user wants to put a breakpoint only for functions "func"
> that was defined in the file name "file.s"
> e.i. of gdb command line: b file.s:func
> Due to the limitation that the GAS doesn't generate debug info for
> functions/symbols, we cannot find the function information if we look only
> in file symbtabs that was collected by using the file name specified by the user.
> We need to look into a global default symtab if we want to find minimal
> information about functions that were written in the ASM file.
> And after that, we need to select only functions that were defined into
> the file name specified by the user.
Hi Mihail-Marian,
Thanks for the patch. I have a few comments about the coding style.
They should be easy to fix, and this is not a technical review (which I
intend to do later).
> gdb/ChangeLog
> 2014-09-15 Mihail-Marian Nistor <mihail.nistor@freescale.com>
> * linespec.c (minsym_found): Add new "linespec_p" argument. Update all callers.
> (maybe_same_source_file) New function.
>
> Signed-off-by: Mihail-Marian Nistor <mihail.nistor@freescale.com>
> ---
> gdb/linespec.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 118 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 8a2c8e3..e6b5ad7 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -363,8 +363,11 @@ static void decode_digits_list_mode (struct linespec_state *self,
> struct symtabs_and_lines *values,
> struct symtab_and_line val);
>
> +static int maybe_same_source_file (struct symtab_and_line* sal, linespec_p ls);
> +
> static void minsym_found (struct linespec_state *self, struct objfile *objfile,
> struct minimal_symbol *msymbol,
> + linespec_p ls,
> struct symtabs_and_lines *result);
>
> static int compare_symbols (const void *a, const void *b);
> @@ -1582,7 +1585,7 @@ linespec_parse_basic (linespec_parser *parser)
> VEC (symbolp) *symbols, *labels;
> VEC (bound_minimal_symbol_d) *minimal_symbols;
> struct cleanup *cleanup;
> -
> +
This only adds a whitespace, please remove it.
> /* Get the next token. */
> token = linespec_lexer_lex_one (parser);
>
> @@ -1625,8 +1628,74 @@ linespec_parse_basic (linespec_parser *parser)
>
> /* Try looking it up as a function/method. */
> find_linespec_symbols (PARSER_STATE (parser),
> - PARSER_RESULT (parser)->file_symtabs, name,
> - &symbols, &minimal_symbols);
> + PARSER_RESULT (parser)->file_symtabs,
> + name, &symbols, &minimal_symbols);
I don't understand why you reorganized the code here. It is not
necessary and doesn't relate to this patch, so this modification should
be removed as well.
> +
> + /* We need to cover the following test case: the user wants to do an action
> + only for the function that was defined into a selected file name.
> + An example: the user wants to put a breakpoint only for functions "func"
> + that was defined in the file name "file.s"
> + e.i. of gdb command line: b file.s:func
> + Due to the limitation that the GAS doesn't generate debug info for functions/symbols,
> + we cannot find the function information if we look only in file symbtabs that was collected
> + by using the file name specified by the user. We need to look into a global default symtab
> + if we want to find minimal information about functions that were written in the ASM file.
> + And after that, we need to select only functions that were defined into the file name
> + specified by the user. We do this action into the mimi_found in the minsym_found function */
I appreciate the comment explaining the problem, but it is not correctly
formatted. Lines shouldn't be longer than 80 characters (76 characters
is a soft limit, 80 is the hard limit), so it would be nice if you could
rewrite/reformat the comment to obey this.
> +
> + /* Verify if the user has specified a file name that was used to build the current file symtabs. */
> + if (PARSER_RESULT (parser)->source_filename)
Please check explicitly against NULL here:
if (PARSER_RESULT (parser)->source_filename != NULL)
> + {
> + VEC (symtab_ptr) * file_symtabs = NULL;
> +
> + int ix;
> + struct symtab *elt;
> + int global_symtabs = 0;
No space between "*" and the variable name:
VEC (symtab_ptr) *file_symtabs = NULL;
Also, no blank line between the variables.
> +
> + /* Verify if we have already used the global default symtab information to find the function/method. */
> + for (ix = 0; VEC_iterate (symtab_ptr, PARSER_RESULT (parser)->file_symtabs, ix, elt); ++ix)
> + {
> + VEC_safe_push (symtab_ptr, file_symtabs, elt);
> + if (elt == NULL)
> + global_symtabs = 1;
> + }
> +
> + if (!global_symtabs)
> + {
> + VEC_safe_push (symtab_ptr, file_symtabs, NULL);
> + }
No need for the { } when you have just one statement.
You are using a strange indentation pattern here. In some places, you
are using the GNU Coding Style, which says that, if you have 8 spaces,
you should replace them with a TAB. In other places, you are just using
spaces. This goes through the file, and makes it a bit hard to read.
You should use the GNU Coding Style in the whole file :-).
> +
> + if (file_symtabs)
> + {
> + int ims;
> + bound_minimal_symbol_d *minsym;
> +
> + VEC (symbolp) *symbols1;
> + VEC (bound_minimal_symbol_d) *minimal_symbols1;
No newline between variables declarations. Also, I really prefer if you
don't just append "1" to the names of the variables. Choose a
meaningful name instead, please.
> +
> + find_linespec_symbols (PARSER_STATE (parser),
> + file_symtabs,
> + name, &symbols1, &minimal_symbols1);
> +
> + /* We should ignore information about function/method that were found by using .debug_info information,
> + because this information was already stored into the symbols vector. */
Lines too long.
> + if (symbols1)
Check explicitly against NULL.
> + VEC_free(symbolp, symbols1);
> +
> + /* Copy information about function/method from minimal_symbols1 to minimal_symbols vector. */
> + if (minimal_symbols1)
> + {
> + for (ims = 0; VEC_iterate (bound_minimal_symbol_d, minimal_symbols1, ims, minsym); ++ims)
> + {
> + VEC_safe_push (bound_minimal_symbol_d, minimal_symbols, minsym);
> + }
> +
This last line has a bunch of whitespaces not needed.
You are also copying minimal_symbols1 to minimal_symbols. You could
probably use VEC_merge to make things quicker, but OK, I think it's good
as is.
> + VEC_free (bound_minimal_symbol_d, minimal_symbols1);
> + }
> +
> + VEC_free (symtab_ptr, file_symtabs);
> + }
> + }
>
> if (symbols != NULL || minimal_symbols != NULL)
> {
> @@ -2059,7 +2128,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
> {
> pspace = elem->objfile->pspace;
> set_current_program_space (pspace);
> - minsym_found (state, elem->objfile, elem->minsym, &sals);
> + minsym_found (state, elem->objfile, elem->minsym, ls, &sals);
> }
> }
> }
> @@ -2337,6 +2406,13 @@ parse_linespec (linespec_parser *parser, const char **argptr)
> values = convert_linespec_to_sals (PARSER_STATE (parser),
> PARSER_RESULT (parser));
>
> + if (values.nelts == 0)
> + {
> + /* The symbol is not found. */
> + symbol_not_found_error (PARSER_RESULT (parser)->function_name,
> + PARSER_RESULT (parser)->source_filename);
> + }
Strange formatting here.
> +
> return values;
> }
>
> @@ -3409,12 +3485,49 @@ collect_symbols (struct symbol *sym, void *data)
> return 1; /* Continue iterating. */
> }
>
> +/* Check whether the user source file is the same as the source file from SAL.
> + If so, return 1. Otherwise, return 0. */
> +static int
> +maybe_same_source_file (struct symtab_and_line* sal, linespec_p ls)
We put the "*" together with the variable name, not the type:
struct symtab_and_line *sal,...
> +{
> + /* We know if the user has specified a source file name by using the source_filename field from linespec. */
> + int need_filter = (ls->source_filename != NULL) ? 1 : 0;
> + if (need_filter)
You can write:
int need_filter = ls->source_filename != NULL;
Or why not just:
if (ls->source_filename != NULL)
?
Also, empty line between variable declaration and code.
> + {
> + int ix;
> + struct symtab *elt;
> + const char *name;
> + const char *fullname;
> +
> + if (sal->symtab == NULL)
> + return 0;
> +
> + fullname = symtab_to_fullname(sal->symtab);
Space between function name and open paren.
> +
> + for (ix = 0; VEC_iterate (symtab_ptr, ls->file_symtabs, ix, elt); ++ix)
> + {
> + if (elt)
Explicit NULL check.
> + {
> + name = symtab_to_fullname (elt);
I'd prefer if you declared "name" inside this block.
> +
Unecessary whitespaces in the line above.
> + if (strcmp(fullname, name) == 0)
Space between function name and open pare.
> + return 1;
> + }
> + }
> +
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> /* We've found a minimal symbol MSYMBOL in OBJFILE to associate with our
> linespec; return the SAL in RESULT. */
>
> static void
> minsym_found (struct linespec_state *self, struct objfile *objfile,
> struct minimal_symbol *msymbol,
> + linespec_p ls,
> struct symtabs_and_lines *result)
> {
> struct gdbarch *gdbarch = get_objfile_arch (objfile);
> @@ -3434,7 +3547,7 @@ minsym_found (struct linespec_state *self, struct objfile *objfile,
> if (self->funfirstline)
> skip_prologue_sal (&sal);
>
> - if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))
> + if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc) && maybe_same_source_file (&sal, ls))
Line too long. You can write as:
if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc)
&& maybe_same_source_file (&sal, ls))
> add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol), 0);
> }
>
And last, but not least, could you submit a testcase for this?
This is not much of a technical review, which I intend to do later. I
just wanted to point the easy-to-fix problems I saw in the code.
Thanks a lot,
--
Sergio
GPG key ID: 0x65FC5E36
Please send encrypted e-mail if possible
http://sergiodj.net/