public inbox for gdb@sourceware.org
 help / color / mirror / Atom feed
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-03 17:07 Aleksandar Ristovski
  2008-01-03 17:13 ` Daniel Jacobowitz
  0 siblings, 1 reply; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-03 17:07 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: gdb, Ryan Mansfield



> -----Original Message-----
> From: Daniel Jacobowitz [mailto:drow@false.org]
> Sent: January 3, 2008 11:47 AM
> To: Aleksandar Ristovski
> Cc: gdb@sourceware.org; Ryan Mansfield
> Subject: Re: gdb_realpath: dealing with ./ and ../
> 
> 
> We should not call realpath on files which are not known to exist on
> the system running GDB.  If we do that somewhere, it's a bug.  

I am not sure it is a bug. At the time of gdb_realpath call, gdb can not
know whether the path exists or not, it takes the path information from
debug info. Second, it is perfectly normal situation where sources are not
available at all, and paths mentioned in debug_info do not physically exist
on the system.

>Your
> patch added several calls of that sort.

No, my patch only checked if realpath returns NULL in which case instead of
returning filename as-is, it normalizes it. 

I also added gdb_realpath call in create_subfile to create canonical
pathname for a new subfile, and to canonicalize path we are looking for.

But I do not claim this is a perfect solution.

> 
> > > > When our cross-compiler generates binary, it stores relative path in
> > > > .debug_line section (relative to compilation dir), i.e. '..'.
> > >
> > > What's in .debug_info?  Also, what version of GDB?
> >
> >
> > In my case:
> >      DW_AT_name        :
> > c:/QNXTau/eclipse/ide-4.5-workspace/testManagedCC/main.cc>~~~~~$
> >      DW_AT_comp_dir    :
> > c:/QNXTau/eclipse/ide-4.5-workspace/testManagedCC/Debug>~~~~~
> 
> Well, that's why.  If DW_AT_name was main.cc, things would have
> worked.  That's what GCC generates for me.  You have debug info which
> refers to the same file using two different pathnames.
> 
> Perhaps we can forcibly associate the compilation unit with the first
> entry in the file name table, if they have the same basename and no
> other file in the line table matches the CU better.

Would this not introduce a bit of guessing? Consider a case where there are
several files with the same basename but different paths (possibly specified
using relative path elements, i.e. different pathnames like in my case). In
this case none of the files with the same base name would perfectly fit and
picking the first one will likely not give the correct answer.

> 
> --
> Daniel Jacobowitz
> CodeSourcery

^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-08 19:21 Aleksandar Ristovski
  0 siblings, 0 replies; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-08 19:21 UTC (permalink / raw)
  To: Mark Kettenis; +Cc: brobecker, dje, gdb, Ryan Mansfield

> 
> > If we can confirm for sure that "normalize_path" is safe, I think the
> idea
> > is good (please read my comment about normalize_path here:
> > http://sourceware.org/ml/gdb-patches/2008-01/msg00138.html)
> >
> > Example
> > DW_AT_name=../main.cc
> > DW_AT_comp_dir=/foo/bar/obj
> > The Directory Table:
> >   ..
> > The File Name Table:$
> >   Entry>Dir>~~~~Time>~~~Size>~~~Name$
> >   1>~~~~1>~~~~~~0>~~~~~~0>~~~~~~main.cc$
> >
> > Now we get rid of the comp_dir information (this is what effectively
> > happens):
> >
> > NAME=main.cc
> > DIR=/foo/bar
> >
> > And there is no info about /foo/bar/obj.
> >
> >
> > I would think that this is safe.
> 
> Unfortunately it isn't.  If /foo/bar/obj is a symlink to /bar/foo/obj,
> then ../main.cc actually refers to /bar/foo/main.cc, and not
> /foo/bar/main.cc.  So just "normalizing" names is defenitely unsafe.
> 
> The big question here is, whether a compiler will actually set
> DW_AT_comp_dir to /foo/bar/obj in that case.  One can argue that the
> compilation directory really is /bar/foo/obj in that case and that
> DW_AT_comp_dir should be set to /bar/foo/obj.

Therefore, this concludes this trail of thoughts and this topic should
probably be considered closed.

Any further discussion related to the issue mentioned at the beginning of
the topic should be continued here:
http://sourceware.org/ml/gdb-patches/2008-01/msg00148.html



^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-08 16:12 Aleksandar Ristovski
  2008-01-08 16:40 ` Mark Kettenis
  0 siblings, 1 reply; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-08 16:12 UTC (permalink / raw)
  To: Joel Brobecker, Doug Evans; +Cc: gdb, Ryan Mansfield

> 
> Maybe I shouldn't have talked about complexity as this may be just me
> needing time to understand the purpose of your patch. So I withdraw
> that comment.

Wherever the fix is put, it will be of approx. the same complexity as it
solves the same problem in a similar way.

> 
> > Handling the issue in each debug info reader is an important
> > consideration and may or may not be a problem.  One would need to
> > examine to what extent the issue exists in the other readers, and to
> > what extent start_subfile can solve it and still be debug-format
> > independent without being more complex.
> 
> My suggestion has two ideas behind it:
> 
>     I reallly think that the wrapper around start_subfile that adjusts
>     NAME and DIRNAME so that NAME is always a basename is a good step
>     forward, because it reduces the number of combinations we need to
>     handle during the matching phase later on.  We don't have to handle
>     the case where NAME is a full path, or a relative path of a basename.

This would effectively make keeping DW_AT_comp_dir value redundant. Not sure
if this would create any issues (I don't see any).

> 
>     Second, I still believe at this point that the problem should be
>     handled outside of the debug info reader.  I know that at least
>     stabs should have the same issues as DWARF. It would be very nice
>     to have the problems fixed in both cases without having to duplicate
>     the algorithms we're developing.

> 
> > One could rewrite that patch to keep dwarf2_start_subfile, but one
> > would have to pass an additional parameter so it would know if it's
> > dealing with the main source file.  The patch as submitted just
> > reorganizes things so that other solutions(/heuristics) are possible
> > without major reworking of the code (the patch itself had to do some
> > reworking, but once that's done it's done (in the problem space being
> > discussed)).  Plus it simplifies all call sites to
> > dwarf2_start_subfile/start_subfile.  Previously, each call site had to
> > process fe->dir_index, and there are three of them.
> 
> I think that the reorganization will not be necessary.  I'd like to
> make the subfile layer work in a way that the debug info reader would
> only have to pass the name and dirname as-is, and be confident that
> it'll just work.

If we can confirm for sure that "normalize_path" is safe, I think the idea
is good (please read my comment about normalize_path here:
http://sourceware.org/ml/gdb-patches/2008-01/msg00138.html)

Example
DW_AT_name=../main.cc
DW_AT_comp_dir=/foo/bar/obj
The Directory Table:
  ..
The File Name Table:$
  Entry>Dir>~~~~Time>~~~Size>~~~Name$
  1>~~~~1>~~~~~~0>~~~~~~0>~~~~~~main.cc$

Now we get rid of the comp_dir information (this is what effectively
happens):

NAME=main.cc
DIR=/foo/bar

And there is no info about /foo/bar/obj.


I would think that this is safe.


Thanks,

Aleksandar

^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-04 22:09 Aleksandar Ristovski
  0 siblings, 0 replies; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-04 22:09 UTC (permalink / raw)
  To: Daniel Jacobowitz, Doug Evans; +Cc: gdb, Ryan Mansfield

> 
> On Fri, Jan 04, 2008 at 01:39:51PM -0800, Doug Evans wrote:
> > The use of #line in the testcases in the appended patch is
> > questionable, but as a data point the testcases fail without the patch
> > and pass with the patch.  These versions of the testcases have been
> > amended to use substitute-path.
@Doug:
Without trying-out your patch, by just looking at it, I don't think it
solves testcase number 2 from my previous post:
http://sourceware.org/ml/gdb/2008-01/msg00024.html 
This test case was my initial issue I wanted to solve, and then other issues
come up.

So my original problem was not related to using substitute-path (the
inconsistency of substitute-path should probably be discussed in a separate
thread), but in having two pathnames for the same physical path. One comes
from
DW_AT_name
And is a full file name, e.g. /foo/bar/main.cc
Second is constructed from .debug_line information, by concatenating
directory (which is "..") and file name ("main.cc") giving "../main.cc"
which is then appended to DW_AT_comp_dir resulting in:
/foo/bar/Debug/../main.cc

Now in start_subfile we are comparing:
/foo/bar/main.cc for which subfile structure had been constructed already
and
/foo/bar/Debug/../main.cc
Which should yield to finding the already constructed structure, but since
FILENAME_CMP does fairly simple file comparison, it fails to match it and
then goes on to create new subfile structure which is wrong. 

After that, another lookup is done and the first subfile is found (using
/foo/bar/main.cc) but due to no line information associated with it, gdb
fails to find info about that line and displays the error message to the
user (something like "no line NN in main.cc").


> 
> This happens during symbol reading, right?  That means substitute-path
> has to be set before GDB loads any files; another reason why I don't
> think it is appropriate to use it while building symtabs.
> 
> I'll look at the rest of this thread later.


Part of the solution to the testcase 2 is to add opposite case (we are
checking only IS_ABSOLUTE_PATH (name)... but we do not cover the case where
NAME is relative (and existing subfile->name is absolute, which is exactly
my case).

Maybe the other part of the solution to testcase 2 should really be in
rewriting FILENAME_CMP to try other alternatives if simple comparison fails.
It could remove '..' path elements and it would be safe if we are dealing
with paths coming from the binary.

> 
> --
> Daniel Jacobowitz
> CodeSourcery

^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-04 20:16 Aleksandar Ristovski
  0 siblings, 0 replies; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-04 20:16 UTC (permalink / raw)
  To: gdb

> 
> Attached is a binary that illustrates the problem I talked about
I couldn't send the attached binary to the mailing list... the mail bounced
back. Sorry about that.

--
Aleksandar Ristovski
QNX Software Systems

^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-04 19:52 Aleksandar Ristovski
  2008-01-04 20:30 ` Doug Evans
  0 siblings, 1 reply; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-04 19:52 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: gdb, Ryan Mansfield, Joel Brobecker

> 
> Yes, this is something I've been meaning to fix for ages, but never
> had time.  We need versions of IS_ABSOLUTE_PATH, FILENAME_CMP, and the
> other related macros which handle DOS pathnames regardless of the host
> system.  We should give them different names, though, so that both are
> available (sometimes we need to manipulate host paths).
>

Submitted a patch:
http://sourceware.org/ml/gdb-patches/2008-01/msg00062.html

 
> It would help a lot if you could write test cases.  We can use
> gdb.dwarf2/ to construct arbitrary DWARF-2 contents, with relative or
> absolute paths as needed.  If you have trouble writing the test cases,
> I can do it if you send me a couple of example binaries, and the
> before and after expected results.
 
It is a good idea to have test cases. I, unfortunately, know very little
about writing tests, and would really appreciate if you wrote the tests.

Attached is a binary that illustrates the problem I talked about, but to
summarize and break-down the problem into independent pieces I will list
test cases I can think of. We need combinations of DW_AT_name,
DW_AT_comp_dir, and directory table (in .debug_line section).  I am not 100%
sure how are you going to test this, but my testcase is to set a breakpoint
using file name, i.e.
(gdb) break main.c:11
Or something like that. I expect testcase 2 to fail. Test 1 and 3 should be
o.k. When testing Na) versions of testcases (DOS paths, linux host), I would
expect all of them to fail.

1. Currently covered case:
DW_AT_name = /foo/bar/main.c
No DW_AT_comp_dir or DW_AT_comp_dir equal to DW_AT_name
The Directory Table:
(empty)
The File Name Table:
 Entry	Dir	Time	Size	Name
 1		1	0	0	main.c

2. Case where DW_AT_name is absolute path, DW_AT_comp_dir is given as a
different absolute path and line info is given relative to comp_dir:
 
DW_AT_name = /foo/bar/main.c
DW_AT_comp_dir = /foo/bar/Debug
The Directory Table:
 ..

The File Name Table:
 Entry	Dir	Time	Size	Name
 1		1	0	0	main.c

3. DW_AT_name relative to DW_AT_comp_dir

DW_AT_name = ../main.c
DW_AT_comp_dir = /foo/bar/Debug

The Directory Table:
 ..

The File Name Table:
 Entry	Dir	Time	Size	Name
 1		1	0	0	main.c


========= Cross-compiled on windows case =============
The following testcases are basically the same as above, except now we use
DOS style paths. The tests are meaningful only when testing on POSIX host.
Otherwise they default to the above tests.

1a. Currently covered case:
DW_AT_name = C:/foo/bar/main.c
No DW_AT_comp_dir or DW_AT_comp_dir equal to DW_AT_name
The Directory Table:
(empty)
The File Name Table:
 Entry	Dir	Time	Size	Name
 1		1	0	0	main.c

2a. Case where DW_AT_name is absolute path, DW_AT_comp_dir is given as a
different absolute path and line info is given relative to comp_dir:
 
DW_AT_name = C:/foo/bar/main.c
DW_AT_comp_dir = C:/foo/bar/Debug
The Directory Table:
 ..

The File Name Table:
 Entry	Dir	Time	Size	Name
 1		1	0	0	main.c

3a. DW_AT_name relative to DW_AT_comp_dir

DW_AT_name = ../main.c
DW_AT_comp_dir = C:/foo/bar/Debug

The Directory Table:
 ..

The File Name Table:
 Entry	Dir	Time	Size	Name
 1		1	0	0	main.c



Thank you,

Aleksandar Ristovski
QNX Software Systems

^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-04 17:04 Aleksandar Ristovski
  2008-01-04 17:42 ` Daniel Jacobowitz
  2008-01-04 21:40 ` Doug Evans
  0 siblings, 2 replies; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-04 17:04 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: gdb, Ryan Mansfield

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

> On Thu, Jan 03, 2008 at 01:25:55PM -0500, Aleksandar Ristovski wrote:
> > 1. Problem one - relative NAME and absolute subfile->name case not
> covered:
> 
> I think you're right, but I can't tell for sure.

I am pretty sure this is real problem as this is what I am seeing with our
case (the instrumentation I posted proves it). This is easy and I think
straight forward to fix. (please see the attachement buildsym.c.patch1).

BUT...
This does not solve all my problems. In fact, when debugging on linux a
binary cross-compiled on windows, it won't work:

IS_ABSOLUTE_PATH macro will differ depending on the configured gdb host. The
problem with this is when one is debugging binary cross-compiled on windows,
on a unix host, IS_ABSOLUTE_PATH will return 0 on "C:.." style paths which
is not correct. Looking at the code, in many places we concatenate directory
and file name if IS_ABSOLUTE_PATH returns zero so in case of cross-compiled
binary I suspect there will be many issues.

> 
> > 2. Problem two - one physical file is specified with two pathnames.
> 
> The specific case that's important here is associating the main file
> from a dwarf2 CU DIE with the right lines from .debug_line.  When we
> go to create that subfile we haven't created the other subfiles yet.
> So what we need, I think, is to handle this in dwarf2read.c by walking
> through the filename / directory table.

dwarf2read.c calls start_subfile or start_symtab which in turn calls
start_subfile. I think we should let our readers deal with "compiled" paths
(i.e. as found in the binary), without conversion. In start_subfile,
however, we can do something with paths. Currently, we do not try to
substitute-path on paths coming from the binary, even though they are
clearly our source paths and should be dealt with the same as with any other
source path. 

My attempt would be patch2 attached (start_subfile.patch). Please take a
look. This should solve both problems.

Note: IS_ABSOLUTE_PATH issue will still exist since we deal with compiled
paths throughout gdb code. Wherever IS_ABSOLUTE_PATH is used on a windows
style path (and gdb was configured for *ix) it will return 0 leading to
problems.

Note2: for some reason, my post to gdb-patches didn't go through even after
two attempts. There I submitted only source.c change dealing with rewriting
source path in case when NAME is an absolute path and at the same time
DIRNAME is not NULL (which can happen).
 

Aleksandar Ristovski
QNX Software Systems


[-- Attachment #2: buildsym.c.patch1 --]
[-- Type: application/octet-stream, Size: 1745 bytes --]

Index: gdb/buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.59
diff -u -3 -p -r1.59 buildsym.c
--- gdb/buildsym.c	1 Jan 2008 22:53:09 -0000	1.59
+++ gdb/buildsym.c	4 Jan 2008 14:41:44 -0000
@@ -583,6 +583,13 @@ void
 start_subfile (char *name, char *dirname)
 {
   struct subfile *subfile;
+  char *absname; /* Absolute NAME.  */
+
+  if (!IS_ABSOLUTE_PATH (name) 
+      && dirname != NULL)
+    absname = concat (dirname, SLASH_STRING, name, NULL);
+  else
+    absname = name;
 
   /* See if this subfile is already known as a subfile of the current
      main source file.  */
@@ -590,6 +597,7 @@ start_subfile (char *name, char *dirname
   for (subfile = subfiles; subfile; subfile = subfile->next)
     {
       char *subfile_name;
+      char *aname;
 
       /* If NAME is an absolute path, and this subfile is not, then
 	 attempt to create an absolute path to compare.  */
@@ -601,7 +609,13 @@ start_subfile (char *name, char *dirname
       else
 	subfile_name = subfile->name;
 
-      if (FILENAME_CMP (subfile_name, name) == 0)
+      if (!IS_ABSOLUTE_PATH (name)
+	  && IS_ABSOLUTE_PATH (subfile->name))
+	aname = absname;
+      else
+	aname = name;
+
+      if (FILENAME_CMP (subfile_name, aname) == 0)
 	{
 	  current_subfile = subfile;
 	  if (subfile_name != subfile->name)
@@ -612,6 +626,9 @@ start_subfile (char *name, char *dirname
 	xfree (subfile_name);
     }
 
+  if (absname != name)
+    xfree (absname);
+
   /* This subfile is not known.  Add an entry for it. Make an entry
      for this subfile in the list of all subfiles of the current main
      source file.  */

[-- Attachment #3: start_subfile.patch --]
[-- Type: application/octet-stream, Size: 5970 bytes --]

Index: gdb/buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.59
diff -u -3 -p -r1.59 buildsym.c
--- gdb/buildsym.c	1 Jan 2008 22:53:09 -0000	1.59
+++ gdb/buildsym.c	4 Jan 2008 16:28:40 -0000
@@ -44,6 +44,7 @@
 #include "cp-support.h"
 #include "dictionary.h"
 #include "addrmap.h"
+#include "source.h"
 
 /* Ask buildsym.h to define the vars it normally declares `extern'.  */
 #define	EXTERN
@@ -574,15 +575,57 @@ make_blockvector (struct objfile *objfil
   return (blockvector);
 }
 \f
+
+/* Workaround for IS_ABSOLUTE_PATH issue. When gdb is configured for
+   *ix, and we are debugging binary cross-compiled on windows, the original
+   IS_ABSOLUTE_PATH will always return 0. This is not good.  */
+#undef IS_ABSOLUTE_PATH
+#undef IS_DIR_SEPARATOR
+
+#define IS_DIR_SEPARATOR(c)	((c) == '/' || (c) == '\\')
+/* Note that IS_ABSOLUTE_PATH accepts d:foo as well, although it is
+   only semi-absolute.  This is because the users of IS_ABSOLUTE_PATH
+   want to know whether to prepend the current working directory to
+   a file name, which should not be done with a name like d:foo.  */
+#define IS_ABSOLUTE_PATH(f)	(IS_DIR_SEPARATOR((f)[0]) || (((f)[0]) && ((f)[1] == ':')))
+
 /* 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 the file was compiled (or NULL if not known).  */
 
 void
-start_subfile (char *name, char *dirname)
+start_subfile (char *name, char *dirname) 
 {
   struct subfile *subfile;
+  char *absname; /* Rewritten absolute NAME.  */
+
+  if (IS_ABSOLUTE_PATH (name))
+    {
+      char *rwname;
+      rwname = rewrite_source_path (name);
+      if (rwname != NULL)
+	{
+	  make_cleanup (xfree, rwname);
+	  name = rwname;
+	}
+      absname = name;
+    }
+  else
+    {
+      if (dirname != NULL)
+	{
+	  char *rwdirname = rewrite_source_path (dirname);
+	  if (rwdirname == NULL)
+	    rwdirname = dirname;
+	  absname = concat (rwdirname, SLASH_STRING, name, NULL);
+	  if (rwdirname != dirname)
+	    xfree (rwdirname);
+	  make_cleanup (xfree, absname);
+	}
+      else
+	absname = name;
+    }
 
   /* See if this subfile is already known as a subfile of the current
      main source file.  */
@@ -590,18 +633,36 @@ start_subfile (char *name, char *dirname
   for (subfile = subfiles; subfile; subfile = subfile->next)
     {
       char *subfile_name;
+      char *aname;
 
       /* 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,
+	{
+	  char *rwsubfile_dirname = rewrite_source_path (subfile->dirname);
+	  if (rwsubfile_dirname == NULL)
+	    rwsubfile_dirname = subfile->dirname;
+	  subfile_name = concat (rwsubfile_dirname, SLASH_STRING,
 			       subfile->name, NULL);
+	  if (rwsubfile_dirname != subfile->dirname)
+	    xfree (rwsubfile_dirname);
+	}
+      else
+	{
+	  subfile_name = rewrite_source_path (subfile->name);
+	  if (subfile_name == NULL)
+	    subfile_name = subfile->name;
+	}
+
+      if (!IS_ABSOLUTE_PATH (name)
+	  && IS_ABSOLUTE_PATH (subfile->name))
+	aname = absname;
       else
-	subfile_name = subfile->name;
+	aname = name;
 
-      if (FILENAME_CMP (subfile_name, name) == 0)
+      if (FILENAME_CMP (subfile_name, aname) == 0)
 	{
 	  current_subfile = subfile;
 	  if (subfile_name != subfile->name)
Index: gdb/source.c
===================================================================
RCS file: /cvs/src/src/gdb/source.c,v
retrieving revision 1.83
diff -u -3 -p -r1.83 source.c
--- gdb/source.c	1 Jan 2008 22:53:13 -0000	1.83
+++ gdb/source.c	4 Jan 2008 16:28:40 -0000
@@ -895,10 +895,12 @@ get_substitute_path_rule (const char *pa
    Return NULL if no substitution rule was specified by the user,
    or if no rule applied to the given PATH.  */
    
-static char *
+char *
 rewrite_source_path (const char *path)
 {
-  const struct substitute_path_rule *rule = get_substitute_path_rule (path);
+  const struct substitute_path_rule *rule = (path != NULL) ? 
+					get_substitute_path_rule (path):
+					NULL;
   char *new_path;
   int from_len;
   
@@ -999,10 +1001,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)
Index: gdb/source.h
===================================================================
RCS file: /cvs/src/src/gdb/source.h,v
retrieving revision 1.9
diff -u -3 -p -r1.9 source.h
--- gdb/source.h	1 Jan 2008 22:53:13 -0000	1.9
+++ gdb/source.h	4 Jan 2008 16:28:40 -0000
@@ -66,4 +66,14 @@ extern struct symtab_and_line set_curren
 
 /* Reset any information stored about a default file and line to print. */
 extern void clear_current_source_symtab_and_line (void);
+
+/* If the user specified a source path substitution rule that applies
+   to PATH, then apply it and return the new path.  This new path must
+   be deallocated afterwards.  
+   
+   Return NULL if no substitution rule was specified by the user,
+   or if no rule applied to the given PATH.  */
+   
+extern char *rewrite_source_path (const char *path);
+
 #endif

^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-03 18:30 Aleksandar Ristovski
  2008-01-04 12:52 ` Daniel Jacobowitz
  0 siblings, 1 reply; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-03 18:30 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: gdb, Ryan Mansfield

> An alternative would be to do some canonicalization - not
> gdb_realpath, which accesses the filesystem, but just string
> manipulation - on the subfile names iff nothing matches the main
> file.  You could remove the ".." there.

Allright, I will try with the alternative. 

I see two problems:

1. Problem one - relative NAME and absolute subfile->name case not covered:

In function start_subfile (buildsym.c:approx 554) we completely miss one
case: when NAME is relative path but subfile->name is absolute.

I put some instrumentation to illustrate this problem. The list below says
what is happening and specifies name, directory:

start_subfile: c:/testManagedCC/main.cc, c:/testManagedCC/Debug
creating subfile: c:/testManagedCC/main.cc, c:/testManagedCC/Debug
start_subfile: ../main.cc, c:/testManagedCC/Debug
comparing: c:/testManagedCC/main.cc, ../main.cc
creating subfile: ../main.cc, c:/testManagedCC/Debug
start_subfile: ../main.cc, c:/testManagedCC/Debug
comparing: ../main.cc, ../main.cc

Then it goes on to compare ../main.cc for each line in linetable.

Note that we created two subfiles for physically one file.


2. Problem two - one physical file is specified with two pathnames.

This is the case I was talking about. To fix it, I would try a combination
of guessing/normalizing, by inserting code for "second-best" match using
normalized path (in the same loop) if FILENAME_CMP fails to match and then
if no perfect match is found, use the second-best match. Not sure what
happens if there are more than one second-best matches (you say it's going
to be very rare)... probably at least issue a warning and behave as if no
match was found? Or resort to getting the first/last second-best match? On a
second thought, if we go with the second-best match we will never get into a
situation where two created subfile-s give more two second-best matches...
it will always be either perfect match or the second-best... this should
work.

Comments?



^ permalink raw reply	[flat|nested] 25+ messages in thread
* RE: gdb_realpath: dealing with ./ and ../
@ 2008-01-03 16:39 Aleksandar Ristovski
  2008-01-03 16:52 ` Daniel Jacobowitz
  0 siblings, 1 reply; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-03 16:39 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: gdb, Ryan Mansfield


> -----Original Message-----
> From: Daniel Jacobowitz [mailto:drow@false.org]
> Sent: January 3, 2008 10:56 AM
> To: Aleksandar Ristovski
> Cc: gdb@sourceware.org; Ryan Mansfield
> Subject: Re: gdb_realpath: dealing with ./ and ../
> 
> On Thu, Jan 03, 2008 at 10:20:19AM -0500, Aleksandar Ristovski wrote:
> > Hello,
> >
> > First a question, to give an idea what I am talking about and then
> detailed
> > explanation.
> >
> > Question: Should gdb_realpath deal with './' and '../' path elements and
> > compact them along with 'canonicalization' it already does?
> 
> The problem with this idea is that removing ../ is not reliably
> correct.  On Unix, symlinks allow foo/../bar and bar to be different

On linux, when realpath works all is fine and it will take care of the
symbolic links. However, the problem starts when paths do not really exist
and realpath fails. A simple example is my cross-compiled binary built on
windows, being debugged on linux. In this case, when realpath fails, I would
like to 'compact' or 'normalize' the path by resolving relative path
elements (and current path elements) thus forming canonical path, whithout
resolving symlinks (which can not be resolved since they do not exist).

Additional problem is on windows, where realpath doesn't work and
gdb_realpath usually simply returns the input parameter. (my proposed patch
doesn't solve this... it is not complete yet).

> directories.  We should only canonicalize paths to local files, not to
> files mentioned in debug information.
> 
> gdb_realpath shouldn't need any changes to handle ..; it works from
> the local filesystem and constructs a real canonical path.  I see that
> you're on Windows.  gdb_realpath may not handle Windows correctly;
> libiberty's lrealpath does and I don't know why we still have both.

I don't think lrealpath would work in case the real path doesn't exist.

> 
> > When our cross-compiler generates binary, it stores relative path in
> > .debug_line section (relative to compilation dir), i.e. '..'.
> 
> What's in .debug_info?  Also, what version of GDB?


In my case:
     DW_AT_name        :
c:/QNXTau/eclipse/ide-4.5-workspace/testManagedCC/main.cc>~~~~~$
     DW_AT_comp_dir    :
c:/QNXTau/eclipse/ide-4.5-workspace/testManagedCC/Debug>~~~~~

GDB version 6.7.

> 
> I have:
> 
>  <0><154>: Abbrev Number: 1 (DW_TAG_compile_unit)
>   <189>     DW_AT_name        : ../main.c
>   <193>     DW_AT_comp_dir    : /home/drow/z/baz
> 
>  The Directory Table:
>   ..
> 
>  The File Name Table:
>   Entry Dir     Time    Size    Name
>   1     1       0       0       main.c
> 
> So everything constructs the same /home/drow/z/baz/../main.c.
> 
> --
> Daniel Jacobowitz
> CodeSourcery

^ permalink raw reply	[flat|nested] 25+ messages in thread
* gdb_realpath: dealing with ./ and ../
@ 2008-01-03 15:25 Aleksandar Ristovski
  2008-01-03 16:00 ` Daniel Jacobowitz
  0 siblings, 1 reply; 25+ messages in thread
From: Aleksandar Ristovski @ 2008-01-03 15:25 UTC (permalink / raw)
  To: gdb; +Cc: Ryan Mansfield

Hello,

First a question, to give an idea what I am talking about and then detailed
explanation.

Question: Should gdb_realpath deal with './' and '../' path elements and
compact them along with 'canonicalization' it already does?

Details:
Our binary was created from one compilation unit:
C:/foo/bar/main.cc
Our compilation directory is:
C:/foo/bar/Debug

When our cross-compiler generates binary, it stores relative path in
.debug_line section (relative to compilation dir), i.e. '..'.

readelf -wl output gives this:
...

  Opcode 6 has 0 args
  Opcode 7 has 0 args
  Opcode 8 has 0 args
  Opcode 9 has 1 args

 The Directory Table:
  ..

 The File Name Table:
  Entry	Dir	Time	Size	Name
  1	1	0	0	main.cc

...

GDB internally gets confused by this: it first creates subfile using
filename:  'C:/foo/bar/main.cc'
But then, when breakpoint is set:
(gdb) b main.cc:11
It loads line table, finds '..', constructs absolute path using compilation
directory and creates "C:/foo/bar/Debug/../main.cc" and then compares this
(using FILENAME_CMP macro) to existing subfile-s and fails to find it (file
buildsym.c, function start_subfile).

It looks like storing relative path in .debug_line is correct (is it?) but
gdb_realpath fails to compact paths containing '..' path elements. 


Question: Should gdb_realpath deal with './' and '../' path elements and
compact them along with 'canonicalization' it already does? Alternatively,
should FILENAME_CMP do more to be smarter about comparing two paths?

Thank you,

Aleksandar Ristovski
QNX Software Systems


Patch that illustrates a solution, providing gdb_realpath indeed needs to
deal with relative path elements.

Index: gdb/utils.c
===================================================================
--- gdb/utils.c	(revision 69)
+++ gdb/utils.c	(working copy)
@@ -2867,6 +2867,109 @@
   return addr;
 }
 
+
+/* Normalize_path does lightweight path clean-up. It removes './' 
+ elements and resolves '../' elements by removing previous entry if any.
+ If FILENAME starts with '../', then '../' does not get removed.  
+
+ Returned value should be freed with xfree.
+
+ Examples:
+ ../main.c   -->    ../main.c
+ ./main.c    -->    main.c
+ /main.c     -->    /main.c
+ /foo/./bar/././main.c  -->   /foo/bar/main.c
+ C:/Temp/Debug/../main.c  -->  C:/Temp/main.c
+  */
+
+char *
+normalize_path (const char *filename)
+{
+  char *p;
+  char *pi;
+  int len;
+# if defined (PATH_MAX)
+  char buf[PATH_MAX];
+#  define USE_REALPATH
+# elif defined (MAXPATHLEN)
+  char buf[MAXPATHLEN];
+# endif
+
+ gdb_assert (filename != NULL);
+
+  strncpy (buf, filename, sizeof (buf));
+  buf[sizeof (buf)] = '\0';
+
+  p = buf;
+
+  while ((pi = strstr (p, "./")))
+    {
+      if (pi == p) /* FILENAME starts with './'. Remove it.  */
+	  p += 2;
+      else
+	break;
+    }
+
+  if (p != buf)
+      strncpy (buf, filename + (p - buf), sizeof (buf));
+ 
+  len = strlen (buf);
+
+  /* Remove all double '//' except the leading occurence.  */
+  p = buf + 1;
+  while (p < buf + len)
+    {
+      if (p[0] == '/' && p[1] == '/')
+	{
+	  memmove (p, p+1, buf + len - p);
+	  len--;
+	}
+      p++;
+    }
+
+  /* Replace all other occurences of '/./' with '/'.  */
+  p = buf;
+  while ((pi = strstr (p, "/./")))
+    {
+      p = pi + 3;
+      memmove (pi, p, buf + len  - p);
+      len -= 3;
+      memset (buf + len, 0, 3);
+    }
+
+  /* Remove trailing '/.'.  */
+  while (buf[len-2] == '/' && buf[len-1] == '.')
+    {
+      len -= 2;
+      buf[len] = '\0';
+      buf[len+1] = '\0';
+    }
+
+  /* Deal with '../' sequences.  */
+  p = buf + 1; /* In an odd case that path begins with '/../' we don't want
+		to know.  */
+  while ((pi = strstr (p, "/..")))
+    {
+      p = pi;
+      /* Reverse find '/'.  */
+      pi = p - 1;
+      while (pi > buf && *pi != '/')
+	pi--;
+
+      if (pi != p)
+	{
+	  p += 3;
+	  memmove (pi, p, buf + len - p);
+	  len -= (p - pi);
+	  memset (buf + len, 0, p - pi);
+	}
+      else
+	p++;
+    }
+
+  return xstrdup (buf);
+}
+
 char *
 gdb_realpath (const char *filename)
 {
@@ -2886,7 +2989,9 @@
 # if defined (USE_REALPATH)
     const char *rp = realpath (filename, buf);
     if (rp == NULL)
-      rp = filename;
+      {
+	return normalize_path (filename);
+      }
     return xstrdup (rp);
 # endif
   }
Index: gdb/buildsym.c
===================================================================
--- gdb/buildsym.c	(revision 69)
+++ gdb/buildsym.c	(working copy)
@@ -548,26 +548,46 @@
   for (subfile = subfiles; subfile; subfile = subfile->next)
     {
       char *subfile_name;
+      char *full_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,
+	{
+	  char *path = concat (subfile->dirname, SLASH_STRING,
 			       subfile->name, NULL);
+	  subfile_name = gdb_realpath (path);
+	  xfree (path);
+	}
       else
 	subfile_name = subfile->name;
 
-      if (FILENAME_CMP (subfile_name, name) == 0)
+      /* If NAME is not an absolute path, try to make it an 
+	 absolute path so we compare apples with apples.  */
+      if (!IS_ABSOLUTE_PATH (name) && dirname != NULL)
+	{
+	  char *path = concat (dirname, SLASH_STRING, name, NULL);
+	  full_name = gdb_realpath (path);
+	  xfree (path);
+	}
+      else
+	full_name = name;
+
+      if (FILENAME_CMP (subfile_name, full_name) == 0)
 	{
 	  current_subfile = subfile;
 	  if (subfile_name != subfile->name)
 	    xfree (subfile_name);
+	  if (full_name != name)
+	    xfree (full_name);
 	  return;
 	}
       if (subfile_name != subfile->name)
 	xfree (subfile_name);
+      if (full_name != name)
+	xfree (full_name);
     }
 
   /* This subfile is not known.  Add an entry for it. Make an entry

^ permalink raw reply	[flat|nested] 25+ messages in thread
end of thread, other threads:[~2008-01-08 19:54 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-03 17:07 gdb_realpath: dealing with ./ and ../ Aleksandar Ristovski
2008-01-03 17:13 ` Daniel Jacobowitz
2008-01-07 14:33   ` Joel Brobecker
2008-01-07 17:00     ` Doug Evans
2008-01-08  5:46       ` Joel Brobecker
2008-01-08 19:54         ` Doug Evans
  -- strict thread matches above, loose matches on Subject: below --
2008-01-08 19:21 Aleksandar Ristovski
2008-01-08 16:12 Aleksandar Ristovski
2008-01-08 16:40 ` Mark Kettenis
2008-01-04 22:09 Aleksandar Ristovski
2008-01-04 20:16 Aleksandar Ristovski
2008-01-04 19:52 Aleksandar Ristovski
2008-01-04 20:30 ` Doug Evans
2008-01-04 17:04 Aleksandar Ristovski
2008-01-04 17:42 ` Daniel Jacobowitz
2008-01-04 18:25   ` Joel Brobecker
2008-01-04 21:40 ` Doug Evans
2008-01-04 21:48   ` Daniel Jacobowitz
2008-01-04 22:23     ` Doug Evans
2008-01-03 18:30 Aleksandar Ristovski
2008-01-04 12:52 ` Daniel Jacobowitz
2008-01-03 16:39 Aleksandar Ristovski
2008-01-03 16:52 ` Daniel Jacobowitz
2008-01-03 15:25 Aleksandar Ristovski
2008-01-03 16:00 ` Daniel Jacobowitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).