[RFC] Don't loose compilation directory in Dwarf2 line-tables

[RFC] Don't loose compilation directory in Dwarf2 line-tables

[Frederic RISS]

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

Hello,

The handing of relative paths in GDB has already been discussed (see
http://sourceware.org/ml/gdb-patches/2005-10/msg00039.html for the
latest thread). I don't believe there has been an agreement on a
solution.
I'd like to propose a little modification that makes things work for me
in the Dwarf2 case.
In fact, my starting point wasn't the general issue of relative paths,
but I saw GDB unable to find the include files of a program structured
like that:

 /project/
        include/
               header.h
        src/
           main.c  /* Includes header.h */
        main.o
        myprog

If GDB isn't launched from the root of the build tree, it is unable to
find header.h, whereas it can find main.c from anywhere. In the dwarf2
debug info, we get:

   DW_AT_name:          src/main.c
   DW_AT_comp_dir:      /project/
   files.files[0].name: header.h
   files.files[0].dir:  include/
   files.files[1].name: main.c
   files.files[1].dir:  src/

So if the build-tree stays at the same place, the debugger has all the
information available to be able to locate header.h, wherever the binary
really is and whatever $cwd is.

The main symtab will be built using DW_AT_name and DW_AT_comp_dir.
That's why GDB can find the main.c sourcefile without trouble. The
symtab for the header.h file is built using the line info dirname and
the line info filename. Which means it looses the comp_dir information.
I believe that the information in the line-tables related to a Dwarf2
compilation unit should be interpreted relatively to the compilation
unit comp_dir.

The attached patch implements something like that. When the line
information doesn't reference an absolute file, it uses comp_dir as the
subfile directory and concat(file.dir, "/", file.name) as filename.
If the build tree hasn't moved, then we've got all the information to
find the sources. If comp_dir doesn't exist anymore, then
find_and_open_source will basically do the same thing as with the old
information (it will split the basename if it can't find the full name).
I think that this also has the nice side-effect of making relative
include paths working with the 'dir' command as the full relative path
will get tested against each directory.
Note that the include psymtab building code a in dwarf_decode_lines does
a similar thing.
This seems also more consistent with find_and_open_source where $cdir
will get replaced by what we put in the symtab's dirname (implying that
we should always put the comp_dir in a symtab's dirname).

I've regression tested it on i686-pc-linux-gnu with gcc4.0. I wonder if
this could break things in some cases, maybe with other compilers than
GCC. Comments ?

Regards,
Fred.


[-- Attachment #2: dwarf2-relative-paths.patch --]
[-- Type: text/x-patch, Size: 2359 bytes --]

--- dwarf2read.c.orig	2006-04-13 11:17:40.000000000 +0200
+++ dwarf2read.c	2006-04-13 11:25:48.000000000 +0200
@@ -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 *);
@@ -6535,7 +6535,7 @@ dwarf_decode_lines (struct line_header *
             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.  */
@@ -6637,7 +6637,7 @@ dwarf_decode_lines (struct line_header *
                 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:
@@ -6736,7 +6736,7 @@ dwarf_decode_lines (struct line_header *
    subfile, so that `break /srcdir/list0.c:1' works as expected.  */
 
 static void
-dwarf2_start_subfile (char *filename, char *dirname)
+dwarf2_start_subfile (char *filename, char *dirname, char *comp_dir)
 {
   /* If the filename isn't absolute, try to match an existing subfile
      with the full pathname.  */
@@ -6744,7 +6744,7 @@ dwarf2_start_subfile (char *filename, ch
   if (!IS_ABSOLUTE_PATH (filename) && dirname != NULL)
     {
       struct subfile *subfile;
-      char *fullname = concat (dirname, "/", filename, (char *)NULL);
+      char *fullname = concat (dirname, SLASH_STRING, filename, (char *)NULL);
 
       for (subfile = subfiles; subfile; subfile = subfile->next)
 	{
@@ -6755,9 +6755,20 @@ dwarf2_start_subfile (char *filename, ch
 	      return;
 	    }
 	}
+
+      /* Make sure we don't loose the compilation directory
+	 information. */
+      if (dirname != comp_dir) 
+	{
+	  filename = fullname;
+	  dirname = comp_dir;
+	}
+      
+      start_subfile (filename, dirname);
       xfree (fullname);
     }
-  start_subfile (filename, dirname);
+  else
+    start_subfile (filename, dirname);
 }
 
 static void

Re: [RFC] Don't loose compilation directory in Dwarf2 line-tables

[Jim Blandy]

Hi, Frederic.  Thanks for the patch!  I think you're right about how
GDB should interpret DW_AT_comp_dir and directories in the line number
information.  Some comments:

You should generally include a ChangeLog entry along with the patch;
you'll have to write it anyway, and it helps reviewers get oriented
for what they're going to see in the patch itself.

Ideally, the use of SLASH_STRING would be a separate patch, but that's
not a big deal.

When you add a new argument to dwarf2_start_subfile, you need to
adjust the comments above the file that describe what the arguments
mean.

The patch changes the logic of dwarf2_start_subfile in a circuitous
way.  If dirname and comp_dir are absolute and different, and filename
is relative, then you end up passing start_subfile two absolute paths.
 I'd rather see the code changed like this:

- Since dwarf2_start_subfile now knows about comp_dir, there's no need
to use comp_dir as a default in dwarf_decode_lines.  Just pass NULL
for dirname when the dir_index is zero.

- Then, at the top of dwarf2_start_subfile, check if dirname is
relative, and if comp_dir is available, prepend comp_dir to it.  At
this point, we know dirname is as absolute as it can be.  Then proceed
as in the original unpatched code.  (Watch out for allocation issues.)

Also, in the comments, "loose" is the opposite of "tight"; you mean "lose".

Re: [RFC] Don't loose compilation directory in Dwarf2 line-tables

[Jason Molenda]

Frederic,

For what it's worth, I fixed the same bug yesterday.  Our changes
are similar - in dwarf2_start_subfile I used this code at the end
of the function:

  /* APPLE LOCAL: If FILENAME isn't an absolute path and DIRNAME is either
     NULL or a relative path, prepend COMP_DIR on there to get an absolute
     path.  */

  if (!IS_ABSOLUTE_PATH (filename) && dirname == NULL)
    dirname = comp_dir;
  else 
    if (!IS_ABSOLUTE_PATH (filename) && !IS_ABSOLUTE_PATH (dirname))
      {
        dirname = concat (comp_dir, SLASH_STRING, dirname, (char *) NULL);
        make_cleanup (xfree, dirname);
      }
  start_subfile (filename, dirname);
  do_cleanups (clean);


(with 'cleanup' initialized to a null_cleanup at the top of the file,
naturally.)

J

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

Him Jim, 

Thanks for your comments.

On Thu, 2006-04-13 at 10:49 -0700, Jim Blandy wrote:
> Hi, Frederic.  Thanks for the patch!  I think you're right about how
> GDB should interpret DW_AT_comp_dir and directories in the line number
> information.  Some comments:

Great.

> You should generally include a ChangeLog entry along with the patch;
> you'll have to write it anyway, and it helps reviewers get oriented
> for what they're going to see in the patch itself.

Will do for the next round.

> Ideally, the use of SLASH_STRING would be a separate patch, but that's
> not a big deal.

Not a big deal for me to do a separate patch.

> When you add a new argument to dwarf2_start_subfile, you need to
> adjust the comments above the file that describe what the arguments
> mean.

Yep

> The patch changes the logic of dwarf2_start_subfile in a circuitous
> way.  If dirname and comp_dir are absolute and different, and filename
> is relative, then you end up passing start_subfile two absolute paths.
>  I'd rather see the code changed like this:

I agree that the way it was done made dwarf2_start_subfile quite strange
to read.

> - Since dwarf2_start_subfile now knows about comp_dir, there's no need
> to use comp_dir as a default in dwarf_decode_lines.  Just pass NULL
> for dirname when the dir_index is zero.

That's a good idea, it makes things simpler. I've a cleaned up patch for
that, but I'm not sure sure about your next proposal:

> - Then, at the top of dwarf2_start_subfile, check if dirname is
> relative, and if comp_dir is available, prepend comp_dir to it.  At
> this point, we know dirname is as absolute as it can be.  Then proceed
> as in the original unpatched code.  (Watch out for allocation issues.)

I don't think we should concat comp_dir and dirname. In my mind,
comp_dir makes only sense as an 'independant' information. Or the other
way around: dirname gives you information about your source tree
structure, and you lose it by prepending comp_dir to it. Have you an
objection to storing "dirname/filename" as filename and comp_dir as
directory? Maybe this makes only sense when dirname is also relative
like in the snippet that Jason posted. Once we agree on this part, I'll
post an updated patch.

> Also, in the comments, "loose" is the opposite of "tight"; you mean "lose".

Doh... back to school :-)

Regards,
Fred

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Jim Blandy]

On 4/14/06, Frederic RISS <frederic.riss@st.com> wrote:
> > - Then, at the top of dwarf2_start_subfile, check if dirname is
> > relative, and if comp_dir is available, prepend comp_dir to it.  At
> > this point, we know dirname is as absolute as it can be.  Then proceed
> > as in the original unpatched code.  (Watch out for allocation issues.)
>
> I don't think we should concat comp_dir and dirname. In my mind,
> comp_dir makes only sense as an 'independant' information. Or the other
> way around: dirname gives you information about your source tree
> structure, and you lose it by prepending comp_dir to it. Have you an
> objection to storing "dirname/filename" as filename and comp_dir as
> directory? Maybe this makes only sense when dirname is also relative
> like in the snippet that Jason posted. Once we agree on this part, I'll
> post an updated patch.

We agree that, when dirname is absolute, comp_dir shouldn't get used at all.

When dirname is relative, I guess we lose information by concatenating
it with comp_dir, but I don't see where we would ever use that
information.  I think the filename passed to start_subfile should be
free of leading directory components; I believe this is necessary to
make things like "print 'foo.c'::x" work.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

On Fri, 2006-04-14 at 00:41 -0700, Jim Blandy wrote:
> On 4/14/06, Frederic RISS <frederic.riss@st.com> wrote:
> > > - Then, at the top of dwarf2_start_subfile, check if dirname is
> > > relative, and if comp_dir is available, prepend comp_dir to it.  At
> > > this point, we know dirname is as absolute as it can be.  Then proceed
> > > as in the original unpatched code.  (Watch out for allocation issues.)
> >
> > I don't think we should concat comp_dir and dirname. In my mind,
> > comp_dir makes only sense as an 'independant' information. Or the other
> > way around: dirname gives you information about your source tree
> > structure, and you lose it by prepending comp_dir to it. Have you an
> > objection to storing "dirname/filename" as filename and comp_dir as
> > directory? Maybe this makes only sense when dirname is also relative
> > like in the snippet that Jason posted. Once we agree on this part, I'll
> > post an updated patch.
> 
> We agree that, when dirname is absolute, comp_dir shouldn't get used at all.

OK

> When dirname is relative, I guess we lose information by concatenating
> it with comp_dir, but I don't see where we would ever use that
> information. 

I see 2 uses for that:
 - differentiating identically named source files in different parts of
the source tree. One great example of that is a Linux kernel: try
something like 'find include -name cache.h' at the root. This works
for .c files because they get their relative directory in their
DW_AT_name.
 - If you move your build tree (or in a networked environment, when you
access it from another mount point), then you just have to point 'dir'
at the new root, and all the files should be found. Right now, if you
have more than one include dir, you have to add themm all to the source
search path.

>  I think the filename passed to start_subfile should be
> free of leading directory components; I believe this is necessary to
> make things like "print 'foo.c'::x" work.

I don't think it is necessary, because I have examples of this working
here. Moreover, if you're right on this, then we should add another
patch that sanitizes DW_AT_name before creating symtabs. Like I said in
my first mail, if the compiler accesses your source file with a relative
path, then you'll get a relative path in the comp unit's DW_AT_name. But
I'm quite sure that this would break some setups. 

I just looked up why it isn't an issue to have relative paths in the
symtab's filename. In symtab.c::lookup_symtab, after a search on the
full symtab name we do:

  /* Now, search for a matching tail (only if name doesn't have any dirs) */

  if (lbasename (name) == name)
    ALL_SYMTABS (objfile, s)
    {
      if (FILENAME_CMP (lbasename (s->filename), name) == 0)
	return s;
    }

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Eli Zaretskii]

> From: Frederic RISS <frederic.riss@st.com>
> Cc: GDB Patches <gdb-patches@sourceware.org>
> Date: Fri, 14 Apr 2006 09:32:22 +0200
> 
> On Thu, 2006-04-13 at 10:49 -0700, Jim Blandy wrote:
> > Hi, Frederic.  Thanks for the patch!  I think you're right about how
> > GDB should interpret DW_AT_comp_dir and directories in the line number
> > information.  Some comments:
> 
> Great.
> 
> > You should generally include a ChangeLog entry along with the patch;
> > you'll have to write it anyway, and it helps reviewers get oriented
> > for what they're going to see in the patch itself.
> 
> Will do for the next round.

Thanks.

In addition, I think the current algorithm for searching the sources
is described in the user's manual; please see if there's something in
that description that needs to be updated with your changes.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

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

On Fri, 2006-04-14 at 11:44 +0300, Eli Zaretskii wrote:
> In addition, I think the current algorithm for searching the sources
> is described in the user's manual; please see if there's something in
> that description that needs to be updated with your changes.

I looked at it (7.4 Specifying source directories). The algorithm that
is described is the one in lookup_(partial_)symtab, and I don't think
I'll change that (my patch only modifies the dwarf2 line-info reading). 

While reading that part, I think that I spotted a very minor issue. One
sentence there reads : 
« Note that the executable search path is not used to locate the source
files. Neither is the current working directory, unless it happens to be
in the source path. »

Which isn't false, but I don't think that the user has the ability to
specify an empty source path, it at least always contains $cdir and
$cwd. This means that the current working directory will always be used
to locate source files.
Also there are 2 mentions below that to 'resetting the source path to
empty', but as I said, the source path is never really empty.

If I'm not mistaken and if its not too much nitpicking, shall I commit
something like the attached patch?


[-- Attachment #2: doc.patch --]
[-- Type: text/x-patch, Size: 1403 bytes --]

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

	* gdb.texinfo (Specifying source directories): Update the description
	of the source file search to reflect the fact that the source path
	always contains at least $cdir and $cwd.



--- gdb.texinfo.orig	2006-04-14 13:18:53.000000000 +0200
+++ gdb.texinfo	2006-04-14 13:34:08.000000000 +0200
@@ -5005,8 +5005,7 @@
 that---@file{/mnt/cross/foo.c}.
 
 Note that the executable search path is @emph{not} used to locate the
-source files.  Neither is the current working directory, unless it
-happens to be in the source path.
+source files.
 
 Whenever you reset or rearrange the source path, @value{GDBN} clears out
 any information it has cached about where source files are found and where
@@ -5048,7 +5047,7 @@
 directory at the time you add an entry to the source path.
 
 @item directory
-Reset the source path to empty again.  This requires confirmation.
+Reset the source path to its initial value.  This requires confirmation.
 
 @c RET-repeat for @code{directory} is explicitly disabled, but since
 @c repeating it would be a no-op we do not say that.  (thanks to RMS)
@@ -5064,7 +5063,7 @@
 
 @enumerate
 @item
-Use @code{directory} with no argument to reset the source path to empty.
+Use @code{directory} with no argument to reset the source path to its initial value.
 
 @item
 Use @code{directory} with suitable arguments to reinstall the

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Eli Zaretskii]

> From: Frederic RISS <frederic.riss@st.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 14 Apr 2006 13:43:59 +0200
> 
> Which isn't false, but I don't think that the user has the ability to
> specify an empty source path, it at least always contains $cdir and
> $cwd.

You are right.

> If I'm not mistaken and if its not too much nitpicking, shall I commit
> something like the attached patch?

Yes, after you fix the following few gotchas:

>  @item directory
> -Reset the source path to empty again.  This requires confirmation.
> +Reset the source path to its initial value.  This requires confirmation.

Please say ``default value'' instead of ``initial value'', and please
show that default value.

Also, `dir' with no arguments no longer requires a confirmation, so
please delete that sentence.

> -Use @code{directory} with no argument to reset the source path to empty.
> +Use @code{directory} with no argument to reset the source path to its initial value.

Same here: please say ``default value''.

Thanks.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

On Fri, 2006-04-14 at 16:07 +0300, Eli Zaretskii wrote:
> >  @item directory
> > -Reset the source path to empty again.  This requires confirmation.
> > +Reset the source path to its initial value.  This requires confirmation.
> 
> Please say ``default value'' instead of ``initial value'', and please
> show that default value.

Like in ``Reset the source path to its default value (@samp{$cdir:$cwd}
on Unix systems).''

Sorry if I ask dumb questions, I've never been good at writing docs.

> Also, `dir' with no arguments no longer requires a confirmation, so
> please delete that sentence.

Really? my checkout from yersterday still asks me :
(gdb) dir
Reinitialize source path to empty? (y or n) y

Source directories searched: $cdir:$cwd

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Daniel Jacobowitz]

On Fri, Apr 14, 2006 at 10:22:56AM +0200, Frederic RISS wrote:
> > When dirname is relative, I guess we lose information by concatenating
> > it with comp_dir, but I don't see where we would ever use that
> > information. 
> 
> I see 2 uses for that:
>  - differentiating identically named source files in different parts of
> the source tree. One great example of that is a Linux kernel: try
> something like 'find include -name cache.h' at the root. This works
> for .c files because they get their relative directory in their
> DW_AT_name.
>  - If you move your build tree (or in a networked environment, when you
> access it from another mount point), then you just have to point 'dir'
> at the new root, and all the files should be found. Right now, if you
> have more than one include dir, you have to add themm all to the source
> search path.

Now, these are useful things to do.  But, it's too compiler-dependant,
and build system dependant, to do it this way.  Consider: you are
relying on the fact that line table entries are relative to the
comp_dir, and that many files have the same comp_dir.  Linux 2.6 does
build things this way.  2.4 built from subdirectories.  Coincidentally,
so does GDB.

For an alternative approach, see this:
  http://sourceware.org/ml/gdb/2006-03/msg00189.html

I think that may be a better solution; Paul, are you going to post
that?  Please? :-)

That said, I don't much care which bits get concatenated with which
other bits; as you found, it doesn't seem to make a lot of difference
(and I'm of the opinion that it shouldn't make a difference).  Doing
it your way may be most useful.  But I'd like a solution to the moved
source tree problem which doesn't rely on this compiler behavior.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Eli Zaretskii]

> From: Frederic RISS <frederic.riss@st.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 14 Apr 2006 15:42:17 +0200
> 
> > > +Reset the source path to its initial value.  This requires confirmation.
> > 
> > Please say ``default value'' instead of ``initial value'', and please
> > show that default value.
> 
> Like in ``Reset the source path to its default value (@samp{$cdir:$cwd}
> on Unix systems).''

Yes.

> > Also, `dir' with no arguments no longer requires a confirmation, so
> > please delete that sentence.
> 
> Really? my checkout from yersterday still asks me :
> (gdb) dir
> Reinitialize source path to empty? (y or n) y
> 
> Source directories searched: $cdir:$cwd

Right you are, but this raises an issue with the Windows port (where I
originally checked this): it does NOT ask for permission.

[Time passes...]  Ah, I see, it's probably because
input_from_terminal_p returns zero on Windows.  Is this fixable?

Anyway, I think we should modify the prompt to say this:

  Reinitialize source path to default? (y or n)

Does anyone disagree?

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

On Fri, 2006-04-14 at 10:04 -0400, Daniel Jacobowitz wrote:
> On Fri, Apr 14, 2006 at 10:22:56AM +0200, Frederic RISS wrote:
> > > When dirname is relative, I guess we lose information by concatenating
> > > it with comp_dir, but I don't see where we would ever use that
> > > information. 
> > 
> > I see 2 uses for that:
> >  - differentiating identically named source files in different parts of
> > the source tree. One great example of that is a Linux kernel: try
> > something like 'find include -name cache.h' at the root. This works
> > for .c files because they get their relative directory in their
> > DW_AT_name.
> >  - If you move your build tree (or in a networked environment, when you
> > access it from another mount point), then you just have to point 'dir'
> > at the new root, and all the files should be found. Right now, if you
> > have more than one include dir, you have to add themm all to the source
> > search path.
> 
> Now, these are useful things to do.  But, it's too compiler-dependant,
> and build system dependant, to do it this way.  Consider: you are
> relying on the fact that line table entries are relative to the
> comp_dir, and that many files have the same comp_dir.  Linux 2.6 does
> build things this way.  2.4 built from subdirectories.  Coincidentally,
> so does GDB.

Very true... I thought that doing it this way would solve a more general
problem, but in fact when your project has several compilation
directories it totally fails!

> For an alternative approach, see this:
>   http://sourceware.org/ml/gdb/2006-03/msg00189.html
> 
> I think that may be a better solution; Paul, are you going to post
> that?  Please? :-)

Seems I missed this thread. The idea is excellent.

> That said, I don't much care which bits get concatenated with which
> other bits; as you found, it doesn't seem to make a lot of difference
> (and I'm of the opinion that it shouldn't make a difference).  Doing
> it your way may be most useful.  But I'd like a solution to the moved
> source tree problem which doesn't rely on this compiler behavior.

I don't have clear-cut opinion anymore either. If Jim expresses no other
opinion until then, I'll send a patch implementing his suggestion
beginning of next week.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Daniel Jacobowitz]

On Fri, Apr 14, 2006 at 05:21:41PM +0300, Eli Zaretskii wrote:
> > Really? my checkout from yersterday still asks me :
> > (gdb) dir
> > Reinitialize source path to empty? (y or n) y
> > 
> > Source directories searched: $cdir:$cwd
> 
> Right you are, but this raises an issue with the Windows port (where I
> originally checked this): it does NOT ask for permission.
> 
> [Time passes...]  Ah, I see, it's probably because
> input_from_terminal_p returns zero on Windows.  Is this fixable?

How current is your binary, and what are you running it in?  And what's
the host platform configured as?

If it's a MinGW32-hosted GDB, and you're running it in a DOS console,
and the binary is more than a few months old, than this is an expected
and fixed bug; try a current CVS.  If this is a DJGPP binary running
in a Windows console, then I'm not really sure what to do.  I haven't
tried that.  You'd have to do something similar to what I did for
mingw32.

The problem came in gdb_has_a_terminal.  You have to have serial_fdopen
work on the console, and you have to be able to get the terminal state
from the result.

> Anyway, I think we should modify the prompt to say this:
> 
>   Reinitialize source path to default? (y or n)
> 
> Does anyone disagree?

This sounds good to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Eli Zaretskii]

> Date: Fri, 14 Apr 2006 10:58:02 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Frederic RISS <frederic.riss@st.com>, gdb-patches@sourceware.org
> 
> > [Time passes...]  Ah, I see, it's probably because
> > input_from_terminal_p returns zero on Windows.  Is this fixable?
> 
> How current is your binary, and what are you running it in?  And what's
> the host platform configured as?

It's a MinGW build, and it's from November 2005.

> If it's a MinGW32-hosted GDB, and you're running it in a DOS console,
> and the binary is more than a few months old, than this is an expected
> and fixed bug; try a current CVS.

Great, thanks.

> > Anyway, I think we should modify the prompt to say this:
> > 
> >   Reinitialize source path to default? (y or n)
> > 
> > Does anyone disagree?
> 
> This sounds good to me.

Thanks for the feedback.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frédéric Riss]

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

Le vendredi 14 avril 2006 à 17:21 +0300, Eli Zaretskii a écrit :
> > From: Frederic RISS <frederic.riss@st.com>
> > Cc: gdb-patches@sourceware.org
> > Date: Fri, 14 Apr 2006 15:42:17 +0200
> > 
> > > > +Reset the source path to its initial value.  This requires confirmation.
> > > 
> > > Please say ``default value'' instead of ``initial value'', and please
> > > show that default value.
> > 
> > Like in ``Reset the source path to its default value (@samp{$cdir:$cwd}
> > on Unix systems).''
> 
> Yes.

I checked in the attached patch.


[-- Attachment #2: doc.patch --]
[-- Type: text/x-patch, Size: 1527 bytes --]

Index: gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.321
retrieving revision 1.322
diff -u -p -r1.321 -r1.322
--- gdb.texinfo	31 Mar 2006 19:47:11 -0000	1.321
+++ gdb.texinfo	14 Apr 2006 18:31:32 -0000	1.322
@@ -5005,8 +5005,7 @@ is recorded as @file{../lib/foo.c}, @val
 that---@file{/mnt/cross/foo.c}.
 
 Note that the executable search path is @emph{not} used to locate the
-source files.  Neither is the current working directory, unless it
-happens to be in the source path.
+source files.
 
 Whenever you reset or rearrange the source path, @value{GDBN} clears out
 any information it has cached about where source files are found and where
@@ -5048,7 +5047,7 @@ session, while the latter is immediately
 directory at the time you add an entry to the source path.
 
 @item directory
-Reset the source path to empty again.  This requires confirmation.
+Reset the source path to its default value (@samp{$cdir:$cwd} on Unix systems).  This requires confirmation.
 
 @c RET-repeat for @code{directory} is explicitly disabled, but since
 @c repeating it would be a no-op we do not say that.  (thanks to RMS)
@@ -5064,7 +5063,7 @@ versions of source.  You can correct the
 
 @enumerate
 @item
-Use @code{directory} with no argument to reset the source path to empty.
+Use @code{directory} with no argument to reset the source path to its default value.
 
 @item
 Use @code{directory} with suitable arguments to reinstall the

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

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

Hi Jim,

Attached a patch implementing your suggestion. Some notes:
I first tried to put the concatenation of comp_dir and dirname at the
top of dwarf2_start_subfile (like suggested), but this caused failures
in the testsuite: changing dirname at the beginning of the function
changes the semantics of the loop that tries to match dirname'/'filename
against existing subfiles. The subfile for the current compilation unit
wasn't matched correctly (DW_AT_name = $srcdir/file.c, lineinfo.dirname
= $srcdir, lineinfo.filename = file.c  where $srcdir is a relative
path). Putting the dirname ``absolutification'' after the loop solves
it, but it looks a bit more hackish.

OK to commit?

All this file matching seems quite fragile, and the current approach
will get it wrong sometimes. Shouldn't the loop in dwarf2_start_subfile
be killed in favor of a search using both xfullpath(dirname'/'filename)
at the start of buildsym.c::start_subfile?

[-- Attachment #2: dwarf2-keep-compdir.patch --]
[-- Type: text/x-patch, Size: 3534 bytes --]

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

	* dwarf2read.c (dwarf2_start_subfile): Change prototype to accept
	compilation directory as last argument. Prepend the passed comp_dir 
	to dirname when necessary.
	(dwarf_decode_lines): Pass the compilation directory to 
	dwarf2_start_subfile.


--- dwarf2read.c.orig	2006-04-13 11:17:40.000000000 +0200
+++ dwarf2read.c	2006-04-18 13:53:13.000000000 +0200
@@ -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.
 
@@ -6736,7 +6735,7 @@ dwarf_decode_lines (struct line_header *
    subfile, so that `break /srcdir/list0.c:1' works as expected.  */
 
 static void
-dwarf2_start_subfile (char *filename, char *dirname)
+dwarf2_start_subfile (char *filename, char *dirname, char *comp_dir)
 {
   /* If the filename isn't absolute, try to match an existing subfile
      with the full pathname.  */
@@ -6757,6 +6756,19 @@ dwarf2_start_subfile (char *filename, ch
 	}
       xfree (fullname);
     }
+
+  /* Make the dirname as absolute as possible. */
+
+  if (dirname == NULL)
+    dirname = comp_dir;
+  else
+    if (!IS_ABSOLUTE_PATH (dirname) && comp_dir != NULL)
+      {
+	dirname = concat (comp_dir, SLASH_STRING, dirname, (char *)NULL);
+	/* do_cleanups is called after line info procesing */
+	make_cleanup (xfree, dirname);
+      }
+
   start_subfile (filename, dirname);
 }
 

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Daniel Jacobowitz]

On Tue, Apr 18, 2006 at 02:32:09PM +0200, Frederic RISS wrote:
> All this file matching seems quite fragile, and the current approach
> will get it wrong sometimes. Shouldn't the loop in dwarf2_start_subfile
> be killed in favor of a search using both xfullpath(dirname'/'filename)
> at the start of buildsym.c::start_subfile?

The problem with using xfullpath is that it requires the file to exist
and already be found; as long as we are consistent, it's not too
fragile, because we're checking against our own output.

> @@ -6757,6 +6756,19 @@ dwarf2_start_subfile (char *filename, ch
>  	}
>        xfree (fullname);
>      }
> +
> +  /* Make the dirname as absolute as possible. */

Two spaces after periods please.

> +
> +  if (dirname == NULL)
> +    dirname = comp_dir;
> +  else
> +    if (!IS_ABSOLUTE_PATH (dirname) && comp_dir != NULL)
> +      {
> +	dirname = concat (comp_dir, SLASH_STRING, dirname, (char *)NULL);
> +	/* do_cleanups is called after line info procesing */
> +	make_cleanup (xfree, dirname);

Comment formatting again (and "processing").  But this is really nasty. 
Why not free it before we return?  We don't return dirname, and
start_subfile takes a copy.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

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

On Tue, 2006-04-18 at 09:04 -0400, Daniel Jacobowitz wrote:
> On Tue, Apr 18, 2006 at 02:32:09PM +0200, Frederic RISS wrote:
> > All this file matching seems quite fragile, and the current approach
> > will get it wrong sometimes. Shouldn't the loop in dwarf2_start_subfile
> > be killed in favor of a search using both xfullpath(dirname'/'filename)
> > at the start of buildsym.c::start_subfile?
> 
> The problem with using xfullpath is that it requires the file to exist

I realized that after having sent the patch.

> and already be found; as long as we are consistent, it's not too
> fragile, because we're checking against our own output.

I just made that comment because of the testsuite breakage that ensued
from modifying dirname before the loop... which seemed to imply that the
mechanism was fragile.

> > +
> > +  if (dirname == NULL)
> > +    dirname = comp_dir;
> > +  else
> > +    if (!IS_ABSOLUTE_PATH (dirname) && comp_dir != NULL)
> > +      {
> > +	dirname = concat (comp_dir, SLASH_STRING, dirname, (char *)NULL);
> > +	/* do_cleanups is called after line info procesing */
> > +	make_cleanup (xfree, dirname);
> 
> Comment formatting again (and "processing").  But this is really nasty. 
> Why not free it before we return?  We don't return dirname, and
> start_subfile takes a copy.

I kind of copied it from the end of decode_dwarf_lines where
make_cleanups are used for the same kind of things.
Thanks for your review, I'm attaching an updated patch.

Fred.

[-- Attachment #2: dwarf2-keep-compdir.patch --]
[-- Type: text/x-patch, Size: 3558 bytes --]

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

	* dwarf2read.c (dwarf2_start_subfile): Change prototype to accept
	compilation directory as last argument. Prepend the passed comp_dir 
	to dirname when necessary.
	(dwarf_decode_lines): Pass the compilation directory to 
	dwarf2_start_subfile.

 
--- dwarf2read.c.orig	2006-04-13 11:17:40.000000000 +0200
+++ dwarf2read.c	2006-04-18 15:34:47.000000000 +0200
@@ -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.
 
@@ -6736,8 +6735,10 @@ dwarf_decode_lines (struct line_header *
    subfile, so that `break /srcdir/list0.c:1' works as expected.  */
 
 static void
-dwarf2_start_subfile (char *filename, char *dirname)
+dwarf2_start_subfile (char *filename, char *dirname, char *comp_dir)
 {
+  char *dir = dirname;
+
   /* If the filename isn't absolute, try to match an existing subfile
      with the full pathname.  */
 
@@ -6757,7 +6758,18 @@ dwarf2_start_subfile (char *filename, ch
 	}
       xfree (fullname);
     }
-  start_subfile (filename, dirname);
+
+  /* Make the dirname as absolute as possible.  */
+
+  if (dirname == NULL)
+    dir = comp_dir;
+  else if (!IS_ABSOLUTE_PATH (dirname) && comp_dir != NULL)
+    dir = concat (comp_dir, SLASH_STRING, dirname, (char *)NULL);
+  
+  start_subfile (filename, dir);
+
+  if (dirname != NULL && dir != dirname)
+    xfree(dir);
 }
 
 static void

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Daniel Jacobowitz]

On Tue, Apr 18, 2006 at 03:51:39PM +0200, Frederic RISS wrote:
> +  if (dirname != NULL && dir != dirname)
> +    xfree(dir);

xfree (dir) :-)

I've got no concerns about this patch; Jim was looking at this more
than I was, though, so I hope he'll take a moment to look at this
version.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Jim Blandy]

On 4/18/06, Frederic RISS <frederic.riss@st.com> wrote:
> On Tue, 2006-04-18 at 09:04 -0400, Daniel Jacobowitz wrote:
> > On Tue, Apr 18, 2006 at 02:32:09PM +0200, Frederic RISS wrote:
> > > All this file matching seems quite fragile, and the current approach
> > > will get it wrong sometimes. Shouldn't the loop in dwarf2_start_subfile
> > > be killed in favor of a search using both xfullpath(dirname'/'filename)
> > > at the start of buildsym.c::start_subfile?
> >
> > The problem with using xfullpath is that it requires the file to exist
>
> I realized that after having sent the patch.
>
> > and already be found; as long as we are consistent, it's not too
> > fragile, because we're checking against our own output.
>
> I just made that comment because of the testsuite breakage that ensued
> from modifying dirname before the loop... which seemed to imply that the
> mechanism was fragile.

Let me think aloud a bit.

There are two distinct ways we call start_subfile, which expects a
directory name and a filename, and stores them as two distinct parts. 
First, when processing dies from .debug_info, we call start_symtab,
passing the CU's DW_AT_name as name and DW_AT_comp_dir as the
directory; start_symtab passes these directly through to
start_subfile.  Second, when reading line number info, we call
start_subfile and pass it the filename and the dirname directly from
the line number info.

These two are inconsistent: in the first case, the subfile's dirname
is the value of DW_AT_comp_dir; in the second case, the comp_dir is
never recorded at all (in the current code).  In each case, the
filename is whatever makes up the difference.

However, we get away with it because generally the line info dirname +
filename matches the CU's DW_AT_name, so concatenating them in
dwarf2_start_subfile yields something that matches the 'name' of some
subfile.  We process the dies first, then the line number info, so all
the files the dies mention will already be in the table, so whenever
there's something for the line number info to match, we actually hit
it in the loop.  So we only add subfiles from dwarf2_start_subfile
when the loop doesn't find an existing match (good), but the arguments
we pass are inconsistent with those we would have passed (probably
bad).

So, Frederic, your last patch doesn't introduce any new problems that
I can see, and it does solve the problem you set out to solve
originally (losing comp_dir), but if you're willing go around one more
time, we can try to add the info consistently:
- Leave the comparison loop alone, as in your last patch.
- If dwarf2_start_subfile does have to start a subfile itself, always
pass comp_dir as start_subfile's second argument, whether it's NULL or
not (because this is what we do when calling start_symtab), and
concatenate dirname, if it's non-null, with filename to get
start_subfile's first argument.  I think this means that 'fullname'
always gets used, so you can hoist that computation and its xfree out
of the 'if'.

Either way, this definitely needs a comment.  If you'd like to write
up one yourself, great; if not, that's fine; I'll put one in after
your patch goes in.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

On Thu, 2006-04-20 at 12:27 -0400, Daniel Jacobowitz wrote:
> On Tue, Apr 18, 2006 at 03:51:39PM +0200, Frederic RISS wrote:
> > +  if (dirname != NULL && dir != dirname)
> > +    xfree(dir);
> 
> xfree (dir) :-)

I've heard that patch reviewers have too much time and not enough work,
so I wouldn't dare to send a patch without something to comment :-)

Thanks again for the review.

> I've got no concerns about this patch; Jim was looking at this more
> than I was, though, so I hope he'll take a moment to look at this
> version.
> 

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

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

On Thu, 2006-04-20 at 18:09 -0700, Jim Blandy wrote:
> So, Frederic, your last patch doesn't introduce any new problems that
> I can see, and it does solve the problem you set out to solve
> originally (losing comp_dir), but if you're willing go around one more
> time, we can try to add the info consistently:

OK, here we go.

> - Leave the comparison loop alone, as in your last patch.
> - If dwarf2_start_subfile does have to start a subfile itself, always
> pass comp_dir as start_subfile's second argument, whether it's NULL or
> not (because this is what we do when calling start_symtab), and
> concatenate dirname, if it's non-null, with filename to get
> start_subfile's first argument.  I think this means that 'fullname'
> always gets used, so you can hoist that computation and its xfree out
> of the 'if'.

But then, the loop in dwarf2_start_subfile doesn't serve any purpose,
because the loop at the beginning of start_subfile proper will do the
same work. Or maybe I'm missing something?

> Either way, this definitely needs a comment.  If you'd like to write
> up one yourself, great; if not, that's fine; I'll put one in after
> your patch goes in.

Attached is a new patch that adds comments and removes the superfluous
loop. How's that?


[-- Attachment #2: dwarf2-keep-compdir.patch --]
[-- Type: text/x-patch, Size: 4620 bytes --]

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.
	


--- dwarf2read.c.orig	2006-04-13 11:17:40.000000000 +0200
+++ dwarf2read.c	2006-04-21 10:02:08.000000000 +0200
@@ -846,7 +846,7 @@
 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 @@
 	     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 @@
                    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 @@
 
 /* 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 @@
    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

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Jim Blandy]

On 4/21/06, Frederic RISS <frederic.riss@st.com> wrote:
> > - Leave the comparison loop alone, as in your last patch.
> > - If dwarf2_start_subfile does have to start a subfile itself, always
> > pass comp_dir as start_subfile's second argument, whether it's NULL or
> > not (because this is what we do when calling start_symtab), and
> > concatenate dirname, if it's non-null, with filename to get
> > start_subfile's first argument.  I think this means that 'fullname'
> > always gets used, so you can hoist that computation and its xfree out
> > of the 'if'.
>
> But then, the loop in dwarf2_start_subfile doesn't serve any purpose,
> because the loop at the beginning of start_subfile proper will do the
> same work. Or maybe I'm missing something?

No --- I hadn't noticed the loop in start_subfile.  That's great!

> > Either way, this definitely needs a comment.  If you'd like to write
> > up one yourself, great; if not, that's fine; I'll put one in after
> > your patch goes in.
>
> Attached is a new patch that adds comments and removes the superfluous
> loop. How's that?

If it doesn't cause any regressions, it looks great to me!  Thanks very much!

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Frederic RISS]

On Fri, 2006-04-21 at 09:46 -0700, Jim Blandy wrote:
> On 4/21/06, Frederic RISS <frederic.riss@st.com> wrote:
> > > - Leave the comparison loop alone, as in your last patch.
> > > - If dwarf2_start_subfile does have to start a subfile itself, always
> > > pass comp_dir as start_subfile's second argument, whether it's NULL or
> > > not (because this is what we do when calling start_symtab), and
> > > concatenate dirname, if it's non-null, with filename to get
> > > start_subfile's first argument.  I think this means that 'fullname'
> > > always gets used, so you can hoist that computation and its xfree out
> > > of the 'if'.
> >
> > But then, the loop in dwarf2_start_subfile doesn't serve any purpose,
> > because the loop at the beginning of start_subfile proper will do the
> > same work. Or maybe I'm missing something?
> 
> No --- I hadn't noticed the loop in start_subfile.  That's great!
> 
> > > Either way, this definitely needs a comment.  If you'd like to write
> > > up one yourself, great; if not, that's fine; I'll put one in after
> > > your patch goes in.
> >
> > Attached is a new patch that adds comments and removes the superfluous
> > loop. How's that?
> 
> If it doesn't cause any regressions, it looks great to me!  Thanks very much!

I had regression tested it on i686-linux. I'll check it in in a few
hours time.

Thanks.

Re: [RFC] Don't lose compilation directory in Dwarf2 line-tables

[Andrew STUBBS]

Jim Blandy wrote:
> If it doesn't cause any regressions, it looks great to me!  Thanks very much!

Fred has committed this (and then forgot to say so before going on holiday).

Andrew