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] Add set/show display-linkname command


>>>>> "Michael" == Michael Eager <eager@eagerm.com> writes:

Thanks for the patch.

Michael> --- gdb/disasm.c	11 Mar 2013 08:53:17 -0000	1.51
Michael> +++ gdb/disasm.c	27 Mar 2013 19:36:23 -0000
[...]

Michael> +      if (!build_address_symbolic (gdbarch, pc, 0, &name, &linkname, &offset, 
Michael> +				   &filename, &line, &unmapped))
Michael>  	{

Nothing frees linkname here.

Note that the current code does:

      if (filename != NULL)
	xfree (filename);

... but really those 'if's are not needed, you can just write

    xfree (linkname);

Michael> +/* Maximum length of linkname to print.  */
Michael> +#define MAX_LINKNAME_LEN 20

If there is going to be a maximum, it should be user-settable.

Michael> -  /* Throw away both name and filename.  */
Michael> +  /* Throw away both name, linkname, and filename.  */
Michael>    struct cleanup *cleanup_chain = make_cleanup (free_current_contents, &name);
Michael>    make_cleanup (free_current_contents, &filename);

This needs a cleanup for linkname.

Michael> +  if (disp_linkname && linkname && strcmp (name, linkname))

gdb style is now to use explicit NULL checks for pointers, so

    ... linkname != NULL ...

Michael> +  /* If we have both symbol names and they are different, let caller know.  */
Michael> +  if (msymbol && symbol 

'!= NULL' here too.

Michael> +      && strcmp (SYMBOL_LINKAGE_NAME (msymbol), SYMBOL_LINKAGE_NAME (symbol)))

'!= 0'.

There are a few more instances of both of these.

Michael> +/* Maximum length of linkname to print.  */
Michael> +#define MAX_LINKNAME_LEN 20

Definitely don't duplicate defines.

I think the comment describing find_frame_funname and also
build_address_symbolic should mention that the linkage name is only set
if it differs from the symbol's name.

Michael> +  /* Print linkage name after source name if requested and different.  */
Michael> +  if (disp_linkname && linkname && strcmp (funname, linkname))
Michael> +    {
Michael> +      char *lname = (char *)linkname;

I think this cast isn't needed -- you can use const here and make a new
"char*" temporary in the block that does the alloca.

Michael> +      if (strlen (lname) > MAX_LINKNAME_LEN) 
Michael> +        {
Michael> +          lname = alloca (MAX_LINKNAME_LEN + 4);
Michael> +	  strncpy (lname, linkname, MAX_LINKNAME_LEN);
Michael> +	  strcat (lname, "...");
Michael> + 	}
Michael> +      ui_out_text (uiout, " [");
Michael> +      ui_out_text (uiout, lname);
Michael> +      ui_out_text (uiout, "]");

I think for MI it is better to make this a real field.
Also I wonder whether this should be emitted unconditionally if
mi-like.  Say:

if ((disp_linkname || ui_out_is_mi_like_p (uiout))
    && linkname != NULL && strcmp (funname, linkname) != 0)
...
  ui_out_text (uiout, " [");
  ui_out_field_string (uiout, "linkage_name", linkname);
  ui_out_text (uiout, "]");

Michael> +int disp_linkname = 0;		/* Default is no. */

I'd prefer it to be spelled out.

One idea would be to have this same variable control the length.
Maybe that is too hacky though.

Tom


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