[rfc] Use substitute-path for filename portion too

[rfc] Use substitute-path for filename portion too

[Daniel Jacobowitz]

I borrowed this bit from Doug's work on canonicalizing filenames last
month.  We ended up without that patch, but does anyone see a problem
with just this part?

This handles the symtabs created by the DWARF-2 reader for header
files in subdirectories.  For instance, if you have
/opt/codesourcery/foo.c which includes /opt/codesourcery/bar/bar.h,
and the current source line is in bar.h, find_and_open_source
will be called with dirname "/opt/codesourcery" and filename
"/opt/codesourcery/bar/bar.h".  We rewrote /opt/codesourcery
but not /opt/codesourcery/bar/bar.h, and ended up searching a number
of silly places for the filename but not the place it was
actually located.

-- 
Daniel Jacobowitz
CodeSourcery

2008-02-07  Doug Evans  <dje@google.com>

	* source.c (find_and_open_source): Always rewrite absolute filenames.

Index: source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.83
diff -u -p -r1.83 source.c
--- source.c	1 Jan 2008 22:53:13 -0000	1.83
+++ source.c	7 Feb 2008 16:09:34 -0000
@@ -999,10 +999,11 @@ find_and_open_source (struct objfile *ob
 	  strcat (path + len, source_path + len + cdir_len);	/* After $cdir */
 	}
     }
-  else
+
+  if (IS_ABSOLUTE_PATH (filename))
     {
-      /* If dirname is NULL, chances are the path is embedded in
-         the filename.  Try the source path substitution on it.  */
+      /* If filename is absolute path, try the source path
+	 substitution on it.  */
       char *rewritten_filename = rewrite_source_path (filename);
 
       if (rewritten_filename != NULL)

Re: [rfc] Use substitute-path for filename portion too

[Joel Brobecker]

> 2008-02-07  Doug Evans  <dje@google.com>
> 
> 	* source.c (find_and_open_source): Always rewrite absolute filenames.

No problem from my end.

> +
> +  if (IS_ABSOLUTE_PATH (filename))
>      {
> -      /* If dirname is NULL, chances are the path is embedded in
> -         the filename.  Try the source path substitution on it.  */
> +      /* If filename is absolute path, try the source path
> +	 substitution on it.  */

Do we want to restrict this to filenames that are absolute paths?
Imagine the filename was codesourcery/bar/bar.h and the rewrite was
s/codesourcery/adacore/, do we want to attempt the rewrite then?
I suppose we should wait to see a real example of such a situation
before trying to support it...

-- 
Joel

Re: [rfc] Use substitute-path for filename portion too

[Daniel Jacobowitz]

On Thu, Feb 07, 2008 at 09:56:58AM -0800, Joel Brobecker wrote:
> > +
> > +  if (IS_ABSOLUTE_PATH (filename))
> >      {
> > -      /* If dirname is NULL, chances are the path is embedded in
> > -         the filename.  Try the source path substitution on it.  */
> > +      /* If filename is absolute path, try the source path
> > +	 substitution on it.  */
> 
> Do we want to restrict this to filenames that are absolute paths?
> Imagine the filename was codesourcery/bar/bar.h and the rewrite was
> s/codesourcery/adacore/, do we want to attempt the rewrite then?
> I suppose we should wait to see a real example of such a situation
> before trying to support it...

Are such rules supported?  I couldn't work out from the manual
what FROM and TO had to be.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Use substitute-path for filename portion too

[Joel Brobecker]

> > Do we want to restrict this to filenames that are absolute paths?
> > Imagine the filename was codesourcery/bar/bar.h and the rewrite was
> > s/codesourcery/adacore/, do we want to attempt the rewrite then?
> > I suppose we should wait to see a real example of such a situation
> > before trying to support it...
> 
> Are such rules supported?  I couldn't work out from the manual
> what FROM and TO had to be.

Yes, the documentation is not completely clear on this. After re-reading
it, I think it seems to suggest that rewriting is done on the full path,
not partial path names. Thinking about supporting it or not, I couldn't
really come up with an answer. It seems that it could be surprising
either way - which is why waiting for real-case scenarios would probably
make sense here (and apply the patch as you and Doug suggested).


-- 
Joel

Re: Re: [rfc] Use substitute-path for filename portion too

[Aleksandar Ristovski]

Joel Brobecker wrote:
>>> Do we want to restrict this to filenames that are absolute paths?
>>> Imagine the filename was codesourcery/bar/bar.h and the rewrite was
>>> s/codesourcery/adacore/, do we want to attempt the rewrite then?
>>> I suppose we should wait to see a real example of such a situation
>>> before trying to support it...
>> Are such rules supported?  I couldn't work out from the manual
>> what FROM and TO had to be.
> 
> Yes, the documentation is not completely clear on this. After re-reading
> it, I think it seems to suggest that rewriting is done on the full path,
> not partial path names. Thinking about supporting it or not, I couldn't
> really come up with an answer. It seems that it could be surprising
> either way - which is why waiting for real-case scenarios would probably
> make sense here (and apply the patch as you and Doug suggested).
> 
> 
Yes, the patch is good, and it should be on absolute paths only.

Re: [rfc] Use substitute-path for filename portion too

[Eli Zaretskii]

> Date: Thu, 7 Feb 2008 10:27:54 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > > Do we want to restrict this to filenames that are absolute paths?
> > > Imagine the filename was codesourcery/bar/bar.h and the rewrite was
> > > s/codesourcery/adacore/, do we want to attempt the rewrite then?
> > > I suppose we should wait to see a real example of such a situation
> > > before trying to support it...
> > 
> > Are such rules supported?  I couldn't work out from the manual
> > what FROM and TO had to be.
> 
> Yes, the documentation is not completely clear on this.

Patches to make it more clear are welcome.

Re: [rfc] Use substitute-path for filename portion too

[Doug Evans]

On Feb 7, 2008 10:13 AM, Daniel Jacobowitz <drow@false.org> wrote:
> On Thu, Feb 07, 2008 at 09:56:58AM -0800, Joel Brobecker wrote:
> > > +
> > > +  if (IS_ABSOLUTE_PATH (filename))
> > >      {
> > > -      /* If dirname is NULL, chances are the path is embedded in
> > > -         the filename.  Try the source path substitution on it.  */
> > > +      /* If filename is absolute path, try the source path
> > > +    substitution on it.  */
> >
> > Do we want to restrict this to filenames that are absolute paths?
> > Imagine the filename was codesourcery/bar/bar.h and the rewrite was
> > s/codesourcery/adacore/, do we want to attempt the rewrite then?
> > I suppose we should wait to see a real example of such a situation
> > before trying to support it...
>
> Are such rules supported?  I couldn't work out from the manual
> what FROM and TO had to be.

There is one related issue that I'd like to get fixed.  If there are
two files foo.c, and I want to set a breakpoint in one of them, there
is currently no robust way to do that.  If DW_AT_name includes _some_
directory information then it can work, i.e. I can say break
bar/foo.c:3 and it will only find bar/foo.c even if there is
baz/foo.c.  But this only works if DW_AT_name contains the requisite
directory instead of DW_AT_comp_dir containing the entire directory.
This can be achieved by specifying the path to gcc, e.g. gcc -c -g
bar/foo.c -o bar/foo.o instead of (cd bar && gcc -c -g foo.c).

I think what should happen is that if I say "break bar/foo.c:3" then
gdb should do the appropriate searching even if DW_AT_name is only
"foo.c".  If there is a collision gdb should probably either give an
error or give the user a choice - I can't imagine the user wanting to
choose "all" but I suppose that could be an option.

In conjunction with that I'd like to have emacs' "C-x space" set the
breakpoint in the correct file, but I'm not sure how much work that
would involve.  [This would extend to other frontends too.]

Re: [rfc] Use substitute-path for filename portion too

[Daniel Jacobowitz]

On Thu, Feb 07, 2008 at 04:30:30PM -0800, Doug Evans wrote:
> There is one related issue that I'd like to get fixed.  If there are
> two files foo.c, and I want to set a breakpoint in one of them, there
> is currently no robust way to do that.  If DW_AT_name includes _some_
> directory information then it can work, i.e. I can say break
> bar/foo.c:3 and it will only find bar/foo.c even if there is
> baz/foo.c.  But this only works if DW_AT_name contains the requisite
> directory instead of DW_AT_comp_dir containing the entire directory.
> This can be achieved by specifying the path to gcc, e.g. gcc -c -g
> bar/foo.c -o bar/foo.o instead of (cd bar && gcc -c -g foo.c).
> 
> I think what should happen is that if I say "break bar/foo.c:3" then
> gdb should do the appropriate searching even if DW_AT_name is only
> "foo.c".  If there is a collision gdb should probably either give an
> error or give the user a choice - I can't imagine the user wanting to
> choose "all" but I suppose that could be an option.

I definitely agree that we need to improve handling of this case
(though I'm not sure it's related).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Use substitute-path for filename portion too

[Daniel Jacobowitz]

On Thu, Feb 07, 2008 at 12:46:45PM -0500, Daniel Jacobowitz wrote:
> I borrowed this bit from Doug's work on canonicalizing filenames last
> month.  We ended up without that patch, but does anyone see a problem
> with just this part?

As no one did, and most of the participants of the recent discussion
agreed, I've checked this in.

-- 
Daniel Jacobowitz
CodeSourcery