Re: [PATCH v3] Make only user-specified executable and symbol filenames sticky

[Pedro Alves <palves@redhat.com>]

Sorry for dropping the ball on this...  Comments below.

On 06/08/2015 10:01 AM, Gary Benson wrote:

> 
> 	PR gdb/17626
> 	* progspace.h (struct program_space)
> 	<pspace_exec_file_is_user_supplied>: New field.
> 	<symfile_object_file_is_user_supplied>: Likewise.
> 	(symfile_objfile_is_user_supplied): New macro.
> 	* exec.h (exec_file_is_user_supplied): Likewise.
> 	* exec.c (exec_close): Clear exec_file_is_user_supplied.
> 	(exec_file_locate_attach): Remove get_exec_file check.
> 	Do not replace user-supplied executable or symbol files.
> 	Warn if user-supplied executable or symbol files do not
> 	match discovered file.
> 	(exec_file_command): Set exec_file_is_user_supplied.
> 	* symfile.c (symbol_file_clear): Clear
> 	symfile_objfile_is_user_supplied.
> 	(symbol_file_command): Set symfile_objfile_is_user_supplied.
> 	* inferior.c (add_inferior_command): Set
> 	exec_file_is_user_supplied and symfile_objfile_is_user_supplied.
> 	* main.c (captured_main): Likewise.
> 	* infcmd.c (attach_command_post_wait): Always call
> 	exec_file_locate_attach.  Only call reopen_exec_file and
> 	reread_symbols if exec_file_is_user_supplied.
> 	* remote.c (remote_add_inferior): Remove get_exec_file check
> 	around exec_file_locate_attach.
> ---
>  gdb/ChangeLog   |   26 ++++++++++++++++++++++++++
>  gdb/exec.c      |   30 +++++++++++++++++++++++-------
>  gdb/exec.h      |    2 ++
>  gdb/infcmd.c    |   13 ++++++++-----
>  gdb/inferior.c  |    3 +++
>  gdb/main.c      |   24 ++++++++++++++++--------
>  gdb/progspace.h |   17 +++++++++++++++++
>  gdb/remote.c    |    9 ++++++---
>  gdb/symfile.c   |    3 +++
>  9 files changed, 104 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/exec.c b/gdb/exec.c
> index 8a4ab6f..11e5db0 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -102,6 +102,7 @@ exec_close (void)
>  
>        xfree (exec_filename);
>        exec_filename = NULL;
> +      exec_file_is_user_supplied = 0;
>      }
>  }
>  
> @@ -142,11 +143,6 @@ exec_file_locate_attach (int pid, int from_tty)
>  {
>    char *exec_file, *full_exec_path = NULL;
>  
> -  /* Do nothing if we already have an executable filename.  */
> -  exec_file = (char *) get_exec_file (0);
> -  if (exec_file != NULL)
> -    return;
> -
>    /* Try to determine a filename from the process itself.  */
>    exec_file = target_pid_to_exec_file (pid);
>    if (exec_file == NULL)
> @@ -171,8 +167,27 @@ exec_file_locate_attach (int pid, int from_tty)
>  	full_exec_path = xstrdup (exec_file);
>      }
>  
> -  exec_file_attach (full_exec_path, from_tty);
> -  symbol_file_add_main (full_exec_path, from_tty);
> +  if (exec_file_is_user_supplied)
> +    {
> +      if (strcmp (full_exec_path, exec_filename) != 0)
> +	warning (_("Process %d has executable file %s,"
> +		   " but executable file is currently set to %s"),
> +		 pid, full_exec_path, exec_filename);
> +    }
> +  else
> +    exec_file_attach (full_exec_path, from_tty);
> +
> +  if (symfile_objfile_is_user_supplied)
> +    {
> +      const char *symbol_filename = objfile_filename (symfile_objfile);
> +
> +      if (strcmp (full_exec_path, symbol_filename) != 0)
> +	warning (_("Process %d has symbol file %s,"
> +		   " but symbol file is currently set to %s"),
> +		 pid, full_exec_path, symbol_filename);
> +    }
> +  else
> +    symbol_file_add_main (full_exec_path, from_tty);
>  }
>  


This function is called while the passed in PID is not the
current inferior.  E.g., the remote_add_inferior path.

Therefore seems to me that these symbol_file_add_main / exec_file_attach
calls can change the symbols of the wrong inferior.

(I still suspect that if we reverse the sense of the flag, then
its management ends up being more centralized, as then the
place that sets it is also the place that needs to check it,
instead of doing that in multiple places.  But, see below.)

I also notice that reread_symbols has an exec_file_attach
call which seems to me results in losing the is_user_supplied
flag if the executable's timestamp changed, at least here:

> +  /* Attempt to open the file that was executed to create this
> +     inferior.  If the user has explicitly specified executable
> +     and/or symbol files then warn the user if their choices do
> +     not match.  Otherwise, set exec_file and symfile_objfile to
> +     the new file.  */
> +  exec_file_locate_attach (ptid_get_pid (inferior_ptid), from_tty);
> +
> +  if (exec_file_is_user_supplied)
>      {
>        reopen_exec_file ();
>        reread_symbols ();


I also notice that the clone-inferior command ends up with
an exec_file_attach call in clone_program_space.  That one
should probably be setting the is_user_supplied flag too,
I'd think.  Or at least, copying it from the original.

At this point, I'm wondering about adding a parameter to
exec_file_attach to force considering (now and in future)
the right value to put in the flag in each case.

> @@ -2490,11 +2490,14 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
>    inferior = current_inferior ();
>    inferior->control.stop_soon = NO_STOP_QUIETLY;
>  
> -  /* If no exec file is yet known, try to determine it from the
> -     process itself.  */
> -  if (get_exec_file (0) == NULL)
> -    exec_file_locate_attach (ptid_get_pid (inferior_ptid), from_tty);
> -  else
> +  /* Attempt to open the file that was executed to create this
> +     inferior.  If the user has explicitly specified executable
> +     and/or symbol files then warn the user if their choices do
> +     not match.  Otherwise, set exec_file and symfile_objfile to
> +     the new file.  */
> +  exec_file_locate_attach (ptid_get_pid (inferior_ptid), from_tty);
> +
> +  if (exec_file_is_user_supplied)
>      {
>        reopen_exec_file ();
>        reread_symbols ();

It seems to me that we should be able to move these reopen/reread
calls inside exec_file_locate_attach.

Thanks,
Pedro Alves