This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add set/show display-linkname command
- From: Tom Tromey <tromey at redhat dot com>
- To: Michael Eager <eager at eagerm dot com>
- Cc: Eli Zaretskii <eliz at gnu dot org>, gdb-patches at sourceware dot org
- Date: Fri, 19 Apr 2013 14:28:11 -0600
- Subject: Re: [PATCH] Add set/show display-linkname command
- References: <5147BD81 dot 6000702 at eagercon dot com> <514B59C2 dot 4060806 at eagerm dot com> <83sj3oy1mb dot fsf at gnu dot org> <514B73CD dot 5030602 at eagerm dot com> <51534BBF dot 9050905 at eagerm dot com>
>>>>> "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