* [RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch. @ 2020-06-21 19:44 Philippe Waroquiers 2020-06-22 10:57 ` Pedro Alves 0 siblings, 1 reply; 4+ messages in thread From: Philippe Waroquiers @ 2020-06-21 19:44 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch. 2020-06-21 19:44 [RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch Philippe Waroquiers @ 2020-06-22 10:57 ` Pedro Alves 2020-06-23 20:23 ` Philippe Waroquiers 0 siblings, 1 reply; 4+ messages in thread From: Pedro Alves @ 2020-06-22 10:57 UTC (permalink / raw) To: Philippe Waroquiers, gdb-patches 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch. 2020-06-22 10:57 ` Pedro Alves @ 2020-06-23 20:23 ` Philippe Waroquiers 2020-06-24 12:18 ` Pedro Alves 0 siblings, 1 reply; 4+ messages in thread From: Philippe Waroquiers @ 2020-06-23 20:23 UTC (permalink / raw) To: Pedro Alves, gdb-patches 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch. 2020-06-23 20:23 ` Philippe Waroquiers @ 2020-06-24 12:18 ` Pedro Alves 0 siblings, 0 replies; 4+ messages in thread From: Pedro Alves @ 2020-06-24 12:18 UTC (permalink / raw) To: Philippe Waroquiers, gdb-patches 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-24 12:18 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-21 19:44 [RFA] Fixes PR 25475: ensure exec-file-mismatch "ask" always asks in case of mismatch Philippe Waroquiers 2020-06-22 10:57 ` Pedro Alves 2020-06-23 20:23 ` Philippe Waroquiers 2020-06-24 12:18 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).