[patch] Fix multiple-symbols=ask regression [Re: [patchv2 7/11] Mechanical symtab->filename -> symtab_to_filename]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Joel,

On Thu, 28 Feb 2013 02:36:39 +0100, Joel Brobecker wrote:
> I have a change of behavior which I think might be unintended.

I agree, thanks for catching it.


> I think it should be fine to change it as attached.

With the included testcase your patch has:
	set filename-display absolute
	(gdb) PASS: gdb.linespec/break-ask.exp: set filename-display absolute
	break twodup
	[0] cancel
	[1] all
	[2] /home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.linespec/base/one/thefile.cc:twodup()
	[3] /home/jkratoch/redhat/gdb-clean/gdb/testsuite/gdb.linespec/base/two/thefile.cc:twodup()
	> PASS: gdb.linespec/break-ask.exp: break twodup absolute
	0
	canceled
	(gdb) PASS: gdb.linespec/break-ask.exp: 0
	set filename-display relative
	(gdb) PASS: gdb.linespec/break-ask.exp: set filename-display relative
	break twodup
	Breakpoint 1 at 0x40062f: twodup. (2 locations)
	(gdb) FAIL: gdb.linespec/break-ask.exp: break twodup relative

I believe "set filename-display" should not affect GDB behavior this way; with
the patch below it displays:

	break twodup
	[0] cancel
	[1] all
	[2] thefile.cc:twodup()
	[3] thefile.cc:twodup()
	> PASS: gdb.linespec/break-ask.exp: break twodup relative

It is only user's problem s/he does not see the whole pathnames.

Therefore linespec_sals->canonical is always stored in absolute form.


> > @@ -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;
> >      }
> >  

> The first one looks like it can only be OK.

Yes; the filename is passed to iterate_over_symtabs so it is better to pass it
always in absolute form.


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

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

It is used for (1) addr_string - which is AFAIK never displayed to user, so it
is correct to use absolute form; and (2) in some error messages, those should
follow filename-display setting although it is not such a problem.

ls->source_filename would be better as symtab, I will fix it in another (small)
patch.

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu.

The patch could be probably slightly smaller if it used the two char * forms
from struct decode_line_2_item instead of char * + symtab of struct
linespec_canonical_name.  Just I find struct symtab * more correct in global
data structures than its two string forms (absolute + display form).

I hope struct linespec_state cannot exist across commands, therefore stale
symtab * cannot happen there.


Thanks,
Jan


gdb/
2013-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* linespec.c (struct linespec_canonical_name): New.
	(struct linespec_state): Change canonical_names type to it.
	(add_sal_to_sals): Change variable canonical_name to canonical.  Change
	xrealloc element size.  Initialize the different CANONICAL fields.
	(canonical_to_fullform): New.
	(filter_results): Use it.  Add variables canonical, fullform and
	cleanup.
	(struct decode_line_2_item, decode_line_2_compare_items): New.
	(decode_line_2): Remove variables iter and item_names, add variables
	items and items_count.  Modify the code for these new variables.

gdb/testsuite/
2013-03-01  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.linespec/base/one/thefile.cc (twodup): New.
	(m): Call it.
	* gdb.linespec/base/two/thefile.cc (dupname): New.
	(n): Call it.
	* gdb.linespec/break-ask.exp: New file.
	* gdb.linespec/lspec.cc (body_elsewhere): New comment marker.

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 2e98db7..bc10843 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -157,6 +157,19 @@ struct linespec
 };
 typedef struct linespec *linespec_p;
 
+/* linespec represented as a symtab-related string.  */
+
+struct linespec_canonical_name
+{
+  /* Remaining text part of the linespec string.  */
+  char *suffix;
+
+  /* If NULL then SUFFIX is the whole linespec string.
+     Otherwise "SYMTAB:SUFFIX" is the linespec string.  SYMTAB can be converted
+     for example by symtab_to_fullname or symtab_to_filename_for_display.  */
+  struct symtab *symtab;
+};
+
 /* An instance of this is used to keep all state while linespec
    operates.  This instance is passed around as a 'this' pointer to
    the various implementation methods.  */
@@ -186,7 +199,7 @@ struct linespec_state
   struct linespec_result *canonical;
 
   /* Canonical strings that mirror the symtabs_and_lines result.  */
-  char **canonical_names;
+  struct linespec_canonical_name *canonical_names;
 
   /* This is a set of address_entry objects which is used to prevent
      duplicate symbols from being entered into the result.  */
@@ -848,10 +861,12 @@ add_sal_to_sals (struct linespec_state *self,
 
   if (self->canonical)
     {
-      char *canonical_name = NULL;
+      struct linespec_canonical_name *canonical;
 
       self->canonical_names = xrealloc (self->canonical_names,
-					sals->nelts * sizeof (char *));
+					(sals->nelts
+					 * sizeof (*self->canonical_names)));
+      canonical = &self->canonical_names[sals->nelts - 1];
       if (!literal_canonical && sal->symtab)
 	{
 	  const char *fullname = symtab_to_fullname (sal->symtab);
@@ -861,17 +876,21 @@ add_sal_to_sals (struct linespec_state *self,
 	     the time being.  */
 	  if (symname != NULL && sal->line != 0
 	      && self->language->la_language == language_ada)
-	    canonical_name = xstrprintf ("%s:%s:%d", fullname, symname,
-					 sal->line);
+	    canonical->suffix = xstrprintf ("%s:%d", symname, sal->line);
 	  else if (symname != NULL)
-	    canonical_name = xstrprintf ("%s:%s", fullname, symname);
+	    canonical->suffix = xstrdup (symname);
 	  else
-	    canonical_name = xstrprintf ("%s:%d", fullname, sal->line);
+	    canonical->suffix = xstrprintf ("%d", sal->line);
+	  canonical->symtab = sal->symtab;
+	}
+      else
+	{
+	  if (symname != NULL)
+	    canonical->suffix = xstrdup (symname);
+	  else
+	    canonical->suffix = NULL;
+	  canonical->symtab = NULL;
 	}
-      else if (symname != NULL)
-	canonical_name = xstrdup (symname);
-
-      self->canonical_names[sals->nelts - 1] = canonical_name;
     }
 }
 
@@ -1200,6 +1219,19 @@ find_toplevel_string (const char *haystack, const char *needle)
   return NULL;
 }
 
+/* Convert CANONICAL to a string using symtab_to_fullname for SYMTAB.
+   Caller has to xfree the result.  */
+
+static char *
+canonical_to_fullform (const struct linespec_canonical_name *canonical)
+{
+  if (canonical->symtab == NULL)
+    return xstrdup (canonical->suffix);
+  else
+    return xstrprintf ("%s:%s", symtab_to_fullname (canonical->symtab),
+		       canonical->suffix);
+}
+
 /* Given FILTERS, a list of canonical names, filter the sals in RESULT
    and store the result in SELF->CANONICAL.  */
 
@@ -1220,8 +1252,18 @@ filter_results (struct linespec_state *self,
 
       for (j = 0; j < result->nelts; ++j)
 	{
-	  if (strcmp (name, self->canonical_names[j]) == 0)
+	  const struct linespec_canonical_name *canonical;
+	  char *fullform;
+	  struct cleanup *cleanup;
+
+	  canonical = &self->canonical_names[j];
+	  fullform = canonical_to_fullform (canonical);
+	  cleanup = make_cleanup (xfree, fullform);
+
+	  if (strcmp (name, fullform) == 0)
 	    add_sal_to_sals_basic (&lsal.sals, &result->sals[j]);
+
+	  do_cleanups (cleanup);
 	}
 
       if (lsal.sals.nelts > 0)
@@ -1247,6 +1289,36 @@ convert_results_to_lsals (struct linespec_state *self,
   VEC_safe_push (linespec_sals, self->canonical->sals, &lsal);
 }
 
+/* Represent struct linespec_canonical_name with constant strings.  */
+
+struct decode_line_2_item
+{
+  /* Provide a string form using symtab_to_fullname.  One has to call xfree
+     for it.  It is set to NULL if it was already chosen by a user.  */
+  char *fullform;
+
+  /* Provide a string form using symtab_to_filename_for_display.  One has to
+     call xfree for it.  */
+  char *displayform;
+};
+
+/* Helper for qsort to sort decode_line_2_item entries by DISPLAYFORM and
+   secondarily by FULLFORM.  */
+
+static int
+decode_line_2_compare_items (const void *ap, const void *bp)
+{
+  const struct decode_line_2_item *a = ap;
+  const struct decode_line_2_item *b = bp;
+  int retval;
+
+  retval = strcmp (a->displayform, b->displayform);
+  if (retval != 0)
+    return retval;
+
+  return strcmp (a->fullform, b->fullform);
+}
+
 /* Handle multiple results in RESULT depending on SELECT_MODE.  This
    will either return normally, throw an exception on multiple
    results, or present a menu to the user.  On return, the SALS vector
@@ -1257,58 +1329,78 @@ decode_line_2 (struct linespec_state *self,
 	       struct symtabs_and_lines *result,
 	       const char *select_mode)
 {
-  const char *iter;
   char *args, *prompt;
   int i;
   struct cleanup *old_chain;
-  VEC (const_char_ptr) *item_names = NULL, *filters = NULL;
+  VEC (const_char_ptr) *filters = NULL;
   struct get_number_or_range_state state;
+  struct decode_line_2_item *items;
+  int items_count;
 
   gdb_assert (select_mode != multiple_symbols_all);
   gdb_assert (self->canonical != NULL);
+  gdb_assert (result->nelts >= 1);
+
+  old_chain = make_cleanup (VEC_cleanup (const_char_ptr), &filters);
 
-  old_chain = make_cleanup (VEC_cleanup (const_char_ptr), &item_names);
-  make_cleanup (VEC_cleanup (const_char_ptr), &filters);
-  for (i = 0; i < result->nelts; ++i)
+  /* Prepare ITEMS array.  */
+  items_count = result->nelts;
+  items = xmalloc (sizeof (*items) * items_count);
+  make_cleanup (xfree, items);
+  for (i = 0; i < items_count; ++i)
     {
-      int j, found = 0;
-      const char *iter;
+      const struct linespec_canonical_name *canonical;
+      struct decode_line_2_item *item;
 
-      gdb_assert (self->canonical_names[i] != NULL);
-      for (j = 0; VEC_iterate (const_char_ptr, item_names, j, iter); ++j)
+      canonical = &self->canonical_names[i];
+      gdb_assert (canonical->suffix != NULL);
+      item = &items[i];
+
+      item->fullform = canonical_to_fullform (canonical);
+      make_cleanup (xfree, item->fullform);
+
+      if (canonical->symtab == NULL)
+	item->displayform = canonical->suffix;
+      else
 	{
-	  if (strcmp (iter, self->canonical_names[i]) == 0)
-	    {
-	      found = 1;
-	      break;
-	    }
+	  const char *fn_for_display;
+	  
+	  fn_for_display = symtab_to_filename_for_display (canonical->symtab);
+	  item->displayform = xstrprintf ("%s:%s", fn_for_display,
+					  canonical->suffix);
+	  make_cleanup (xfree, item->displayform);
 	}
+    }
+
+  /* Sort the list of method names.  */
+  qsort (items, items_count, sizeof (*items), decode_line_2_compare_items);
 
-      if (!found)
-	VEC_safe_push (const_char_ptr, item_names, self->canonical_names[i]);
+  /* Remove entries with the same FULLFORM.  */
+  if (items_count >= 2)
+    {
+      struct decode_line_2_item *dst, *src;
+
+      dst = items;
+      for (src = &items[1]; src < &items[items_count]; src++)
+	if (strcmp (src->fullform, dst->fullform) != 0)
+	  *++dst = *src;
+      items_count = dst + 1 - items;
     }
 
-  if (select_mode == multiple_symbols_cancel
-      && VEC_length (const_char_ptr, item_names) > 1)
+  if (select_mode == multiple_symbols_cancel && items_count > 1)
     error (_("canceled because the command is ambiguous\n"
 	     "See set/show multiple-symbol."));
   
-  if (select_mode == multiple_symbols_all
-      || VEC_length (const_char_ptr, item_names) == 1)
+  if (select_mode == multiple_symbols_all || items_count == 1)
     {
       do_cleanups (old_chain);
       convert_results_to_lsals (self, result);
       return;
     }
 
-  /* Sort the list of method names alphabetically.  */
-  qsort (VEC_address (const_char_ptr, item_names),
-	 VEC_length (const_char_ptr, item_names),
-	 sizeof (const_char_ptr), compare_strings);
-
   printf_unfiltered (_("[0] cancel\n[1] all\n"));
-  for (i = 0; VEC_iterate (const_char_ptr, item_names, i, iter); ++i)
-    printf_unfiltered ("[%d] %s\n", i + 2, iter);
+  for (i = 0; i < items_count; i++)
+    printf_unfiltered ("[%d] %s\n", i + 2, items[i].displayform);
 
   prompt = getenv ("PS2");
   if (prompt == NULL)
@@ -1343,16 +1435,16 @@ decode_line_2 (struct linespec_state *self,
 	}
 
       num -= 2;
-      if (num >= VEC_length (const_char_ptr, item_names))
+      if (num >= items_count)
 	printf_unfiltered (_("No choice number %d.\n"), num);
       else
 	{
-	  const char *elt = VEC_index (const_char_ptr, item_names, num);
+	  struct decode_line_2_item *item = &items[num];
 
-	  if (elt != NULL)
+	  if (item->fullform != NULL)
 	    {
-	      VEC_safe_push (const_char_ptr, filters, elt);
-	      VEC_replace (const_char_ptr, item_names, num, NULL);
+	      VEC_safe_push (const_char_ptr, filters, item->fullform);
+	      item->fullform = NULL;
 	    }
 	  else
 	    {
@@ -2326,8 +2418,8 @@ decode_line_full (char **argptr, int flags,
       make_cleanup (xfree, state->canonical_names);
       for (i = 0; i < result.nelts; ++i)
 	{
-	  gdb_assert (state->canonical_names[i] != NULL);
-	  make_cleanup (xfree, state->canonical_names[i]);
+	  gdb_assert (state->canonical_names[i].suffix != NULL);
+	  make_cleanup (xfree, state->canonical_names[i].suffix);
 	}
     }
 
diff --git a/gdb/testsuite/gdb.linespec/base/one/thefile.cc b/gdb/testsuite/gdb.linespec/base/one/thefile.cc
index f8712c2..0417b7a 100644
--- a/gdb/testsuite/gdb.linespec/base/one/thefile.cc
+++ b/gdb/testsuite/gdb.linespec/base/one/thefile.cc
@@ -9,9 +9,14 @@
 
 
 
+static int twodup ()
+{
+  return 0;
+}
+
 int m(int x)
 {
-  return x + 23;		/* thefile breakpoint */
+  return x + 23 + twodup ();	/* thefile breakpoint */
 }
 
 int NameSpace::overload(int x)
diff --git a/gdb/testsuite/gdb.linespec/base/two/thefile.cc b/gdb/testsuite/gdb.linespec/base/two/thefile.cc
index f0c04cc..88188a5 100644
--- a/gdb/testsuite/gdb.linespec/base/two/thefile.cc
+++ b/gdb/testsuite/gdb.linespec/base/two/thefile.cc
@@ -9,10 +9,15 @@ static int dupname(int y)
  label: return y;
 }
 
+static int twodup ()
+{
+  return 0;
+}
+
 int n(int y)
 {
   int v = dupname(y) - 23;	/* thefile breakpoint */
-  return v;			/* after dupname */
+  return v + twodup ();		/* after dupname */
 }
 
 int NameSpace::overload(double x)
diff --git a/gdb/testsuite/gdb.linespec/break-ask.exp b/gdb/testsuite/gdb.linespec/break-ask.exp
new file mode 100644
index 0000000..10945f5
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/break-ask.exp
@@ -0,0 +1,100 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile lspec.cc
+
+if {[skip_cplus_tests]} {
+    unsupported ${testfile}.exp
+    return
+}
+
+set opts {debug c++}
+set objfile1 [standard_output_file ${testfile}one.o]
+set objfile2 [standard_output_file ${testfile}two.o]
+
+if { [file pathtype $objdir] == "relative" } {
+    untested "objdir $objdir should be absolute"
+    return
+}
+set saved_pwd [pwd]
+cd $srcdir/${subdir}/base/one
+set err1 [gdb_compile "thefile.cc" $objfile1 object $opts]
+cd $saved_pwd
+cd $srcdir/${subdir}/base/two
+set err2 [gdb_compile "thefile.cc" $objfile2 object $opts]
+cd $saved_pwd
+if { $err1 != "" || $err2 != "" } {
+    untested "compilation failed"
+    return -1
+}
+
+if { [gdb_compile "$srcdir/${subdir}/$srcfile $objfile1 $objfile2" \
+		  $binfile executable $opts] != "" } {
+    return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test_no_output "set multiple-symbols ask"
+
+gdb_test_no_output "set filename-display absolute"
+set cmd "break twodup"
+set test "break twodup absolute"
+gdb_test_multiple $cmd $test {
+    -re "^$cmd\r\n\\\[0\\\] cancel\r\n\\\[1\\\] all\r\n\\\[2\\\] \[^\r\n\]+/base/one/thefile\\.cc:twodup\\\(\\\)\r\n\\\[3\\\] \[^\r\n\]+/base/two/thefile\\.cc:twodup\\\(\\\)\r\n> $" {
+	pass $test
+    }
+}
+gdb_test "0" "canceled"
+
+gdb_test_no_output "set filename-display relative"
+
+set cmd "break twodup"
+set test "break twodup relative"
+gdb_test_multiple $cmd $test {
+    -re "^$cmd\r\n\\\[0\\\] cancel\r\n\\\[1\\\] all\r\n\\\[2\\\] thefile\\.cc:twodup\\\(\\\)\r\n\\\[3\\\] thefile\\.cc:twodup\\\(\\\)\r\n> $" {
+	pass $test
+    }
+}
+gdb_test "2" "^2\r\nBreakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file thefile\\.cc, line \[0-9a-f\]+\\."
+
+gdb_breakpoint "body_elsewhere"
+
+gdb_run_cmd
+gdb_test "" "Breakpoint \[0-9\]+, twodup \\(\\) at thefile.cc:\[0-9\]+\r\n.*" "expect breakpoint"
+
+gdb_test "info source" "\r\nLocated in \[^\r\n\]+/base/one/thefile\\.cc\r\n.*"
+
+gdb_continue_to_breakpoint "body_elsewhere" ".* body_elsewhere marker .*"
+
+delete_breakpoints
+
+set cmd "break twodup"
+set test "break twodup relative other"
+gdb_test_multiple $cmd $test {
+    -re "^$cmd\r\n\\\[0\\\] cancel\r\n\\\[1\\\] all\r\n\\\[2\\\] thefile\\.cc:twodup\\\(\\\)\r\n\\\[3\\\] thefile\\.cc:twodup\\\(\\\)\r\n> $" {
+	pass $test
+    }
+}
+gdb_test "3" "^3\r\nBreakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file thefile\\.cc, line \[0-9a-f\]+\\."
+
+gdb_breakpoint "body_elsewhere"
+
+gdb_run_cmd
+gdb_test "" "Breakpoint \[0-9\]+, twodup \\(\\) at thefile.cc:\[0-9\]+\r\n.*" "expect breakpoint other"
+
+gdb_test "info source" "\r\nLocated in \[^\r\n\]+/base/two/thefile\\.cc\r\n.*" "info source other"
+
+gdb_continue_to_breakpoint "body_elsewhere other" ".* body_elsewhere marker .*"
diff --git a/gdb/testsuite/gdb.linespec/lspec.cc b/gdb/testsuite/gdb.linespec/lspec.cc
index b1092e2..bb660fb 100644
--- a/gdb/testsuite/gdb.linespec/lspec.cc
+++ b/gdb/testsuite/gdb.linespec/lspec.cc
@@ -9,7 +9,7 @@ int NameSpace::overload()
 
 int body_elsewhere()
 {
-  int x = 5;
+  int x = 5;	/* body_elsewhere marker */
 #include "body.h"
 }