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

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

[Philippe Waroquiers]

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.  */
 
-  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."));
 
-- 
2.20.1

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

[Pedro Alves]

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

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

[Philippe Waroquiers]

On Mon, 2020-06-22 at 11:57 +0100, Pedro Alves wrote:
> 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.
Is this ok to push (with the ALWAYS_CONFIRM uppercased) ?

...
> 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
If we would implement a symfile build-id check, what would be
the check ?  Unclear how to
determine a symfile
from the process we are attaching to.  So, would this check 
be a
comparison between the exec-file build-id and the symfile
build-id ?  And if we detect a
mismatch for symfile, again unclear
which file we would be able to find/load automatically.
So, maybe we could/should just give a warning whenever an exec-file
build-id does not match
a symfile build-id, in whatever context ?

> 
> 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.
I am wondering.  As far as I understand, we detect a mismatch for
the exec-file only.  If the exec-file is still ok, but the symbol file
would not, then the current code does not detect anything.
So, it looks to me that the current name corresponds to what is detected.

So, maybe it is good enough to do what you suggest below, i.e.
clarify in the help the action that GDB does for "ask", i.e.
that it reload the exec file and its debug info ?

If that sounds the best, I can prepare a patch for that,
together with introducing the word "build-id" in the help text
and in the warn/ask message produced when a mismatch is detected.

Thanks
Philippe

> 
> 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
> 

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

[Pedro Alves]

On 6/23/20 9:23 PM, Philippe Waroquiers via Gdb-patches wrote:
> On Mon, 2020-06-22 at 11:57 +0100, Pedro Alves wrote:

>>
>> This seems fine to me.
> Is this ok to push (with the ALWAYS_CONFIRM uppercased) ?

OK.

> 
> ...
>> 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
> If we would implement a symfile build-id check, what would be
> the check ?  Unclear how to
> determine a symfile
> from the process we are attaching to.  So, would this check 
> be a
> comparison between the exec-file build-id and the symfile
> build-id ?  

Same as exec-file detection, we can compare the build IDs of the
executable that the target process is running (or was running, in
case of cores), and the build ID of the symbols file loaded
in GDB.

> And if we detect a
> mismatch for symfile, again unclear
> which file we would be able to find/load automatically.

We could use debuginfod to try loading the symbol file automatically,
for example.

> So, maybe we could/should just give a warning whenever an exec-file
> build-id does not match
> a symfile build-id, in whatever context ?

That sounds desirable, yes.  validate_files() in corefile.c does that
already, but only for core file vs exec file.  Speaking of which, 
validate_exec_files currently doesn't consider cores.  And
validate_files only warns -- if we teach it to load files
from debuginfod, then it could ask too.  Kind of feels like these
functions and their UI could be merged somehow.  Anyway, doesn't have
to be now.

> 
>>
>> 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.
> I am wondering.  As far as I understand, we detect a mismatch for
> the exec-file only.  If the exec-file is still ok, but the symbol file
> would not, then the current code does not detect anything.
> So, it looks to me that the current name corresponds to what is detected.

True.

> 
> So, maybe it is good enough to do what you suggest below, i.e.
> clarify in the help the action that GDB does for "ask", i.e.
> that it reload the exec file and its debug info ?
> 

The point is, what happens when add detection of symbol file
mismatches as well.  Imagine we've already implemented that.
If we have that detection too, does the current command name
make sense?  I'm not sure.

Maybe it's not a big deal, and we can always add an alias later
if/when we decide to extend its scope later.

> If that sounds the best, I can prepare a patch for that,
> together with introducing the word "build-id" in the help text
> and in the warn/ask message produced when a mismatch is detected.

Thanks.  If we stick with the current name and behavior, then
I think that the documentation and online help bits are the most
important.  We can add the extra symbol vs exec warning later, for
example.

>> 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