From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18502 invoked by alias); 17 Dec 2010 17:50:52 -0000 Received: (qmail 18478 invoked by uid 22791); 17 Dec 2010 17:50:48 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,TW_BJ,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 17 Dec 2010 17:50:41 +0000 Received: (qmail 15502 invoked from network); 17 Dec 2010 17:50:39 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 17 Dec 2010 17:50:39 -0000 From: Pedro Alves To: gdb-patches@sourceware.org, Eli Zaretskii Subject: Re: proposal: substitute-path handles foreign dir separators Date: Fri, 17 Dec 2010 17:50:00 -0000 User-Agent: KMail/1.13.5 (Linux/2.6.33-29-realtime; KDE/4.4.5; x86_64; ; ) Cc: Raphael Zulliger References: <4D0B7125.2010203@indel.ch> <834oach14b.fsf@gnu.org> In-Reply-To: <834oach14b.fsf@gnu.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201012171750.36936.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-12/txt/msg00343.txt.bz2 On Friday 17 December 2010 15:26:12, Eli Zaretskii wrote: > > Date: Fri, 17 Dec 2010 15:18:13 +0100 > > From: Raphael Zulliger > > > > 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 > + 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 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); +} + 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 #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 } -