Re: [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 Sat, 02 Mar 2013 01:07:22 +0100, Joel Brobecker wrote:
> > 	break twodup
> > 	[0] cancel
> > 	[1] all
> > 	[2] thefile.cc:twodup()
> > 	[3] thefile.cc:twodup()
> > 	> PASS: gdb.linespec/break-ask.exp: break twodup relative
> 
> And if I understand your patch correctly, it would also honor
> "set filename-display" right?

Yes.


> Also, this is really a nitpick but the length
> of the lines in your comments is a little too long, and I don't see
> a real reason to exceed our soft limit in those cases (70 characters).

So far I was aware of 72 columns but I cannot find it GDB Coding Standards nor
in GNU Coding Standards, even the GNU Indent commandline does not reformat
comments to any column.  Does Emacs format it to 70/72 columns or where this
rule came from?

While one could make a new GDB COding Standards note TBH I am not much fond in
this rule as VIM (and I guess also other editors) cannot format code to
a different width (80) than comments (72) so emulating the exact behavior of
Emacs by hand even in this case is really inconvenient for new contributors.


> > +{
> > +  /* 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;
> 
>      /* The form using symtab_to_fullname,  [It is set to NULL if
>        it was already chosen by a user.]
> 
>        It must be xfree'ed after use.  */
> 
> I don't understand the comment about "It is set to NULL if it was
> already chosen by a user.", and looking at the code where it is set
> to NULL, I don't quite see what you are trying to say or do with it.
> What does "chosen by the user" mean?

[0] cancel
[1] all
[2] /home/jkratoch/t/overload.C:f()
[3] /home/jkratoch/t/overload.C:f(int)
> 2 2
duplicate request for 0 ignored.

NULL is there to detected the second "2" entry.
(BTW GDB has a bug, it should print "duplicate request for 2 ignored.", I will
check in a fix after this patch is done.)


> I wish I had a little more time
> to try to figure this out. Does it look like what you really want is
> an extra flag (int selected = 0, set to 1 when chosen)?

I have put there the new 'selected' field, it is sub-optimal wrt coding (as the
string value is no longer needed after it has been already chosen by user) but
it may make the code more readable as you suggest.


> > +/* 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)
> > +{
[...]
> > +  return strcmp (a->fullform, b->fullform);
> 
> If fullform can be NULL, are you sure this is OK to do?

(1) FULLFORM can no longer be NULL.  (2) It could be NULL only in later phase
of the code.  During the initial sorting it still could not be NULL.


> Trailing spaces here...
[...]
> > -  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."));
> >    
> 
> Same here...

These are not created by this patch, fixing them is unrelated to this patch.


Other comment suggestions I have just accepted in the patch below.

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


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..327495a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -157,6 +157,21 @@ struct linespec
 };
 typedef struct linespec *linespec_p;
 
+/* A canonical linespec represented as a symtab-related string.
+
+   Each entry represents the "SYMTAB:SUFFIX" linespec string.  SYMTAB can be
+   converted for example by symtab_to_fullname or
+   symtab_to_filename_for_display as needed.  */
+
+struct linespec_canonical_name
+{
+  /* Remaining text part of the linespec string.  */
+  char *suffix;
+
+  /* If NULL then SUFFIX is the whole linespec string.  */
+  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 +201,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 +863,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 +878,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 +1221,19 @@ find_toplevel_string (const char *haystack, const char *needle)
   return NULL;
 }
 
+/* Convert CANONICAL to its string representation using symtab_to_fullname for
+   SYMTAB.  The caller must 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 +1254,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 +1291,44 @@ convert_results_to_lsals (struct linespec_state *self,
   VEC_safe_push (linespec_sals, self->canonical->sals, &lsal);
 }
 
+/* A structure that contains two string representations of a struct
+   linespec_canonical_name:
+     - one where the the symtab's fullname is used;
+     - one where the filename followed the "set filename-display"
+       setting.  */
+
+struct decode_line_2_item
+{
+  /* The form using symtab_to_fullname.
+     It must be xfree'ed after use.  */
+  char *fullform;
+
+  /* The form using symtab_to_filename_for_display.
+     It must be xfree'ed after use.  */
+  char *displayform;
+
+  /* Field is initialized to zero and it is set to one if the user requested
+     breakpoint for this entry.  */
+  unsigned int selected : 1;
+};
+
+/* 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 +1339,80 @@ 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;
+
+      canonical = &self->canonical_names[i];
+      gdb_assert (canonical->suffix != NULL);
+      item = &items[i];
 
-      gdb_assert (self->canonical_names[i] != NULL);
-      for (j = 0; VEC_iterate (const_char_ptr, item_names, j, iter); ++j)
+      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);
 	}
 
-      if (!found)
-	VEC_safe_push (const_char_ptr, item_names, self->canonical_names[i]);
+      item->selected = 0;
     }
 
-  if (select_mode == multiple_symbols_cancel
-      && VEC_length (const_char_ptr, item_names) > 1)
+  /* Sort the list of method names.  */
+  qsort (items, items_count, sizeof (*items), decode_line_2_compare_items);
+
+  /* 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 && 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 +1447,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->selected)
 	    {
-	      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->selected = 1;
 	    }
 	  else
 	    {
@@ -2326,8 +2430,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"
 }