Re: [PATCH] gdb/linespec.c: Fix missing source file during breakpoint re-set.

[Bruno Larsen <blarsen@redhat.com>]

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