Re: [patchv2 7/11] Mechanical symtab->filename -> symtab_to_filename

[Joel Brobecker <brobecker at adacore dot com>]

Hi Jan,

On Fri, Feb 01, 2013 at 09:56:28PM +0100, Jan Kratochvil wrote:
> On Sun, 27 Jan 2013 23:35:38 +0100, Jan Kratochvil wrote:
> > formerly:
> > 	[patch 6/9] Mechanical symtab->filename -> symtab_to_filename
> > 	http://sourceware.org/ml/gdb-patches/2013-01/msg00390.html
> 
> Updated for symtab_to_filename -> symtab_to_filename_for_display.

> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -854,19 +854,19 @@ add_sal_to_sals (struct linespec_state *self,
>  					sals->nelts * sizeof (char *));
>        if (!literal_canonical && sal->symtab)
>  	{
> -	  char *filename = sal->symtab->filename;
> +	  const char *fullname = symtab_to_fullname (sal->symtab);

I have a change of behavior which I think might be unintended.
Before this change, I would get:

    (gdb) set multiple-symbols ask
    (gdb) b normal_menu
    [0] cancel
    [1] all
    [2] pck.adb:pck.normal_menu:10
    [3] pck.adb:pck.normal_menu:5
    >

But now, we get:

    (gdb) b normal_menu
    [0] cancel
    [1] all
    [2] /path/to/pck.adb:pck.normal_menu:10
    [3] /path/to/pck.adb:pck.normal_menu:5
    >

This is because decode_line_2 uses that cannonical_name to display
the breakpoint location match. I don't think we could have caught
this with the testsuite, because we use a project file to compile
Ada programs, and this has the side-effect of getting all sources
compiled with filename == fullname, that is gcc gets called with
the sources fullpath. (the use of project files is there because
it makes it very easy to build the program regardless of where
the sources are, and in particular whether we have an in-tree or
out-of-tree build).

Anyways, I think that the name displayed in the multiple-menu
should obbey the "filename-display" setting. Looking at the current
use of that field, and its value prior to your change (which is
advertised as mechanical), I think it should be fine to change
it as attached.

> @@ -1729,15 +1729,17 @@ create_sals_line_offset (struct linespec_state *self,
>    if (VEC_length (symtab_p, ls->file_symtabs) == 1
>        && VEC_index (symtab_p, ls->file_symtabs, 0) == NULL)
>      {
> +      const char *fullname;
> +
>        set_current_program_space (self->program_space);
>  
>        /* Make sure we have at least a default source line.  */
>        set_default_source_symtab_and_line ();
>        initialize_defaults (&self->default_symtab, &self->default_line);
> +      fullname = symtab_to_fullname (self->default_symtab);
>        VEC_pop (symtab_p, ls->file_symtabs);
>        VEC_free (symtab_p, ls->file_symtabs);
> -      ls->file_symtabs
> -	= collect_symtabs_from_filename (self->default_symtab->filename);
> +      ls->file_symtabs = collect_symtabs_from_filename (fullname);
>        use_default = 1;
>      }
>  
> @@ -1939,7 +1941,11 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls)
>  
>  	/* Make sure we have a filename for canonicalization.  */
>  	if (ls->source_filename == NULL)
> -	  ls->source_filename = xstrdup (state->default_symtab->filename);
> +	  {
> +	    const char *fullname = symtab_to_fullname (state->default_symtab);
> +
> +	    ls->source_filename = xstrdup (fullname);
> +	  }
>      }
>    else
>      {

I am not really sure about the above two hunks, as they also
do not follow the pattern of the rest of the patch. The first
one looks like it can only be OK.  The second one, on the other
hand, seems like another place where we should be using
symtab_to_filename_for_display.  Analyzing its current use,
though, I think we (temporarily) get away with it; it's just
latent, and waiting to show up one day.

For now, the attached patch only changes the first occurrence.
I haven't touched at the last hunk, since as I haven't been able
 to produce any user-visible issue related to that change.

gdb/ChangeLog:

        * linespec.c (add_sal_to_sals): Use symtab_to_filename_for_display
        instead of symtab_to_fullname to determine the source file name
        in the canonical_name.  Rename local variable "fullname" back
        to "filename".

Tested on x86_64-linux. WDYT?

Thanks,
-- 
Joel

>From 9c6f838668288ba5d447d15abb398e34fe09a12e Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Wed, 27 Feb 2013 16:59:49 -0800
Subject: [PATCH] Honor "set filename-display" in breakpoint multiple-choice
 menu.

This patch fixes a regression introduced by a commit at the FSF:

    commit 85622eb8d78dfa93e7034ef519e2e7f8557d6cc5
    Author: Jan Kratochvil <jan.kratochvil@redhat.com>
    Date:   Sun Feb 3 16:13:26 2013 +0000
    Subject: [patchv2 7/11] Mechanical symtab->filename -> symtab_to_filename

Prior to the patch above, we had:

    (gdb) set multiple-symbols ask
    (gdb) b normal_menu
    [0] cancel
    [1] all
    [2] pck.adb:pck.normal_menu:10
    [3] pck.adb:pck.normal_menu:5
    >

But now, we get:

    (gdb) b normal_menu
    [0] cancel
    [1] all
    [2] /path/to/pck.adb:pck.normal_menu:10
    [3] /path/to/pck.adb:pck.normal_menu:5
    >

Instead of using the source file's full path, the filename should
follow what the user asked via the filename-display setting (set
to "relative" by default).  This patch implements this.

gdb/ChangeLog:

        * linespec.c (add_sal_to_sals): Use symtab_to_filename_for_display
        instead of symtab_to_fullname to determine the source file name
        in the canonical_name.  Rename local variable "fullname" back
        to "filename".
---
 gdb/linespec.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 2e98db7..0bd1f11 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -854,19 +854,19 @@ add_sal_to_sals (struct linespec_state *self,
 					sals->nelts * sizeof (char *));
       if (!literal_canonical && sal->symtab)
 	{
-	  const char *fullname = symtab_to_fullname (sal->symtab);
+	  const char *filename = symtab_to_filename_for_display (sal->symtab);
 
 	  /* Note that the filter doesn't have to be a valid linespec
 	     input.  We only apply the ":LINE" treatment to Ada for
 	     the time being.  */
 	  if (symname != NULL && sal->line != 0
 	      && self->language->la_language == language_ada)
-	    canonical_name = xstrprintf ("%s:%s:%d", fullname, symname,
+	    canonical_name = xstrprintf ("%s:%s:%d", filename, symname,
 					 sal->line);
 	  else if (symname != NULL)
-	    canonical_name = xstrprintf ("%s:%s", fullname, symname);
+	    canonical_name = xstrprintf ("%s:%s", filename, symname);
 	  else
-	    canonical_name = xstrprintf ("%s:%d", fullname, sal->line);
+	    canonical_name = xstrprintf ("%s:%d", filename, sal->line);
 	}
       else if (symname != NULL)
 	canonical_name = xstrdup (symname);
-- 
1.7.10.4