gdb/linespec.c: Fix missing source file during breakpoint re-set.

  Hi,

I'm reposting this patch with a testcase added to
testsuite/gdb.debuginfod/fetch_src_and_symbols.exp.  I tried finding a
way to reproduce the error without debuginfod by using 'set
substitute-path' but the substitution rules always converted the symtab
fullname to the filename which avoided the error.  Since I don't know
of any other way to trigger the error other than with source files
downloaded from debuginfod, the gdb.debuginfod seems like the best (only?)
place for the test.

Aaron

---

During breakpoint re-setting, the source_filename of an
explicit_location_spec is used to lookup the symtabs associated with
the breakpoint being re-set.  This source_filename is compared with each
known symtab filename in order to retrieve the breakpoint's symtabs.

However the source_filename may have been originally copied from a
symtab's fullname (the path where GDB found the source file) when the
breakpoint was first created.  If a breakpoint symtab's filename and
fullname differ and there is no substitute-path setting that converts
the fullname to the filename, this will cause a NOT_FOUND_ERROR to be
thrown during re-setting.

Fix this by using a symtab's filename to set the explicit_location_spec
source_filename instead of the symtab's fullname.
---
 gdb/linespec.c                                         | 4 ++--
 gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp | 7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)
  

On 12/10/2022 22:49, Aaron Merey via Gdb-patches wrote:
> Hi,
>
> I'm reposting this patch with a testcase added to
> testsuite/gdb.debuginfod/fetch_src_and_symbols.exp.  I tried finding a
> way to reproduce the error without debuginfod by using 'set
> substitute-path' but the substitution rules always converted the symtab
> fullname to the filename which avoided the error.  Since I don't know
> of any other way to trigger the error other than with source files
> downloaded from debuginfod, the gdb.debuginfod seems like the best (only?)
> place for the test.

Another option would be making a unit test, so you can programmatically 
control the state from within GDB.

That said, your test does trigger the problem and shows that the patch 
fixes it, so I'm ok with this test. Just a minor nit.

> Aaron
>
> ---
>
> During breakpoint re-setting, the source_filename of an
> explicit_location_spec is used to lookup the symtabs associated with
> the breakpoint being re-set.  This source_filename is compared with each
> known symtab filename in order to retrieve the breakpoint's symtabs.
>
> However the source_filename may have been originally copied from a
> symtab's fullname (the path where GDB found the source file) when the
> breakpoint was first created.  If a breakpoint symtab's filename and
> fullname differ and there is no substitute-path setting that converts
> the fullname to the filename, this will cause a NOT_FOUND_ERROR to be
> thrown during re-setting.
>
> Fix this by using a symtab's filename to set the explicit_location_spec
> source_filename instead of the symtab's fullname.
> ---
>   gdb/linespec.c                                         | 4 ++--
>   gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp | 7 +++++--
>   2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 3db35998f7e..805c98ca201 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -2281,13 +2281,13 @@ convert_linespec_to_sals (struct linespec_state *state, linespec *ls)
>   	/* Make sure we have a filename for canonicalization.  */
>   	if (ls->explicit_loc.source_filename == NULL)
>   	  {
> -	    const char *fullname = symtab_to_fullname (state->default_symtab);
> +	    const char *filename = state->default_symtab->filename;
>   
>   	    /* It may be more appropriate to keep DEFAULT_SYMTAB in its symtab
>   	       form so that displaying SOURCE_FILENAME can follow the current
>   	       FILENAME_DISPLAY_STRING setting.  But as it is used only rarely
>   	       it has been kept for code simplicity only in absolute form.  */
> -	    ls->explicit_loc.source_filename = xstrdup (fullname);
> +	    ls->explicit_loc.source_filename = xstrdup (filename);
>   	  }
>       }
>     else
> diff --git a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> index 8bb9203686d..9e7d4321913 100644
> --- a/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> +++ b/gdb/testsuite/gdb.debuginfod/fetch_src_and_symbols.exp
> @@ -213,8 +213,11 @@ proc_with_prefix local_url { } {
>       gdb_test "file $binfile" "" "file [file tail $binfile]" "Enable debuginfod?.*" "y"
>       gdb_test_no_output "set substitute-path $outputdir /dev/null" \
>   	"set substitute-path"
> -    gdb_test "br main" "Breakpoint 1 at.*file.*"
> -    gdb_test "l" ".*This program is distributed in the hope.*"
> +    gdb_test "set cwd $debugdir" "" "file [file tail $binfile] cwd"
> +    gdb_test "list 1" ".*This program is distributed in the hope.*"
> +    gdb_test "break 25" "Breakpoint 1 at.*file.*"
> +    gdb_test "run" "Breakpoint 1,.*" \
> +	"file [file tail $binfile] set breakpoint]"

extra ']' at the end

Also, would be nice to have a small comment mentioning that there could 
be an error when resetting the breakpoint, so we have some context when 
looking at this test in the future.

With these changes, I'm ok with the patch going in, but I can't approve 
it for pushing. Don't forget to add the tag to the commit :)

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Cheers,
Bruno

>   
>       # GDB should now find the executable file.
>       clean_restart
  

On Thu, Oct 13, 2022 at 3:59 AM Bruno Larsen <blarsen@redhat.com> wrote:
> extra ']' at the end
>
> Also, would be nice to have a small comment mentioning that there could
> be an error when resetting the breakpoint, so we have some context when
> looking at this test in the future.
>
> With these changes, I'm ok with the patch going in, but I can't approve
> it for pushing. Don't forget to add the tag to the commit :)
>
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Thanks Bruno, fixed.
  

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> I'm reposting this patch with a testcase added to
Aaron> testsuite/gdb.debuginfod/fetch_src_and_symbols.exp.  I tried finding a
Aaron> way to reproduce the error without debuginfod by using 'set
Aaron> substitute-path' but the substitution rules always converted the symtab
Aaron> fullname to the filename which avoided the error.  Since I don't know
Aaron> of any other way to trigger the error other than with source files
Aaron> downloaded from debuginfod, the gdb.debuginfod seems like the best (only?)
Aaron> place for the test.

Sorry about the long delay on this.

It's pretty hard to review a patch like this, since it's hard to know
what impact it might have.  However I am mostly fine with it (see the
end), assuming you regression-tested it.

Aaron> +    gdb_test "run" "Breakpoint 1,.*" \
Aaron> +	"file [file tail $binfile] set breakpoint]"
 
I think there's an unmatched "]" here.

Tom