RFC: example fix to pr 2360

RFC: example fix to pr 2360

[Douglas Evans]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

To make things more concrete to what I was refering to in
http://sourceware.org/ml/gdb/2007-11/msg00138.html I've filed pr 2360,
and attached is an example fix.

I'm not sufficiently happy with it that I'd submit it RFA, but that's
mostly because of perceived objections to the use of xfullpath.  Maybe
the thing to do is write a canonicalizer like what is done in
cli-cmds.c:cd_command (and presumably move it to a more generic
place).

Suggestions?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-071115-hashline-1.patch --]
[-- Type: text/x-patch; name=gdb-071115-hashline-1.patch, Size: 10668 bytes --]

2007-11-15  Doug Evans  <dje@google.com>

	Fix PR 2360.
	* buildsym.h (struct subfile): New member `comparable_name'.
	* buildsym.c (start_subfile): Record comparable_name, use it when
	comparing new subfiles.
	(end_symtab): Free comparable_name.
	(canonicalize_subfile_path): New fn.

	* gdb.base/hashline1.exp: New file.
	* gdb.base/hashline2.exp: New file.
	* gdb.base/hashline3.exp: New file.

Index: buildsym.h
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.h,v
retrieving revision 1.18
diff -u -p -r1.18 buildsym.h
--- buildsym.h	15 Nov 2007 22:54:22 -0000	1.18
+++ buildsym.h	15 Nov 2007 23:37:11 -0000
@@ -63,6 +63,7 @@ struct subfile
     struct subfile *next;
     char *name;
     char *dirname;
+    char *comparable_name;
     struct linetable *line_vector;
     int line_vector_length;
     enum language language;
Index: buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.57
diff -u -p -r1.57 buildsym.c
--- buildsym.c	15 Nov 2007 22:54:22 -0000	1.57
+++ buildsym.c	15 Nov 2007 23:37:11 -0000
@@ -68,6 +68,7 @@ static struct pending *free_pendings;
 
 static int have_line_numbers;
 \f
+static char *canonicalize_subfile_path (const char *path);
 static int compare_line_numbers (const void *ln1p, const void *ln2p);
 \f
 
@@ -517,33 +518,31 @@ void
 start_subfile (char *name, char *dirname)
 {
   struct subfile *subfile;
+  char *comparable_name;
+
+  /* Create a version of name that can be used to match different spellings.
+     PR 2360 */
+  {
+    char *p = name;
+    if (! IS_ABSOLUTE_PATH (name)
+	&& dirname != NULL)
+      p = concat (dirname, SLASH_STRING, name, NULL);
+    comparable_name = canonicalize_subfile_path (p);
+    if (p != name)
+      xfree (p);
+  }
 
   /* See if this subfile is already known as a subfile of the current
      main source file.  */
 
   for (subfile = subfiles; subfile; subfile = subfile->next)
     {
-      char *subfile_name;
-
-      /* If NAME is an absolute path, and this subfile is not, then
-	 attempt to create an absolute path to compare.  */
-      if (IS_ABSOLUTE_PATH (name)
-	  && !IS_ABSOLUTE_PATH (subfile->name)
-	  && subfile->dirname != NULL)
-	subfile_name = concat (subfile->dirname, SLASH_STRING,
-			       subfile->name, NULL);
-      else
-	subfile_name = subfile->name;
-
-      if (FILENAME_CMP (subfile_name, name) == 0)
+      if (FILENAME_CMP (subfile->comparable_name, comparable_name) == 0)
 	{
 	  current_subfile = subfile;
-	  if (subfile_name != subfile->name)
-	    xfree (subfile_name);
+	  xfree (comparable_name);
 	  return;
 	}
-      if (subfile_name != subfile->name)
-	xfree (subfile_name);
     }
 
   /* This subfile is not known.  Add an entry for it. Make an entry
@@ -560,6 +559,7 @@ start_subfile (char *name, char *dirname
   subfile->name = (name == NULL) ? NULL : savestring (name, strlen (name));
   subfile->dirname =
     (dirname == NULL) ? NULL : savestring (dirname, strlen (dirname));
+  subfile->comparable_name = comparable_name;
 
   /* Initialize line-number recording for this subfile.  */
   subfile->line_vector = NULL;
@@ -615,6 +615,16 @@ start_subfile (char *name, char *dirname
     }
 }
 
+/* Subroutine of start_subfile to canonicalize a path for comparison purposes.
+   Here we care that multiple spellings of the same path map to the same
+   file.  Space for the result is allocated with malloc.  */
+
+static char *
+canonicalize_subfile_path (const char *path)
+{
+  return xfullpath (path);
+}
+
 /* For stabs readers, the first N_SO symbol is assumed to be the
    source file name, and the subfile struct is initialized using that
    assumption.  If another N_SO symbol is later seen, immediately
@@ -1039,6 +1049,10 @@ end_symtab (CORE_ADDR end_addr, struct o
 	{
 	  xfree ((void *) subfile->dirname);
 	}
+      if (subfile->comparable_name != NULL)
+	{
+	  xfree ((void *) subfile->comparable_name);
+	}
       if (subfile->line_vector != NULL)
 	{
 	  xfree ((void *) subfile->line_vector);
Index: testsuite/gdb.base/hashline1.exp
===================================================================
RCS file: testsuite/gdb.base/hashline1.exp
diff -N testsuite/gdb.base/hashline1.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/hashline1.exp	15 Nov 2007 23:37:11 -0000
@@ -0,0 +1,58 @@
+# Copyright 2007 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/>.
+
+# This file is part of the gdb testsuite.
+
+# Test loading of line number information with absolute path in #line, bug 2360.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 2360
+set bug_id 0
+
+# srcfile is in objdir because we need to machine generate it in order
+# to get correct the correct path in the #line directive.
+
+set testfile "hashline1"
+set srcfile "${testfile}.c"
+set binfile "${objdir}/${subdir}/${testfile}"
+
+set fd [open ${objdir}/${subdir}/${srcfile} w]
+puts $fd "#line 2 \"[pwd]/${subdir}/${srcfile}\""
+puts $fd "int main () { return 0; } /* set breakpoint here */"
+close $fd
+
+# The choice of path name for the source file is important in order to
+# trigger the bug.  Using ${objdir}/${subdir}/${srcfile} here won't trigger
+# the bug.
+if { [gdb_compile "./${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested hashline1.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "set breakpoint here" ${objdir}/${subdir}/${srcfile}]
+
+# Try to set a breakpoint on the specified file location.
+
+gdb_test "break $srcfile:$bp_location" \
+    "Breakpoint.*at.* file .*$srcfile, line.*" \
+    "set breakpoint"
Index: testsuite/gdb.base/hashline2.exp
===================================================================
RCS file: testsuite/gdb.base/hashline2.exp
diff -N testsuite/gdb.base/hashline2.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/hashline2.exp	15 Nov 2007 23:37:11 -0000
@@ -0,0 +1,56 @@
+# Copyright 2007 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/>.
+
+# This file is part of the gdb testsuite.
+
+# Test loading of line number information with an absolute path with extra
+# /'s in #line, bug 2360.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 2360
+set bug_id 0
+
+# srcfile is in objdir because we need to machine generate it in order
+# to get correct the correct path in the #line directive.
+
+set testfile "hashline2"
+set srcfile "${testfile}.c"
+set binfile "${objdir}/${subdir}/${testfile}"
+
+set fd [open ${objdir}/${subdir}/${srcfile} w]
+puts $fd "#line 2 \"///[pwd]/${subdir}/${srcfile}\""
+puts $fd "int main () { return 0; } /* set breakpoint here */"
+close $fd
+
+if { [gdb_compile "${objdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested hashline1.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "set breakpoint here" ${objdir}/${subdir}/${srcfile}]
+
+# Try to set a breakpoint on the specified file location.
+
+gdb_test "break $srcfile:$bp_location" \
+    "Breakpoint.*at.* file .*$srcfile, line.*" \
+    "set breakpoint"
Index: testsuite/gdb.base/hashline3.exp
===================================================================
RCS file: testsuite/gdb.base/hashline3.exp
diff -N testsuite/gdb.base/hashline3.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/hashline3.exp	15 Nov 2007 23:37:11 -0000
@@ -0,0 +1,55 @@
+# Copyright 2007 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/>.
+
+# This file is part of the gdb testsuite.
+
+# Test loading of line number information with relative path in #line, bug 2360.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 2360
+set bug_id 0
+
+# srcfile is in objdir because we need to machine generate it in order
+# to get correct the correct path in the #line directive.
+
+set testfile "hashline3"
+set srcfile "${testfile}.c"
+set binfile "${objdir}/${subdir}/${testfile}"
+
+set fd [open ${objdir}/${subdir}/${srcfile} w]
+puts $fd "#line 2 \"./${subdir}/${srcfile}\""
+puts $fd "int main () { return 0; } /* set breakpoint here */"
+close $fd
+
+if { [gdb_compile "${objdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested hashline1.exp
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+set bp_location [gdb_get_line_number "set breakpoint here" ${objdir}/${subdir}/${srcfile}]
+
+# Try to set a breakpoint on the specified file location.
+
+gdb_test "break $srcfile:$bp_location" \
+    "Breakpoint.*at.* file .*$srcfile, line.*" \
+    "set breakpoint"

Re: RFC: example fix to pr 2360

[Douglas Evans]

On Nov 15, 2007 3:55 PM, Douglas Evans <dje@google.com> wrote:
> To make things more concrete to what I was refering to in
> http://sourceware.org/ml/gdb/2007-11/msg00138.html I've filed pr 2360,
> and attached is an example fix.
>
> I'm not sufficiently happy with it that I'd submit it RFA, but that's
> mostly because of perceived objections to the use of xfullpath.  Maybe
> the thing to do is write a canonicalizer like what is done in
> cli-cmds.c:cd_command (and presumably move it to a more generic
> place).
>
> Suggestions?

Ping.

One use case that xfullpath might not always handle properly is if the
binary is used
on a different host than it was compiled on and the path name doesn't exist.
In this case one would still want the path name canonicalized but
xfullpath won't work.

Is there anything I'm missing?

TIA

Re: RFC: example fix to pr 2360

[Daniel Jacobowitz]

On Thu, Nov 15, 2007 at 03:55:41PM -0800, Douglas Evans wrote:
> To make things more concrete to what I was refering to in
> http://sourceware.org/ml/gdb/2007-11/msg00138.html I've filed pr 2360,
> and attached is an example fix.
> 
> I'm not sufficiently happy with it that I'd submit it RFA, but that's
> mostly because of perceived objections to the use of xfullpath.  Maybe
> the thing to do is write a canonicalizer like what is done in
> cli-cmds.c:cd_command (and presumably move it to a more generic
> place).
> 
> Suggestions?

Why should we canonicalize paths at all?  Definitely the use of
xfullpath is suspect on anything read from debug information.  Those
are paths on some system where the program was compiled, not paths on
the system where GDB is running; there are a variety of commands like
"dir" and "set substitute-path" designed to map the one to the other,
and this goes behind their back to look at the filesystem.

The root of the problem seems to me to be inconsistent information in
the source file.  Your debug information describes two files,
e.g. /tmp/bug2.c and //tmp/bug2.c (one via DW_AT_comp_dir, the other
via .debug_line).  There's nothing like an inode in the DWARF, so GDB
has reasonably assumed that they are different files.  Then you say
"bug2.c:2" and it happens to pick the one you didn't mean.

Are there any examples of this without #line?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: example fix to pr 2360

[Doug Evans]

On Dec 16, 2007 4:01 PM, Daniel Jacobowitz <drow@false.org> wrote:
> Are there any examples of this without #line?

Thanks for the review.  I'm not aware of any examples without #line.

While dwarf doesn't understand inodes the paths do have some canonical
behaviour.  e.g. As I understand it, posix says //tmp/foo == /tmp/foo.
 I recognize the paths are from the compilation host not the debug
host, we could write a path canonicalizer.

But no matter, I can certainly pursue a different fix for the problem
at hand, perhaps by adding an option to gcc to set DW_AT_comp_dir.

Re: RFC: example fix to pr 2360

[Daniel Jacobowitz]

On Thu, Dec 20, 2007 at 11:37:13AM -0800, Doug Evans wrote:
> On Dec 16, 2007 4:01 PM, Daniel Jacobowitz <drow@false.org> wrote:
> > Are there any examples of this without #line?
> 
> Thanks for the review.  I'm not aware of any examples without #line.
> 
> While dwarf doesn't understand inodes the paths do have some canonical
> behaviour.  e.g. As I understand it, posix says //tmp/foo == /tmp/foo.
>  I recognize the paths are from the compilation host not the debug
> host, we could write a path canonicalizer.

Actually, I believe this is one of the few places where / and // are
separate, and POSIX allows //tmp to be different from /tmp.  /tmp//foo
is another story.

> But no matter, I can certainly pursue a different fix for the problem
> at hand, perhaps by adding an option to gcc to set DW_AT_comp_dir.

Does -fworking-directory help?  That's the default if you preprocess
with -g, if I understand correctly.

-- 
Daniel Jacobowitz
CodeSourcery