proposal: substitute-path handles foreign dir separators

proposal: substitute-path handles foreign dir separators

[Raphael Zulliger]

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

Hi

This is my first patch sent to this list. Please let me know if I'm 
doing something wrong.

This is a not a bug fix, it's a feature: This patch extends 
'substitute-path' to handle paths containing 'foreign' directory 
separators. Means e.g.: A powerpc-eabi-gdb running on Linux can handle 
powerpc-eabi-binaries built on Windows and vice versa.

Several points need to be considered:
  1. Most importantly: Do we want such a 'feature'? Personally, I think 
it makes sense to have such a feature. A prominent use case may occur 
when developing for an embedded system. Having platform independent IDEs 
like Eclipse together with the GNU toolchain (cygwin, mingw, *nix) makes 
it possible that some members of a development team work on e.g. 
Windows, while the rest works on e.g. Linux. "Working" in this case 
could mean that people compile their code and probably create libraries 
on their (preferred) OS. Later these libraries will be linked together 
to a binary which will end up on an embedded target that needs to be 
debugged. (Sure you can argue that this wouldn't happen if they'd use a 
buildmachine, but...). Debugging, again, may be done by a person using 
Linux or Windows. That's where this patch would become useful.

To continue the discussion lets assume that we all want this feature.
  2. I created this patch because I actually debug binaries on a Linux 
box which have been created on a Windows machine (GNU toolchain/Cygwin). 
Empirically, I've realized that 'substitute-path' doesn't work for this 
purpose. After having single-stepped GDB and modified its code slightly, 
I ended up with this patch. The point is that I figured it out 
empirically. It could the case that I justed missed an already existing 
solution. Please let me know if my patch is crap because there exists 
another way of handling my use case.

  3. I didn't write any tests yet. I would write them if the patch would 
get accepted. If this would be the case, it would be nice if you could 
give me some hints where to start (which file, what kind of test, ...)

  4. I didn't verify/test my code on any other system than Linux. I 
could test it on Windows (mingw) if you would accept the patch

  5. I didn't yet fill out the 'copyright assignment' form (mentioned in 
the CONTRIBUTE file). I would do so if required.


Raphael



[-- Attachment #2: substitute-path.diff --]
[-- Type: text/plain, Size: 2267 bytes --]

*** /tmp/gdb-7.2/gdb/source.c	2010-12-17 14:12:11.618336763 +0100
--- gdb-7.2/gdb/source.c	2010-12-17 14:13:02.347583097 +0100
*************** substitute_path_rule_matches (const stru
*** 882,888 ****
    /* Make sure that the region in the path that matches the substitution
       rule is immediately followed by a directory separator (or the end of
       string character).  */
!   if (path[from_len] != '\0' && !IS_DIR_SEPARATOR (path[from_len]))
      return 0;
  
    return 1;
--- 882,888 ----
    /* Make sure that the region in the path that matches the substitution
       rule is immediately followed by a directory separator (or the end of
       string character).  */
!   if (path[from_len] != '\0' && (!IS_UNIX_DIR_SEPARATOR (path[from_len]) && !IS_DOS_DIR_SEPARATOR (path[from_len])))
      return 0;
  
    return 1;
*************** find_and_open_source (const char *filena
*** 1015,1021 ****
  	}
      }
  
!   if (IS_ABSOLUTE_PATH (filename))
      {
        /* If filename is absolute path, try the source path
  	 substitution on it.  */
--- 1015,1021 ----
  	}
      }
  
!   if (IS_UNIX_ABSOLUTE_PATH (filename) || IS_DOS_ABSOLUTE_PATH(filename))
      {
        /* If filename is absolute path, try the source path
  	 substitution on it.  */
*************** find_and_open_source (const char *filena
*** 1023,1028 ****
--- 1023,1046 ----
  
        if (rewritten_filename != NULL)
          {
+     	  /* Especially for embedded systems, it may be the case that a
+     	 binary has been built on Windows but the embedded system is now
+     	 being debugged on a Unix machine (and vice versa). In order to
+     	 make path substitution work on such 'mixed' path styles, we need
+     	 to convert foreign dir separators to native ones. */
+ #ifdef HAVE_DOS_BASED_FILE_SYSTEM
+     	  const char from = '/';
+     	  const char to = '\\';
+ #else
+     	  const char from = '\\';
+     	  const char to = '/';
+ #endif
+           unsigned int i=0;
+           for( i=0; i<strlen(rewritten_filename); ++i ) {
+         	  if( rewritten_filename[i] == from ) {
+         		  rewritten_filename[i] = to;
+         	  }
+           }
            make_cleanup (xfree, rewritten_filename);
            filename = rewritten_filename;
          }

Re: proposal: substitute-path handles foreign dir separators

[Eli Zaretskii]

> Date: Fri, 17 Dec 2010 15:18:13 +0100
> From: Raphael Zulliger <zulliger@indel.ch>
> 
>   1. Most importantly: Do we want such a 'feature'? Personally, I think 
> it makes sense to have such a feature.

There were discussions on this a few months ago, you may wish to look
them up.

FWIW, I'm not opposed to having this feature, but I think it should
not be on by default on Posix platforms, because they support file
names with literal backslashes.

>   2. I created this patch because I actually debug binaries on a Linux 
> box which have been created on a Windows machine (GNU toolchain/Cygwin). 
> Empirically, I've realized that 'substitute-path' doesn't work for this 
> purpose. After having single-stepped GDB and modified its code slightly, 
> I ended up with this patch. The point is that I figured it out 
> empirically. It could the case that I justed missed an already existing 
> solution. Please let me know if my patch is crap because there exists 
> another way of handling my use case.

I don't think your approach is ``crap'', but the patch needs work,
IMO.  See below.

> !   if (path[from_len] != '\0' && (!IS_UNIX_DIR_SEPARATOR (path[from_len]) && !IS_DOS_DIR_SEPARATOR (path[from_len])))

This isn't the right way, for 2 reasons:

 . It makes the code uglier than it must be.

 . The support for DOS-style file names is based on compile-time
   macros, so it cannot be turned on and off.  Thus, you leave no fire
   escape for Unix users who just happen to have file names with
   literal backslashes, improbable as that may be.

The first part could be taken care of by crafting a single macro that
replaces the two macros.  The second part calls for an option which
could be tested in the same macro.

> !   if (IS_UNIX_ABSOLUTE_PATH (filename) || IS_DOS_ABSOLUTE_PATH(filename))

Similarly here.

>         if (rewritten_filename != NULL)
>           {
> +     	  /* Especially for embedded systems, it may be the case that a
> +     	 binary has been built on Windows but the embedded system is now
> +     	 being debugged on a Unix machine (and vice versa). In order to
> +     	 make path substitution work on such 'mixed' path styles, we need
> +     	 to convert foreign dir separators to native ones. */
> + #ifdef HAVE_DOS_BASED_FILE_SYSTEM
> +     	  const char from = '/';
> +     	  const char to = '\\';
> + #else
> +     	  const char from = '\\';
> +     	  const char to = '/';
> + #endif
> +           unsigned int i=0;
> +           for( i=0; i<strlen(rewritten_filename); ++i ) {
> +         	  if( rewritten_filename[i] == from ) {
> +         		  rewritten_filename[i] = to;
> +         	  }
> +           }

In this part, I simply don't understand why you needed the
HAVE_DOS_BASED_FILE_SYSTEM branch.  DOS/Windows file-system calls
support forward slashes just fine, so there's no need to rewrite the
slashes.

Finally, please wait for other opinions, as I'm not sure mine is in
consensus.

Re: proposal: substitute-path handles foreign dir separators

[Pedro Alves]

On Friday 17 December 2010 15:26:12, Eli Zaretskii wrote:
> > Date: Fri, 17 Dec 2010 15:18:13 +0100
> > From: Raphael Zulliger <zulliger@indel.ch>
> > 
> >   1. Most importantly: Do we want such a 'feature'? Personally, I think 
> > it makes sense to have such a feature.
> 
> There were discussions on this a few months ago, you may wish to look
> them up.
> 
> FWIW, I'm not opposed to having this feature, but I think it should
> not be on by default on Posix platforms, because they support file
> names with literal backslashes.
> 
> >   2. I created this patch because I actually debug binaries on a Linux 
> > box which have been created on a Windows machine (GNU toolchain/Cygwin). 
> > Empirically, I've realized that 'substitute-path' doesn't work for this 
> > purpose. After having single-stepped GDB and modified its code slightly, 
> > I ended up with this patch. The point is that I figured it out 
> > empirically. It could the case that I justed missed an already existing 
> > solution. Please let me know if my patch is crap because there exists 
> > another way of handling my use case.
> 
> I don't think your approach is ``crap'', but the patch needs work,
> IMO.  See below.
> 
> > !   if (path[from_len] != '\0' && (!IS_UNIX_DIR_SEPARATOR (path[from_len]) && !IS_DOS_DIR_SEPARATOR (path[from_len])))
> 
> This isn't the right way, for 2 reasons:
> 
>  . It makes the code uglier than it must be.
> 
>  . The support for DOS-style file names is based on compile-time
>    macros, so it cannot be turned on and off.  Thus, you leave no fire
>    escape for Unix users who just happen to have file names with
>    literal backslashes, improbable as that may be.
> 
> The first part could be taken care of by crafting a single macro that
> replaces the two macros.  The second part calls for an option which
> could be tested in the same macro.
> 
> > !   if (IS_UNIX_ABSOLUTE_PATH (filename) || IS_DOS_ABSOLUTE_PATH(filename))
> 
> Similarly here.
> 
> >         if (rewritten_filename != NULL)
> >           {
> > +     	  /* Especially for embedded systems, it may be the case that a
> > +     	 binary has been built on Windows but the embedded system is now
> > +     	 being debugged on a Unix machine (and vice versa). In order to
> > +     	 make path substitution work on such 'mixed' path styles, we need
> > +     	 to convert foreign dir separators to native ones. */
> > + #ifdef HAVE_DOS_BASED_FILE_SYSTEM
> > +     	  const char from = '/';
> > +     	  const char to = '\\';
> > + #else
> > +     	  const char from = '\\';
> > +     	  const char to = '/';
> > + #endif
> > +           unsigned int i=0;
> > +           for( i=0; i<strlen(rewritten_filename); ++i ) {
> > +         	  if( rewritten_filename[i] == from ) {
> > +         		  rewritten_filename[i] = to;
> > +         	  }
> > +           }
> 
> In this part, I simply don't understand why you needed the
> HAVE_DOS_BASED_FILE_SYSTEM branch.  DOS/Windows file-system calls
> support forward slashes just fine, so there's no need to rewrite the
> slashes.
> 
> Finally, please wait for other opinions, as I'm not sure mine is in
> consensus.
> 

Here's a patch I wrote a few months back when I was working on
the "set/show target-file-system-kind" support, but put it on
the back burner and never looked at it again.  It adds a new
"set source-filenames-matching-scheme dos/unix" command to select
the behavior at run-time, and adds a new source_filename_cmp function
to be used whenever we are comparing source file names instead of
using FILENAME_CMP.  I never did an exaustive check on all FILENAME_CMP
calls that should be replaced, and neither did much testing on it
when I wrote it.  I've new comfirmed it still builds, but didn't retest
more than that.

-- 
Pedro Alves

9999-99-99  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* buildsym.c: Include "source.h".
	(start_subfile): Use source_filename_cmp instead of FILENAME_CMP.
	* symtab.c: Remove comment on reason of filenames.h inclusion.
	(lookup_symtab): Use source_filename_cmp instead of FILENAME_CMP.
	(append_exact_match_to_sals): Ditto.
	* psymtab.c (lookup_partial_symtab): Ditto.
	* source.c (substitute_path_rule_matches): Ditto.
	(find_substitute_path_rule): Ditto.
	(show_substitute_path_command): Ditto.
	(unset_substitute_path_command): Ditto.
	(source_file_names_dos_based, source_file_names_unix)
	(source_file_names_modes, source_file_names_mode): New.
	(show_source_filenames_matching_scheme_command): New.
	(source_filename_cmp): New.
	(_initialize_source): Install new set/show
	source-filenames-matching-scheme commands.
	* source.h (source_filename_cmp): Declare.
	* dwarf2read.c: Include source.h.
	(dw2_lookup_symtab): Use source_filename_cmp instead of
	FILENAME_CMP.

	include/
	* filenames.h (filename_cmp): Delete declaration.
	(dos_filename_cmp, unix_filename_cmp): New declarations.
	(FILENAME_CMP): Reimplement.

	libiberty/
	* filename_cmp.c (filename_cmp): Delete, split into...
	(dos_filename_cmp, unix_filename_cmp): ... these new functions.

---
 gdb/buildsym.c           |    3 +-
 gdb/dwarf2read.c         |    3 +-
 gdb/psymtab.c            |    8 ++---
 gdb/source.c             |   66 ++++++++++++++++++++++++++++++++++++++++++++---
 gdb/source.h             |   13 +++++++++
 gdb/symtab.c             |   18 ++++++------
 include/filenames.h      |   10 +++++--
 libiberty/filename_cmp.c |   13 ++++-----
 8 files changed, 107 insertions(+), 27 deletions(-)

Index: src/gdb/buildsym.c
===================================================================
--- src.orig/gdb/buildsym.c	2010-10-06 23:13:47.000000000 +0100
+++ src/gdb/buildsym.c	2010-12-17 17:11:42.000000000 +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
@@ -544,7 +545,7 @@ start_subfile (const char *name, const c
       else
 	subfile_name = subfile->name;
 
-      if (FILENAME_CMP (subfile_name, name) == 0)
+      if (source_filename_cmp (subfile_name, name) == 0)
 	{
 	  current_subfile = subfile;
 	  if (subfile_name != subfile->name)
Index: src/gdb/symtab.c
===================================================================
--- src.orig/gdb/symtab.c	2010-11-09 16:13:38.000000000 +0000
+++ src/gdb/symtab.c	2010-12-17 17:13:03.000000000 +0000
@@ -37,7 +37,7 @@
 #include "inferior.h"
 #include "linespec.h"
 #include "source.h"
-#include "filenames.h"		/* for FILENAME_CMP */
+#include "filenames.h"
 #include "objc-lang.h"
 #include "d-lang.h"
 #include "ada-lang.h"
@@ -180,7 +180,7 @@ got_symtab:
 
   ALL_SYMTABS (objfile, s)
   {
-    if (FILENAME_CMP (name, s->filename) == 0)
+    if (source_filename_cmp (name, s->filename) == 0)
       {
 	return s;
       }
@@ -192,7 +192,7 @@ got_symtab:
       {
         const char *fp = symtab_to_fullname (s);
 
-        if (fp != NULL && FILENAME_CMP (full_path, fp) == 0)
+        if (fp != NULL && source_filename_cmp (full_path, fp) == 0)
           {
             return s;
           }
@@ -207,7 +207,7 @@ got_symtab:
             char *rp = gdb_realpath (fullname);
 
             make_cleanup (xfree, rp);
-            if (FILENAME_CMP (real_path, rp) == 0)
+            if (source_filename_cmp (real_path, rp) == 0)
               {
                 return s;
               }
@@ -220,7 +220,7 @@ got_symtab:
   if (lbasename (name) == name)
     ALL_SYMTABS (objfile, s)
     {
-      if (FILENAME_CMP (lbasename (s->filename), name) == 0)
+      if (source_filename_cmp (lbasename (s->filename), name) == 0)
 	return s;
     }
 
@@ -2201,11 +2201,11 @@ find_line_symtab (struct symtab *symtab,
 	struct linetable *l;
 	int ind;
 
-	if (FILENAME_CMP (symtab->filename, s->filename) != 0)
+	if (source_filename_cmp (symtab->filename, s->filename) != 0)
 	  continue;
 	if (symtab->fullname != NULL
 	    && symtab_to_fullname (s) != NULL
-	    && FILENAME_CMP (symtab->fullname, s->fullname) != 0)
+	    && source_filename_cmp (symtab->fullname, s->fullname) != 0)
 	  continue;	
 	l = LINETABLE (s);
 	ind = find_line_common (l, line, &exact);
@@ -4513,14 +4513,14 @@ append_exact_match_to_sals (char *filena
   ALL_PSPACES (pspace)
     ALL_PSPACE_SYMTABS (pspace, objfile, symtab)
     {
-      if (FILENAME_CMP (filename, symtab->filename) == 0)
+      if (source_filename_cmp (filename, symtab->filename) == 0)
 	{
 	  struct linetable *l;
 	  int len;
 
 	  if (fullname != NULL
 	      && symtab_to_fullname (symtab) != NULL
-    	      && FILENAME_CMP (fullname, symtab->fullname) != 0)
+    	      && source_filename_cmp (fullname, symtab->fullname) != 0)
     	    continue;		  
 	  l = LINETABLE (symtab);
 	  if (!l)
Index: src/gdb/psymtab.c
===================================================================
--- src.orig/gdb/psymtab.c	2010-11-25 13:03:20.000000000 +0000
+++ src/gdb/psymtab.c	2010-12-17 17:11:42.000000000 +0000
@@ -81,7 +81,7 @@ lookup_partial_symtab (struct objfile *o
 
   ALL_OBJFILE_PSYMTABS (objfile, pst)
   {
-    if (FILENAME_CMP (name, pst->filename) == 0)
+    if (source_filename_cmp (name, pst->filename) == 0)
       {
 	return (pst);
       }
@@ -92,7 +92,7 @@ lookup_partial_symtab (struct objfile *o
       {
 	psymtab_to_fullname (pst);
 	if (pst->fullname != NULL
-	    && FILENAME_CMP (full_path, pst->fullname) == 0)
+	    && source_filename_cmp (full_path, pst->fullname) == 0)
 	  {
 	    return pst;
 	  }
@@ -107,7 +107,7 @@ lookup_partial_symtab (struct objfile *o
             rp = gdb_realpath (pst->fullname);
             make_cleanup (xfree, rp);
           }
-	if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
+	if (rp != NULL && source_filename_cmp (real_path, rp) == 0)
 	  {
 	    return pst;
 	  }
@@ -119,7 +119,7 @@ lookup_partial_symtab (struct objfile *o
   if (lbasename (name) == name)
     ALL_OBJFILE_PSYMTABS (objfile, pst)
     {
-      if (FILENAME_CMP (lbasename (pst->filename), name) == 0)
+      if (source_filename_cmp (lbasename (pst->filename), name) == 0)
 	return (pst);
     }
 
Index: src/gdb/source.c
===================================================================
--- src.orig/gdb/source.c	2010-11-09 16:20:32.000000000 +0000
+++ src/gdb/source.c	2010-12-17 17:33:20.000000000 +0000
@@ -909,7 +909,7 @@ substitute_path_rule_matches (const stru
   strncpy (path_start, path, from_len);
   path_start[from_len] = '\0';
 
-  if (FILENAME_CMP (path_start, rule->from) != 0)
+  if (source_filename_cmp (path_start, rule->from) != 0)
     return 0;
 
   /* Make sure that the region in the path that matches the substitution
@@ -1751,7 +1751,7 @@ find_substitute_path_rule (const char *f
 
   while (rule != NULL)
     {
-      if (FILENAME_CMP (rule->from, from) == 0)
+      if (source_filename_cmp (rule->from, from) == 0)
         return rule;
       rule = rule->next;
     }
@@ -1847,7 +1847,7 @@ show_substitute_path_command (char *args
 
   while (rule != NULL)
     {
-      if (from == NULL || FILENAME_CMP (rule->from, from) == 0)
+      if (from == NULL || source_filename_cmp (rule->from, from) == 0)
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
@@ -1887,7 +1887,7 @@ unset_substitute_path_command (char *arg
     {
       struct substitute_path_rule *next = rule->next;
 
-      if (from == NULL || FILENAME_CMP (from, rule->from) == 0)
+      if (from == NULL || source_filename_cmp (from, rule->from) == 0)
         {
           delete_substitute_path_rule (rule);
           rule_found = 1;
@@ -1943,6 +1943,46 @@ set_substitute_path_command (char *args,
   forget_cached_source_info ();
 }
 
+/* Allow the user to configure the debugger behavior with respect to
+   source filename matching.  */
+
+static const char source_file_names_dos_based[] = "dos-based";
+static const char source_file_names_unix[] = "unix";
+static const char *source_file_names_modes[] =
+{
+  source_file_names_dos_based,
+  source_file_names_unix
+};
+
+/* Handle binaries compiled on DOS-based filesystems (e.g, Windows),
+   by default, even if GDB itself is not running on such a system.
+   Such binaries may contain debug info with source paths the native
+   path handling functions wouldn't understand (e.g., backslash as
+   directory separator, drive names, and case insensitivity).  The
+   risk of this going wrong is very minor in practice, so it's more
+   useful to leave this as default.  */
+static const char *source_file_names_mode = source_file_names_dos_based;
+
+static void
+show_source_filenames_matching_scheme_command (struct ui_file *file,
+					       int from_tty,
+					       struct cmd_list_element *c,
+					       const char *value)
+{
+  fprintf_filtered (file, _("\
+File name matching scheme for source paths is \"%s\".\n"),
+		    value);
+}
+
+int
+source_filename_cmp (const char *s1, const char *s2)
+{
+  if (source_file_names_mode == source_file_names_dos_based)
+    return dos_filename_cmp (s1, s2);
+  else
+    return unix_filename_cmp (s1, s2);
+}
+
 \f
 void
 _initialize_source (void)
@@ -2058,4 +2098,22 @@ Usage: show substitute-path [FROM]\n\
 Print the rule for substituting FROM in source file names. If FROM\n\
 is not specified, print all substitution rules."),
            &showlist);
+
+  add_setshow_enum_cmd ("source-filenames-matching-scheme",
+			class_support,
+			source_file_names_modes,
+			&source_file_names_mode, _("\
+Set file name matching scheme for source paths"), _("\
+Set file name matching scheme for source paths"),
+			_("\
+If `unix', source paths (e.g., program source paths included in debug \n\
+info) starting the forward slash (`/') character are considered \n\
+absolute, and the directory separator character is the forward slash \n\
+(`/').  If `dos-based' (which is the default), source paths starting with \n\
+a drive name (e.g., `c:'), are also considered absolute, the backslash \n\
+(`\\') is also considered a directory separator, and source filename \n\
+comparisons are not case sensitive."),
+			NULL, /* setfunc */
+			show_source_filenames_matching_scheme_command,
+			&setlist, &showlist);
 }
Index: src/gdb/source.h
===================================================================
--- src.orig/gdb/source.h	2010-03-12 19:12:13.000000000 +0000
+++ src/gdb/source.h	2010-12-17 17:11:42.000000000 +0000
@@ -68,4 +68,17 @@ extern void clear_current_source_symtab_
 
 /* Add a source path substitution rule.  */
 extern void add_substitute_path_rule (char *, char *);
+
+/* Return zero if the two file names S1 and S2 are equivalent.  If not
+   equivalent, the returned value is similar to what strcmp would
+   return.  In other words, it returns a negative value if S1 is less
+   than S2, or a positive value if S2 is greater than S2.
+
+   This function does not normalize file names.  As a result, this
+   function will treat filenames that are spelled differently as
+   different even in the case when the two filenames point to the same
+   underlying file.  However, it does handle the fact that if `set
+   source-filenames-matching-scheme' mode is set to `dos-based',
+   forward and backward slashes are equal.  */
+extern int source_filename_cmp (const char *s1, const char *s2);
 #endif
Index: src/gdb/dwarf2read.c
===================================================================
--- src.orig/gdb/dwarf2read.c	2010-12-13 16:31:03.000000000 +0000
+++ src/gdb/dwarf2read.c	2010-12-17 17:30:37.000000000 +0000
@@ -57,6 +57,7 @@
 #include "vec.h"
 #include "c-lang.h"
 #include "valprint.h"
+#include "source.h"
 
 #include <fcntl.h>
 #include "gdb_string.h"
@@ -2285,7 +2286,7 @@ dw2_lookup_symtab (struct objfile *objfi
 	{
 	  const char *this_name = file_data->file_names[j];
 
-	  if (FILENAME_CMP (name, this_name) == 0)
+	  if (source_filename_cmp (name, this_name) == 0)
 	    {
 	      *result = dw2_instantiate_symtab (objfile, per_cu);
 	      return 1;
Index: src/include/filenames.h
===================================================================
--- src.orig/include/filenames.h	2010-06-16 10:55:28.000000000 +0100
+++ src/include/filenames.h	2010-12-17 17:11:42.000000000 +0000
@@ -70,8 +70,14 @@ extern "C" {
   (IS_DIR_SEPARATOR_1 (dos_based, (f)[0])		 \
    || HAS_DRIVE_SPEC_1 (dos_based, f))
 
-extern int filename_cmp (const char *s1, const char *s2);
-#define FILENAME_CMP(s1, s2)	filename_cmp(s1, s2)
+extern int dos_filename_cmp (const char *s1, const char *s2);
+extern int unix_filename_cmp (const char *s1, const char *s2);
+
+#if (HAVE_DOS_BASED_FILE_SYSTEM)
+#  define FILENAME_CMP(s1, s2)	dos_filename_cmp (s1, s2)
+#else
+#  define FILENAME_CMP(s1, s2)	unix_filename_cmp (s1, s2)
+#endif
 
 #ifdef __cplusplus
 }
Index: src/libiberty/filename_cmp.c
===================================================================
--- src.orig/libiberty/filename_cmp.c	2007-05-04 00:40:11.000000000 +0100
+++ src/libiberty/filename_cmp.c	2010-12-17 17:11:42.000000000 +0000
@@ -48,11 +48,14 @@ and backward slashes are equal.
 */
 
 int
-filename_cmp (const char *s1, const char *s2)
+unix_filename_cmp (const char *s1, const char *s2)
+{
+  return strcmp (s1, s2);
+}
+
+int
+dos_filename_cmp (const char *s1, const char *s2)
 {
-#ifndef HAVE_DOS_BASED_FILE_SYSTEM
-  return strcmp(s1, s2);
-#else
   for (;;)
     {
       int c1 = TOLOWER (*s1);
@@ -73,6 +76,4 @@ filename_cmp (const char *s1, const char
       s1++;
       s2++;
     }
-#endif
 }
-

Re: proposal: substitute-path handles foreign dir separators

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 17 Dec 2010 17:50:11 +0000
> Cc: Raphael Zulliger <zulliger@indel.ch>
> 
> Here's a patch I wrote a few months back when I was working on
> the "set/show target-file-system-kind" support, but put it on
> the back burner and never looked at it again.  It adds a new
> "set source-filenames-matching-scheme dos/unix" command to select
> the behavior at run-time, and adds a new source_filename_cmp function
> to be used whenever we are comparing source file names instead of
> using FILENAME_CMP.

Thanks.  I have 2 comments:

 . if we are going to add this, it needs documentation

 . why only source file names? the issue is relevant to any file name

Re: proposal: substitute-path handles foreign dir separators

[Pedro Alves]

On Friday 17 December 2010 19:23:22, Eli Zaretskii wrote:
> Thanks.  I have 2 comments:
> 
>  . if we are going to add this, it needs documentation

For sure.  I only posted what I had because the subject came up, not
because I considered it completely finished.

>  . why only source file names? the issue is relevant to any file name

Can you give an example?  My thinking is/was we should keep target paths, host
paths and source paths separate worlds.  Target path comparisions are handled
with "set/show target-file-system-kind".  Host paths should be compared with
other host paths using FILENAME_CMP, which uses the appropriate native
host compare function.  The source paths setting is for the case of
e.g., a arm-linux program that was compiled on a Windows machine,
and is later debugged on an x86-linux host gdb (against a arm-linux
gdbserver).  You want gdb to consider the target filesystem of unix
kind, but, you want source path comparisions with paths embedded in
debug info to be permissive by using dos style path comparisons.

-- 
Pedro Alves

Re: proposal: substitute-path handles foreign dir separators

[Pedro Alves]

On Friday 17 December 2010 19:43:56, Pedro Alves wrote:
> On Friday 17 December 2010 19:23:22, Eli Zaretskii wrote:
> > Thanks.  I have 2 comments:
> > 
> >  . if we are going to add this, it needs documentation
> 
> For sure.  I only posted what I had because the subject came up, not
> because I considered it completely finished.
> 
> >  . why only source file names? the issue is relevant to any file name
> 
> Can you give an example?  My thinking is/was we should keep target paths, host
> paths and source paths separate worlds.  Target path comparisions are handled
> with "set/show target-file-system-kind".  Host paths should be compared with
> other host paths using FILENAME_CMP, which uses the appropriate native
> host compare function.  The source paths setting is for the case of
> e.g., a arm-linux program that was compiled on a Windows machine,
> and is later debugged on an x86-linux host gdb (against a arm-linux
> gdbserver).  You want gdb to consider the target filesystem of unix
> kind, but, you want source path comparisions with paths embedded in
> debug info to be permissive by using dos style path comparisons.

Here's the link to the previous short discussion on this, for the
archives.  Sorry, should have included it earlier:

 <http://sourceware.org/ml/gdb-patches/2010-04/msg00759.html>

-- 
Pedro Alves

Re: proposal: substitute-path handles foreign dir separators

[Raphael Zulliger]

On 17.12.2010 16:26, Eli Zaretskii wrote:
> There were discussions on this a few months ago, you may wish to look
> them up.
I just tried to look it up but I couldn't find the particular discussion 
unless you refer to the one mentioned by Pedor Alves:

<http://sourceware.org/ml/gdb-patches/2010-04/msg00759.html>

> This isn't the right way, for 2 reasons:
>
>   . It makes the code uglier than it must be.
>
>   . The support for DOS-style file names is based on compile-time
>     macros, so it cannot be turned on and off.  Thus, you leave no fire
>     escape for Unix users who just happen to have file names with
>     literal backslashes, improbable as that may be.
I agree with both. I didn't thought about the case of using escaped file 
names on Unix systems.
> In this part, I simply don't understand why you needed the
> HAVE_DOS_BASED_FILE_SYSTEM branch.  DOS/Windows file-system calls
> support forward slashes just fine, so there's no need to rewrite the
> slashes.
Thanks. Somehow I wasn't aware of that.
> Finally, please wait for other opinions, as I'm not sure mine is in
> consensus.
>
I think we should forget about my patch as I posted it. Let's continue 
the discussion based on Pedros patch instead.

Thanks.
Raphael

Re: proposal: substitute-path handles foreign dir separators

[Raphael Zulliger]

On 17.12.2010 18:50, Pedro Alves wrote:
>
> Here's a patch I wrote a few months back when I was working on
> the "set/show target-file-system-kind" support, but put it on
> the back burner and never looked at it again.  It adds a new
> "set source-filenames-matching-scheme dos/unix" command to select
> the behavior at run-time, and adds a new source_filename_cmp function
> to be used whenever we are comparing source file names instead of
> using FILENAME_CMP.  I never did an exaustive check on all FILENAME_CMP
> calls that should be replaced, and neither did much testing on it
> when I wrote it.  I've new comfirmed it still builds, but didn't retest
> more than that.
>
I applied your patch and tested it against my use case. First of all I 
need to admit that I really don't have the "overall picture about GDB 
internals" yet. I mention this because I can't judge most parts of your 
patch (psymtab.c, buildsym.c, etc.). Opposed to that, I think I do 
understand the changes you've done in source.c, filenames.h and 
filename_cmp.c.

First of all, your patch doesn't help for my use case. I guess (although 
I didn't verify that) your patch only handles "relative" source file 
names. Unfortunately, in my case, find_and_open_source is called with 
the following argument values:

   filename: 
"X:\\projects\\inos_projects\\inos_trunk\\os\\inos\\src\\inos.cpp"
   dirname: 
"/cygdrive/x/projects/inos_projects/inos_trunk/testing/hwtarget_603/"
   *fullpath: 0

I use the following substitute-path rule:
   set substitute-path X:\\projects\\inos_projects 
/mnt/hgfs/projects/inos_projects/

According to my understanding, if having absolute file names in the 
binary, the following if statement (in find_and_open_source) should 
become true:
   if (IS_ABSOLUTE_PATH (filename))
which, unfortunately, is not the case because IS_ABSOLUTE_PATH 
internally uses Unix style file name rules.

To make a long story short: I think that we still need to do the things 
that are done by my original patch to make my use case working:
  . Create some kind of IS_ABSOLUTE_PATH which (optionally) works for 
all file name styles
  . Create some kind fo IS_DIR_SEPARATOR which (optionally) works for 
all file name styles
  . Make sure we don't pass DOSish file names to openp (the one in 
find_and_open_source) if running on a Unix-like system

Assuming that my statements/observations are right, how would be 
implement these points?
We could do similar things with IS_ABSOLUTE_PATH and IS_DIR_SEPARATOR as 
has been done with the FILENAME_CMP macro by your patch. Having done 
this, we probably need a third options for 
source-filenames-matching-scheme: 'mixed'. As I mentioned in the 
original post, it may happen that you end up with a binary which has 
been linked from libraries that were compiled on different platforms 
(e.g. by different developer teams). As far as I understand GDB's source 
code handling, this would require that IS_ABSOLUTE_PATH and 
IS_DIR_SEPARATOR need to test for both file name styles.

Thanks.
Raphael

Re: proposal: substitute-path handles foreign dir separators

[Pedro Alves]

On Monday 20 December 2010 07:40:53, Raphael Zulliger wrote:
> First of all, your patch doesn't help for my use case. I guess (although 
> I didn't verify that) your patch only handles "relative" source file 
> names. Unfortunately, in my case, find_and_open_source is called with 
> the following argument values:
> 
>    filename: 
> "X:\\projects\\inos_projects\\inos_trunk\\os\\inos\\src\\inos.cpp"
>    dirname: 
> "/cygdrive/x/projects/inos_projects/inos_trunk/testing/hwtarget_603/"
>    *fullpath: 0
> 
> I use the following substitute-path rule:
>    set substitute-path X:\\projects\\inos_projects 
> /mnt/hgfs/projects/inos_projects/
> 
> According to my understanding, if having absolute file names in the 
> binary, the following if statement (in find_and_open_source) should 
> become true:
>    if (IS_ABSOLUTE_PATH (filename))
> which, unfortunately, is not the case because IS_ABSOLUTE_PATH 
> internally uses Unix style file name rules.
> 
> To make a long story short: I think that we still need to do the things 
> that are done by my original patch to make my use case working:
>   . Create some kind of IS_ABSOLUTE_PATH which (optionally) works for 
> all file name styles
>   . Create some kind fo IS_DIR_SEPARATOR which (optionally) works for 
> all file name styles
>   . Make sure we don't pass DOSish file names to openp (the one in 
> find_and_open_source) if running on a Unix-like system
> 
> Assuming that my statements/observations are right, how would be 
> implement these points?
> We could do similar things with IS_ABSOLUTE_PATH and IS_DIR_SEPARATOR as 
> has been done with the FILENAME_CMP macro by your patch. Having done 
> this, we probably need a third options for 
> source-filenames-matching-scheme: 'mixed'. As I mentioned in the 
> original post, it may happen that you end up with a binary which has 
> been linked from libraries that were compiled on different platforms 
> (e.g. by different developer teams). As far as I understand GDB's source 
> code handling, this would require that IS_ABSOLUTE_PATH and 
> IS_DIR_SEPARATOR need to test for both file name styles.

Yes, that sounds like a natural extension to my patch.  In
gdb/filesystem.h, we have IS_TARGET_ABSOLUTE_PATH, etc., to handle
the similar case with target paths (paths to DSOs in the target
filesystem, for example).  It sound like we need to add similar
IS_SOURCE_ABSOLUTE_PATH and friends to source.h, and make substitute-path
use them, quite like gdb/solib.c:solib_find uses IS_TARGET_ABSOLUTE_PATH,
etc.

-- 
Pedro Alves