Re: [patch v9 1/5] New remove-symbol-file command.

Luis Machado
2013-06-18T17:26:18Z

Found a few more nits.

> @@ -7457,6 +7457,66 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
> }
> }
>
> +/* Disable any breakpoints and tracepoints in OBJFILE upon
> + notification of free_objfile. Only apply to enabled breakpoints,
> + disabled ones can just stay disabled. */
> +
> +static void
> +disable_breakpoints_in_freed_objfile (struct objfile * objfile)

struct objfile *objfile

> +{
> + struct breakpoint *b;
> +
> + if (objfile == NULL)
> + return;
> +
> + /* If the file is a shared library not loaded by the user then
> + solib_unloaded was notified and disable_breakpoints_in_unloaded_shlib
> + was called. In that case there is no need to take action again. */
> + if ((objfile->flags & OBJF_SHARED) && !(objfile->flags & OBJF_USERLOADED))
> + return;
> +
> + ALL_BREAKPOINTS (b)
> + {

Identation is odd here. Should be further in.

> @@ -1463,6 +1466,26 @@ resume_section_map_updates_cleanup (void *arg)
> resume_section_map_updates (arg);
> }
>
> +/* Return 1 if ADDR maps into one of the sections of OBJFILE and 0
> + otherwise. */
> +
> +int
> +is_addr_in_objfile (CORE_ADDR addr, const struct objfile * objfile)

const struct objfile *objfile

> +{
> + struct obj_section *osect;
> +
> + if (objfile == NULL)
> + return 0;
> +
> + ALL_OBJFILE_OSECTIONS (objfile, osect)
> + {

Identation off as well. Should be further in.

> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index adb1ef8..b25afb1 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -482,6 +482,8 @@ extern int have_full_symbols (void);
>
> extern void objfiles_changed (void);
>
> +extern int is_addr_in_objfile (CORE_ADDR, const struct objfile *);
> +

Prototype arguments should be named as in the rest of the prototypes of
this header file.

> diff --git a/gdb/solib.c b/gdb/solib.c
> index c987fe5..7ceef29 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1478,6 +1478,26 @@ gdb_bfd_lookup_symbol (bfd *abfd,
> return symaddr;
> }
>
> +/* SO_LIST_HEAD may contain user-loaded object files that can be removed
> + out-of-band by the user. So upon notification of free_objfile remove
> + any reference to any user-loaded file that is about to be freed. */
> +

while at it, "any references to any user-loaded...".

> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index c2ad797..f7ad268 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2330,6 +2330,81 @@ add_symbol_file_command (char *args, int from_tty)
> }
> ^L
>
> +/* This function removes a symbol file that was added via add-symbol-file. */
> +
> +static void
> +remove_symbol_file_command (char *args, int from_tty)
> +{
> + char **argv;
> + struct objfile* objf = NULL;
> + struct cleanup *my_cleanups;
> + struct program_space *pspace = current_program_space;
> + struct gdbarch *gdbarch = get_current_arch ();
> +
> + dont_repeat ();
> +
> + if (args == NULL)
> + error (_("USAGE: remove-symbol-file FILENAME\n\
> + remove-symbol-file -a ADDRESS"));

I discovered something odd. The only two commands using "USAGE" in help
texts are this one and add-symbol-file. I think we should drop it.

Moreover, add-symbol-file prints this when ran without arguments:

(gdb) add-symbol-file
add-symbol-file takes a file name and an address

And this when ran with more arguments than it should:

USAGE: add-symbol-file <filename> <textaddress> [-readnow]
[-s <secname> <addr>]*

So it is a little odd. We should probably fix this in another patch. For
this
one i think we should only drop USAGE.

> +
> + my_cleanups = make_cleanup (null_cleanup, NULL);
> +
> + argv = gdb_buildargv (args);
> +
> + if (strcmp (argv[0], "-a") == 0)
> + {
> + /* Interpret the next argument as an address. */
> + CORE_ADDR addr;
> +
> + if (argv[1] == NULL)
> + error (_("Missing address argument"));
> +
> + if (argv[2] != NULL)
> + error (_("Junk after %s"), argv[1]);
> +
> + addr = parse_and_eval_address (argv[1]);
> +
> + ALL_OBJFILES (objf)
> + {

Identation off here. Should be further in.

> + if (objf->flags & OBJF_USERLOADED
> + && objf->pspace == pspace
> + && is_addr_in_objfile (addr, objf))
> + break;
> + }
> + }
> + else if (argv[0] != NULL)
> + {
> + /* Interpret the current argument as a file name. */
> + char *filename;
> +
> + if (argv[1] != NULL)
> + error (_("Junk after %s"), argv[0]);
> +
> + filename = tilde_expand (argv[0]);
> + make_cleanup (xfree, filename);
> +
> + ALL_OBJFILES (objf)
> + {

Identation off as well. Should be further in.