[RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch.

Pedro Alves palves@redhat.com
Mon Jun 22 10:57:30 GMT 2020


On 6/21/20 8:44 PM, Philippe Waroquiers via Gdb-patches wrote:
> As explained in https://sourceware.org/bugzilla/show_bug.cgi?id=25475,
> when the currently loaded file has no debug symbol,
> symbol_file_add_with_addrs does not ask a confirmation to the user
> before loading the new symbol file.  The behaviour is not consistent
> when symbol_file_add_with_addrs is called due to exec-file-mismatch "ask"
> setting.
> 
> The PR discusses several solutions/approaches.
> The preferred approach (suggested by Joel) is to ensure that GDB always asks
> a confirmation when it loads a new symbol file due to exec-file-mismatch,
> using a new SYMFILE add-flag.
> 
> I tested this manually.  If OK, we can remove the bypass introduced by Tom
> in 6b9374f1, in order to always answer to the 'load' question.
> 
> gdb/ChangeLog
> YYYY-MM-DD  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* symfile-add-flags.h: New flag SYMFILE_ALWAYS_CONFIRM.
> 	* exec.c (validate_exec_file): If from_tty, set both
> 	SYMFILE_VERBOSE (== from_tty) and SYMFILE_ALWAYS_CONFIRM.
> 	* symfile.c (symbol_file_add_with_addrs): if always_confirm
> 	and from_tty, unconditionally ask a confirmation.
> ---
>  gdb/exec.c              |  5 ++++-
>  gdb/symfile-add-flags.h |  6 ++++++
>  gdb/symfile.c           | 10 ++++++----
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/exec.c b/gdb/exec.c
> index fa770c6f02..de473fbcb2 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -315,7 +315,10 @@ validate_exec_file (int from_tty)
>  	{
>  	  symfile_add_flags add_flags = SYMFILE_MAINLINE;
>  	  if (from_tty)
> -	    add_flags |= SYMFILE_VERBOSE;
> +	    {
> +	      add_flags |= SYMFILE_VERBOSE;
> +	      add_flags |= SYMFILE_ALWAYS_CONFIRM;
> +	    }
>  	  try
>  	    {
>  	      symbol_file_add_main (exec_file_target.c_str (), add_flags);
> diff --git a/gdb/symfile-add-flags.h b/gdb/symfile-add-flags.h
> index 740357bc12..2b2c2e68f2 100644
> --- a/gdb/symfile-add-flags.h
> +++ b/gdb/symfile-add-flags.h
> @@ -44,6 +44,12 @@ enum symfile_add_flag : unsigned
>  
>      /* The new objfile should be marked OBJF_NOT_FILENAME.  */
>      SYMFILE_NOT_FILENAME = 1 << 5,
> +
> +    /* If SYMFILE_VERBOSE (interpreted as from_tty) and SYMFILE_ALWAYS_CONFIRM,
> +       always ask user to confirm loading the symbol file.
> +       Without this flag, symbol_file_add_with_addrs asks a confirmation only
> +       for a main symbol file replacing a file having symbols.  */
> +    SYMFILE_ALWAYS_CONFIRM = 1 << 6,
>   };
>  
>  DEF_ENUM_FLAGS_TYPE (enum symfile_add_flag, symfile_add_flags);
> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index b29f864b37..0e2310176f 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -1051,6 +1051,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
>    struct objfile *objfile;
>    const int from_tty = add_flags & SYMFILE_VERBOSE;
>    const int mainline = add_flags & SYMFILE_MAINLINE;
> +  const int always_confirm = add_flags & SYMFILE_ALWAYS_CONFIRM;
>    const int should_print = (print_symbol_loading_p (from_tty, mainline, 1)
>  			    && (readnow_symbol_files
>  				|| (add_flags & SYMFILE_NO_READ) == 0));
> @@ -1069,12 +1070,13 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name,
>    if ((add_flags & SYMFILE_NOT_FILENAME) != 0)
>      flags |= OBJF_NOT_FILENAME;
>  
> -  /* Give user a chance to burp if we'd be
> +  /* Give user a chance to burp if always_confirm or we'd be
>       interactively wiping out any existing symbols.  */

ALWAYS_CONFIRM, uppercase.

>  
> -  if ((have_full_symbols () || have_partial_symbols ())
> -      && mainline
> -      && from_tty
> +  if (from_tty
> +      && (always_confirm
> +	  || ((have_full_symbols () || have_partial_symbols ())
> +	      && mainline))
>        && !query (_("Load new symbol table from \"%s\"? "), name))
>      error (_("Not confirmed."));
>  
> 

This seems fine to me.  Like Joel, I would also like to get a better
understanding on why we shouldn't always ask when there's no debug
info, but this patch let's us treat that as an orthogonal issue.


I have one remark to make, while we're thinking about UI and the
incoming release -- it is about the setting name and its description:

 (gdb) help set exec-file-mismatch 
 Set exec-file-mismatch handling (ask|warn|off).
 Specifies how to handle a mismatch between the current exec-file
 loaded by GDB and the exec-file automatically determined when attaching
 to a process:

  ask  - warn the user and ask whether to load the determined exec-file.
  warn - warn the user, but do not change the exec-file.
  off  - do not check for mismatch.
 (gdb) 

Note how this is only talking about "exec-file".  The code is only 
validating exec-file.  But, on mismatch, it reloads _both_ the 
exec-file and the symbol-file.  These are different files in GDB:

 (gdb) help exec-file 
 Use FILE as program for getting contents of pure memory.
 If FILE cannot be found as specified, your execution directory path
 is searched for a command of that name.
 No arg means have no executable file.

 (gdb) help symbol-file 
 Load symbol table from executable file FILE.
 Usage: symbol-file [-readnow | -readnever] [-o OFF] FILE
 OFF is an optional offset which is added to each section address.
 The `file' command can also load symbol tables, as well as setting the file
 to execute.
 The '-readnow' option will cause GDB to read the entire symbol file
 immediately.  This makes the command slower, but may make future operations
 faster.
 The '-readnever' option will prevent GDB from reading the symbol file's
 symbolic debug information.

With the "file" command being a wrapper that does both exec-file + symbol-file:

 (gdb) help file
 Use FILE as program to be debugged.
 It is read for its symbols, for getting the contents of pure memory,
 and it is the program executed when you use the `run' command.
 If FILE cannot be found as specified, your execution directory path ($PATH) is 
 searched for a command of that name.
 No arg means to have no executable file and no symbols.
 (gdb) 

This is why you get two questions when you use the "file" command:

 (gdb) file /usr/bin/sleep 
 A program is being debugged already.
 Are you sure you want to change the file? (y or n) y
 Load new symbol table from "/usr/bin/sleep"? (y or n) y

These are the same questions if you get if issue the commands
separately:

 (gdb) exec-file /usr/bin/sleep 
 A program is being debugged already.
 Are you sure you want to change the file? (y or n) y

 (gdb) symbol-file /usr/bin/sleep 
 Load new symbol table from "/usr/bin/sleep"? (y or n) y


So, I'm not sure it's worth it to have separate settings for
exec-file and symbol-file:

  set exec-file-mismatch
  set symbol-file-mismatch

But it does look to me that we should do separate build ID
validations for the exec-file and the symbol-file.

And with that in mind (symbol-file validation + reload symbol-file),
perhaps we should consider whether "set exec-file-mismatch" is the right
name for this setting, or whether we should rename it to e.g.,
"set file-mismatch", to go with the "file" command instead of
the "exec-file" command.

At least, the documentation and online help should clearly state
that the symbols are reloaded as well, not just the "exec-file".

Thanks,
Pedro Alves



More information about the Gdb-patches mailing list