This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3] Added command remove-symbol-file.
- From: Pedro Alves <palves at redhat dot com>
- To: Nicolas Blanc <nicolas dot blanc at intel dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 26 Apr 2013 18:33:30 +0100
- Subject: Re: [PATCH 1/3] Added command remove-symbol-file.
- References: <1366098721-18302-1-git-send-email-nicolas dot blanc at intel dot com> <1366098721-18302-2-git-send-email-nicolas dot blanc at intel dot com>
On 04/16/2013 08:51 AM, Nicolas Blanc wrote:
> 2013-18-03 Nicolas Blanc <nicolas.blanc@intel.com>
>
> * breakpoint.c (disable_breakpoints_in_free_objfile): Created
> function for disabling breakoints in objfiles upon FREE_OBJFILE
> notifications.
Present tense "create", etc. (throughout the log entry).
Typo breakoints.
> * doc/observer.text: Created FREE_OBJFILE event.
> * objfiles.c (free_objfile): Notify FREE_OBJFILE.
> * printcmd.c (clear_dangling_display_expressions): Act upon FREE_OBJFILE
> events instead of SOLIB_UNLOADED events.
> (_initialize_printcmd): Register observer for FREE_OBJFILE instead
> of SOLIB_UNLOADED notifications.
> * solib.c (remove_user_added_objfile): Created function for removing
> dangling references upon notification of FREE_OBJFILE.
> * symfile.c (add_symbol_file_command): Set OBJFILE->LOW_ADDRESS.
LOW_ADDR.
> (remove_symbol_file_command): Created command for removing symbol files.
> (_initialize_symfile): Added remove-symbol-file.
> +/* Upon notification of FREE_OBJFILE remove any reference
> + to any user-added file that is about to be freed. */
Why only user-added files?
> +static void
> +remove_user_added_objfile (struct objfile *objfile)
> +{
> + struct so_list *gdb;
> +
> + if (!objfile)
> + return;
> +
> + if (!(objfile->flags & OBJF_USERLOADED)
> + || !(objfile->flags & OBJF_SHARED))
> + return;
> +
> +
> + gdb = so_list_head;
> + while (gdb)
> + {
> + if (gdb->objfile == objfile)
> + gdb->objfile = NULL;
> + gdb = gdb->next;
> + }
Or rather/also, this looks a bit weird to me.
Can we ever really ever find a user-loaded file in the
so_list_head list? What would that mean?
IIRC, the only way to get a OBJF_USERLOADED|OBJF_SHARED
objfile is through "dll-symbols" (dll_symbol_command),
but that doesn't create any entry in the shared library list.
BTW, calling the variable "gdb" was probably copied from
solib.c:update_solib_list. It makes sense to call it
that to contrast with "inferior", but not here. Just call
it something like "so". That while loop can be written
simpler as a for loop:
for (so = so_list_head; so != NULL; so = so->next)
if (so->objfile == objfile)
so->objfile = NULL;
> +static void
> +disable_breakpoints_in_free_objfile (struct objfile * objfile)
This is clearly mirroring the naming of
disable_breakpoints_in_unloaded_shlib. Should be "in freed objfile".
"in free objfile" would mean something else.
> + /* If the file is a shared library not loaded by the user then
> + SOLIB_UNLOADED was notified and DISABLE_BREAKPIONTS_IN_UNLOADED_SHLIB
Typo BREAKPIONTS. Actually, uppercase is used when referring to
a value of a variable (see GNU coding standards), which is not the
case here.
> + error (_("USAGE: remove-symbol-file <text_low_address>"));
I'd s/low// here. text_address is clear and common enough that
having "low" there makes me go "what does low mean here?" Or just
<address> even.
> + if (objf->flags & OBJF_USERLOADED && objf->addr_low == addr)
As I mentioned, the .text address may not be the lower address
in the object at all, so this "addr_low" confused me.
I'd be happier with naming the field for what ig really is, something
like "add_symbol_file_addr", with a comment indicating this is related
to "add-symbol-file", but I see you're reusing an existing variable. At
the very least, the variable's definition should gain a comment explaining
its overloading for add-symbol-file.
--
Pedro Alves