Re: The future of dwarf2_physname

[Tom Tromey <tromey@redhat.com>]

Jan> I have concerns about regressions for gdb-7.3, I would like to fix
Jan> regressions gdb-7.1 (pre-physname) -> gdb-7.2 (physname) but
Jan> possibly not introduce regressions gdb-7.2 (physname) -> gdb-7.3
Jan> (?).

I don't see how that is really possible.
I am willing to accept some minor regressions for 7.3 in exchange for
doing better in the long run.

Jan> I was trying to propose DMGL_RET_POSTFIX but it has other drawbacks
Jan> and it is in fact a new feature (and not a regression fixup)
Jan> because the trailing type was not present in gdb-7.2 anyway.

I think we should not take this approach, for the simple reason that it
will be weird to C++ users.  I think it would be better to just require
quotes in this case for 7.3, like "break 'int f<int> ()'", then see
about fixing up linespec, or whatever, in 7.4+.  Also for later would be
making some kind of symbol alias for this, so that in the normal case
users can omit the return type.

Jan> (gdb) complete p 'f(
Jan> p 'f(int)
Jan> p 'f(t)
Jan>  - I do not find it clear this is right.  There should be
Jan>  definitely the `f(int)' - demangled linkage name - variant.  But
Jan>  whether to show the "convenient" aliases as SYMBOL_SEARCH_NAME I
Jan>  do not find clear, it may be better but it also complicates the
Jan>  list of all the available overloads for a function name, one
Jan>  should ask practical C++ programmer for it IMO.

I think this one is ok, but I can see how that could go either way.
I find it a side issue at present.

Jan> Regarding the proposals to fix dwarf2_physname computation instead
Jan> of preferring DW_AT_linkage_name.  ISTM we have never found the
Jan> agreement with Keith, when DW_AT_linkage_name is present it is
Jan> guaranteed to be correct and I do not know why not to use it.

I think we should move forward with it, on the basis that it is more
correct and probably has better performance to boot.

I suggest something like the appended, on top of Keith's patches and
your patch.  The idea here is to prefer the fast path, but have a debug
setting so we can easily do physname checking.

WDYT?  I didn't run it through the test suite or anything yet.

Jan> Keith proposes to fix dwarf2_physname instead of using
Jan> DW_AT_linkage_name.

We must do both in the medium term.

Tom

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 7530e3e..fff8e3b 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -119,6 +119,9 @@ _STATEMENT_PROLOGUE;
 /* When non-zero, dump DIEs after they are read in.  */
 static int dwarf2_die_debug = 0;
 
+/* When non-zero, cross-check physname against demangler.  */
+static int check_physname = 0;
+
 static int pagesize;
 
 /* When set, the file that we're processing is known to have debugging
@@ -5142,71 +5145,71 @@ dwarf2_full_name (char *name, struct die_info *die, struct dwarf2_cu *cu)
 static const char *
 dwarf2_physname (char *name, struct die_info *die, struct dwarf2_cu *cu)
 {
-  const char *physname = dwarf2_compute_name (name, die, cu, NAME_KIND_PHYS);
+  const char *physname;
   struct attribute *attr;
-  const char *mangled, *retval;
-  char *demangled, *canon;
-  struct cleanup *back_to;
-
-  /* Do not check PHYSNAME if it never will be needed by GDB.  GDB does not
-     need to support something it has no use for.  */
-  if (!die_needs_namespace (die, cu))
-    return physname;
+  const char *retval, *mangled = NULL;
+  char *canon = NULL;
+  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
+  int need_copy = 1;
 
   attr = dwarf2_attr (die, DW_AT_linkage_name, cu);
   if (!attr)
     attr = dwarf2_attr (die, DW_AT_MIPS_linkage_name, cu);
-  if (!attr || !DW_STRING (attr))
+  /* DW_AT_linkage_name is missing in some cases - depend on what GDB
+     has computed.  */
+  if (attr && DW_STRING (attr))
     {
-      /* DW_AT_linkage_name is missing in some cases - depend on what GDB has
-	 computed.  */
-
-      return physname;
-    }
-  mangled = DW_STRING (attr);
-
-  /* As both DW_AT_linkage_name (MANGLED) and computed PHYSNAME are present
-     cross-check them.  */
-
-  back_to = make_cleanup (null_cleanup, 0);
+      char *demangled;
 
-  demangled = cplus_demangle (mangled, DMGL_PARAMS | DMGL_ANSI);
-  if (demangled)
-    make_cleanup (xfree, demangled);
-  else
-    demangled = (char *) mangled;
+      mangled = DW_STRING (attr);
 
-  if (cu->language == language_cplus)
-    {
-      canon = cp_canonicalize_string (demangled);
-      if (canon != NULL)
-	make_cleanup (xfree, canon);
+      demangled = cplus_demangle (mangled, (DMGL_PARAMS | DMGL_ANSI
+					    | DMGL_VERBOSE));
+      if (demangled)
+	{
+	  make_cleanup (xfree, demangled);
+	  canon = demangled;
+	}
       else
-	canon = demangled;
+	{
+	  canon = (char *) mangled;
+	  need_copy = 0;
+	}
     }
-  else
-    canon = demangled;
 
-  if (strcmp (physname, canon) != 0)
+  if (canon == NULL || check_physname)
     {
-      /* It may not mean a bug in GDB.  The compiler could also compute
-	 DW_AT_linkage_name incorrectly.  But in such case GDB would need to be
-	 bug-to-bug compatible.  */
+      physname = dwarf2_compute_name (name, die, cu, NAME_KIND_PHYS);
 
-      complaint (&symfile_complaints,
-		 _("Computed physname <%s> does not match demangled <%s> "
-		   "(from linkage <%s>) - DIE at 0x%x [in module %s]"),
-		 physname, canon, mangled, die->offset, cu->objfile->name);
+      if (canon != NULL && strcmp (physname, canon) != 0)
+	{
+	  /* It may not mean a bug in GDB.  The compiler could also
+	     compute DW_AT_linkage_name incorrectly.  But in such case
+	     GDB would need to be bug-to-bug compatible.  */
+
+	  complaint (&symfile_complaints,
+		     _("Computed physname <%s> does not match demangled <%s> "
+		       "(from linkage <%s>) - DIE at 0x%x [in module %s]"),
+		     physname, canon, mangled, die->offset, cu->objfile->name);
 
-      /* Prefer DW_AT_linkage_name (in the CANON form) - when it is available
-	 here - over computed PHYSNAME.  It is safer against both buggy GDB and
-	 buggy compilers.  */
+	  /* Prefer DW_AT_linkage_name (in the CANON form) - when it
+	     is available here - over computed PHYSNAME.  It is safer
+	     against both buggy GDB and buggy compilers.  */
 
-      retval = obsavestring (canon, strlen (canon),
-			     &cu->objfile->objfile_obstack);
+	  retval = canon;
+	}
+      else
+	{
+	  retval = physname;
+	  need_copy = 0;
+	}
     }
   else
-    retval = physname;
+    retval = canon;
+
+  if (need_copy)
+    retval = obsavestring (retval, strlen (retval),
+			   &cu->objfile->objfile_obstack);
 
   do_cleanups (back_to);
   return retval;
@@ -16247,6 +16250,15 @@ show_dwarf2_always_disassemble (struct ui_file *file, int from_tty,
 		    value);
 }
 
+static void
+show_check_physname (struct ui_file *file, int from_tty,
+		     struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file,
+		    _("Whether to check \"physname\" is %s.\n"),
+		    value);
+}
+
 void _initialize_dwarf2_read (void);
 
 void
@@ -16302,6 +16314,14 @@ The value is the maximum depth to print."),
 			    NULL,
 			    &setdebuglist, &showdebuglist);
 
+  add_setshow_boolean_cmd ("check-physname", no_class, &check_physname, _("\
+Set cross-checking of \"physname\" code against demangler."), _("\
+Show cross-checking of \"physname\" code against demangler."), _("\
+When enabled, GDB's internal \"physname\" code is checked against\n\
+the demangler."),
+			   NULL, show_check_physname,
+			   &setdebuglist, &showdebuglist);
+
   c = add_cmd ("gdb-index", class_files, save_gdb_index_command,
 	       _("\
 Save a gdb-index file.\n\