This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] Bug 17394: we cannot put a break-point at a global function for ASM file


Hi Sergio,

Thank you very much for your comments.
I will send a new patch code that includes your recommendation.

Best regards,
Mihai

-----Original Message-----
From: Sergio Durigan Junior [mailto:sergiodj@redhat.com] 
Sent: Monday, September 15, 2014 11:44 PM
To: Nistor Mihail-MNISTOR1
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] Bug 17394: we cannot put a break-point at a global function for ASM file

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/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]