This is the mail archive of the gdb@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

start_subfile oddities


Hi.  This is a bit long so I've split this into short and long versions.

Short version:

The appended suspect1.patch breaks the setting of breakpoints by
file:lineno.  The appended suspect2.patch fixes some cases but not
all.  grep for session.log below.  My current thinking is to have a
new version of FILENAME_CMP that properly handles different spellings
of path names and use that in appropriate places to compare file
names.  Does this seem like it's heading in the right direction? No
claim is made that it is of course. :-)

Long version:

I'm trying to understand the appended patches: suspect1.patch,
suspect2.patch.  The first patch causes "break file.c:3" to no longer
work for the three appended testcases: bug1.c, bug2.c, bug3.c (see the
sample session below).  The second patch fixes the situation for some
cases but not all.  The basic problem is that:

- multiple subfiles are created for the same file,
- linetable info is recorded with one of the subfiles,
- but during lookup the other subfile is used (without linetable info)
and setting the breakpoint fails with "No line 3 in file.c."

A search is done to find the other subfile in find_line_symtab, so the
code is trying to handle this situation, but the strcmp of the file
names fails.  The search would still fail if FILENAME_CMP were used
because it cares about path spellings.

1) It seems that, as a matter of cleanliness, if file is an absolute
path, or dirname+file is an absolute path, then comp_dir should not be
passed as the dirname argument to start_subfile in dwarf2read.c.  I
see the comment in dwarf2read.c:dwarf2_start_subfile saying the goal
of the current behaviour is consistency.

  /* While reading the DIEs, we call start_symtab(DW_AT_name,
DW_AT_comp_dir).
     `start_symtab' will always pass the contents of DW_AT_comp_dir as
     second argument to start_subfile.  To be consistent, we do the
     same here.  In order not to lose the line information directory,
     we concatenate it to the filename when it makes sense.
     Note that the Dwarf3 standard says (speaking of filenames in line
     information): ``The directory index is ignored for file names
     that represent full path names''.  Thus ignoring dirname in the
     `else' branch below isn't an issue.  */

But the comment for start_subfile says this:

/* Start recording information about source code that came from an
   included (or otherwise merged-in) source file with a different
   name.  NAME is the name of the file (cannot be NULL), **** DIRNAME
is
   the directory in which it resides (or NULL if not known). **** */

[**** emphasis added]

If the file name is /tmp/foo/bar.c and comp_dir is /tmp/baz, passing
/tmp/baz as dirname to start_subfile, while perhaps innocuous, can't
be good.  It may not cause any bugs (today), but it makes tracking
them down harder.  OTOH comp_dir is presumably useful if the file's
directory is unspecified or is a relative path.  I could be missing
something though.

2) Fixing the underlying problem seems to require canonicalizing path
names before doing a file name comparison.  If one canonicalized path
names during subfile processing we'd avoid creating (seemingly)
unnecessary extra subfiles.  This in and of itself seems like a good
thing.  Am I missing something?  Or one could canonicalize file names
in places like find_line_symtab before doing the strcmp here:

        if (strcmp (symtab->filename, p->filename) != 0)
          continue;

find_line_symtab does this twice though, once to do PSYMTAB_TO_SYMTAB
calls, and another time to find a matching symtab.  This suggests
caching the comparable form of the name in the  symtab, but if that's
the case one might just store symtab->filename in comparable form in
the first place.

I tried using xfullpath to record a canonicalized path in
symtab->filename but that causes failures in the testsuite, relative
paths became absolute.  Preserving relativeness of paths seems like a
good thing so I abandoned that.  I think a solution would be to call
something like xfullpath right before calling strcmp, but I'm not sure
of the performance issues.  One could cache the "comparable filename"
in the symtab struct but that seems like a long term source of bugs.
Plus xfullpath uses gdb_realpath which has issues (though it is used
in a few places in gdb).  Trying to handle every possible case of
/path1 == /path2 may be excessive, but a solution that headed in that
direction seems reasonable.  I thought of using FILENAME_CMP in
find_line_symtab (which maybe it should be doing anyway), but it's
defined to not handle different spellings of of the same paths.  And
for relative path names one might want to take into account the
directory search list.

One possibility is to have a version of FILENAME_CMP that doesn't care
about path spellings and use that.  It could keep an internal map of
char* path -> char* canonicalized_path, and perhaps the cache could be
cleared of relative entries when the source search directory list
changed.  I'm really just "thinking out loud" here but it feels right.

Is any of this headed in the right direction? [no claim is made that it is]

I'd also like to add the appended testcases to the testsuite but they
require adjusting the path arg to #line.  I'm guessing it'd be ok to
machine generate them from a template and stuff in the correct
pathname at "make check" time.  Comments?

If you've gotten this far, thanks.

--- snip --- session.log ---

Notice that with gdb 6.4 I can set a breakpoint on bug[123].c:3 just
fine, whereas in 6.7.1 I can only set a breakpoint on bug1.c:3.  The
problem was introduced in gdb 6.5.

dje@ruffy:/tmp$ cat bug1.c
#line 2 "/tmp/bug1.c"

int main () {return 0;}
dje@ruffy:/tmp$ gcc -g bug1.c -o bug1.x
dje@ruffy:/tmp$ cat bug2.c
#line 2 "//tmp/bug2.c"

int main () {return 0;}
dje@ruffy:/tmp$ gcc -g bug2.c -o bug2.x
dje@ruffy:/tmp$ cat bug3.c
#line 2 "./bug3.c"

int main () {return 0;}
dje@ruffy:/tmp$ gcc -g bug3.c -o bug3.x
dje@ruffy:/tmp$ ~/local/gnu/obj-gdb-6.4/gdb/gdb bug1.x
GNU gdb 6.4
Copyright 2005 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-linux"...Using host libthread_db
library "/lib/tls/i686/cmov/libthread_db.so.1".

(gdb) b bug1.c:3
Breakpoint 1 at 0x804832c: file bug1.c, line 3.
(gdb) q
dje@ruffy:/tmp$ ~/local/gnu/obj-gdb-6.4/gdb/gdb bug2.x
GNU gdb 6.4
Copyright 2005 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-linux"...Using host libthread_db
library "/lib/tls/i686/cmov/libthread_db.so.1".

(gdb) b bug2.c:3
Breakpoint 1 at 0x804832c: file bug2.c, line 3.
(gdb) q
dje@ruffy:/tmp$ ~/local/gnu/obj-gdb-6.4/gdb/gdb bug3.x
GNU gdb 6.4
Copyright 2005 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-linux"...Using host libthread_db
library "/lib/tls/i686/cmov/libthread_db.so.1".

(gdb) b bug3.c:3
Breakpoint 1 at 0x804832c: file bug3.c, line 3.
(gdb) q
dje@ruffy:/tmp$ ~/local/gnu/obj-gdb-6.7.1/gdb/gdb bug1.x
GNU gdb 6.7.1
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-linux"...
Using host libthread_db library "/lib/tls/i686/cmov/libthread_db.so.1".
(gdb) b bug1.c:3
Breakpoint 1 at 0x804832c: file bug1.c, line 3.
(gdb) q
dje@ruffy:/tmp$ ~/local/gnu/obj-gdb-6.7.1/gdb/gdb bug2.x
GNU gdb 6.7.1
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-linux"...
Using host libthread_db library "/lib/tls/i686/cmov/libthread_db.so.1".
(gdb) b bug2.c:3
No line 3 in file "bug2.c".
(gdb) q
dje@ruffy:/tmp$ ~/local/gnu/obj-gdb-6.7.1/gdb/gdb bug3.x
GNU gdb 6.7.1
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-linux"...
Using host libthread_db library "/lib/tls/i686/cmov/libthread_db.so.1".
(gdb) b bug3.c:3
No line 3 in file "bug3.c".
(gdb) q
dje@ruffy:/tmp$

--- snip --- suspect1.patch ---

2006-04-21  Frederic Riss  <frederic.riss@st.com>

	* dwarf2read.c (dwarf2_start_subfile): Change prototype to accept
	compilation directory as last argument.
	Always pass comp_dir as second argument to start_subfile and prepend
	dirname to the filename when necessary.
	Remove now superfluous search for pre-existing subfile.
	(dwarf_decode_lines): Pass the compilation directory to
	dwarf2_start_subfile.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.193
retrieving revision 1.194
diff -u -p -r1.193 -r1.194
--- dwarf2read.c	9 Feb 2006 18:18:41 -0000	1.193
+++ dwarf2read.c	21 Apr 2006 20:26:07 -0000	1.194
@@ -846,7 +846,7 @@ static struct line_header *(dwarf_decode
 static void dwarf_decode_lines (struct line_header *, char *, bfd *,
 				struct dwarf2_cu *, struct partial_symtab *);

-static void dwarf2_start_subfile (char *, char *);
+static void dwarf2_start_subfile (char *, char *, char *);

 static struct symbol *new_symbol (struct die_info *, struct type *,
 				  struct dwarf2_cu *);
@@ -6529,13 +6529,12 @@ dwarf_decode_lines (struct line_header *
 	     directory and file name numbers in the statement program
 	     are 1-based.  */
           struct file_entry *fe = &lh->file_names[file - 1];
-          char *dir;
+          char *dir = NULL;

           if (fe->dir_index)
             dir = lh->include_dirs[fe->dir_index - 1];
-          else
-            dir = comp_dir;
-	  dwarf2_start_subfile (fe->name, dir);
+
+	  dwarf2_start_subfile (fe->name, dir, comp_dir);
 	}

       /* Decode the table.  */
@@ -6627,17 +6626,16 @@ dwarf_decode_lines (struct line_header *
                    0-based, but the directory and file name numbers in
                    the statement program are 1-based.  */
                 struct file_entry *fe;
-                char *dir;
+                char *dir = NULL;

                 file = read_unsigned_leb128 (abfd, line_ptr, &bytes_read);
                 line_ptr += bytes_read;
                 fe = &lh->file_names[file - 1];
                 if (fe->dir_index)
                   dir = lh->include_dirs[fe->dir_index - 1];
-                else
-                  dir = comp_dir;
+
                 if (!decode_for_pst_p)
-                  dwarf2_start_subfile (fe->name, dir);
+                  dwarf2_start_subfile (fe->name, dir, comp_dir);
               }
 	      break;
 	    case DW_LNS_set_column:
@@ -6717,7 +6715,8 @@ dwarf_decode_lines (struct line_header *

 /* Start a subfile for DWARF.  FILENAME is the name of the file and
    DIRNAME the name of the source directory which contains FILENAME
-   or NULL if not known.
+   or NULL if not known.  COMP_DIR is the compilation directory for the
+   linetable's compilation unit or NULL if not known.
    This routine tries to keep line numbers from identical absolute and
    relative file names in a common subfile.

@@ -6733,31 +6732,35 @@ dwarf_decode_lines (struct line_header *
    files.files[1].dir:  /srcdir

    The line number information for list0.c has to end up in a single
-   subfile, so that `break /srcdir/list0.c:1' works as expected.  */
-
-static void
-dwarf2_start_subfile (char *filename, char *dirname)
-{
-  /* If the filename isn't absolute, try to match an existing subfile
-     with the full pathname.  */
+   subfile, so that `break /srcdir/list0.c:1' works as expected.
+   start_subfile will ensure that this happens provided that we pass the
+   concatenation of files.files[1].dir and files.files[1].name as the
+   subfile's name.  */
+
+static void
+dwarf2_start_subfile (char *filename, char *dirname, char *comp_dir)
+{
+  char *fullname;
+
+  /* While reading the DIEs, we call start_symtab(DW_AT_name, DW_AT_comp_dir).
+     `start_symtab' will always pass the contents of DW_AT_comp_dir as
+     second argument to start_subfile.  To be consistent, we do the
+     same here.  In order not to lose the line information directory,
+     we concatenate it to the filename when it makes sense.
+     Note that the Dwarf3 standard says (speaking of filenames in line
+     information): ``The directory index is ignored for file names
+     that represent full path names''.  Thus ignoring dirname in the
+     `else' branch below isn't an issue.  */

   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
-    {
-      struct subfile *subfile;
-      char *fullname = concat (dirname, "/", filename, (char *)NULL);
+    fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);
+  else
+    fullname = filename;

-      for (subfile = subfiles; subfile; subfile = subfile->next)
-	{
-	  if (FILENAME_CMP (subfile->name, fullname) == 0)
-	    {
-	      current_subfile = subfile;
-	      xfree (fullname);
-	      return;
-	    }
-	}
-      xfree (fullname);
-    }
-  start_subfile (filename, dirname);
+  start_subfile (fullname, comp_dir);
+
+  if (fullname != filename)
+    xfree (fullname);
 }

 static void

--- snip --- suspect2.patch ---

2007-05-14  Maxim Grigoriev  <maxim2405@gmail.com>

	* buildsym.c (start_subfile): Handle absolute pathnames
	while comparing subfile names.

Index: buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.47
retrieving revision 1.48
diff -u -p -r1.47 -r1.48
--- buildsym.c	27 Feb 2007 22:57:42 -0000	1.47
+++ buildsym.c	14 May 2007 14:10:35 -0000	1.48
@@ -549,11 +549,27 @@ start_subfile (char *name, char *dirname

   for (subfile = subfiles; subfile; subfile = subfile->next)
     {
-      if (FILENAME_CMP (subfile->name, name) == 0)
+      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)
 	{
 	  current_subfile = subfile;
+	  if (subfile_name != subfile->name)
+	    xfree (subfile_name);
 	  return;
 	}
+      if (subfile_name != subfile->name)
+	xfree (subfile_name);
     }

   /* This subfile is not known.  Add an entry for it. Make an entry


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]