[patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

Hi,

compare_filenames_for_search did not support IS_ABSOLUTE_PATH search_name and
therefore all its callers had to handle such case specially.

So now compare_filenames_for_search just supports it and all the callers get
simplified.

This patch should have no functionality effect.


Thanks,
Jan


gdb/
2013-01-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* breakpoint.c (clear_command): Remove variable is_abs, unify the
	call of filename_cmp with compare_filenames_for_search.
	* dwarf2read.c (dw2_map_symtabs_matching_filename): Remove variable
	is_abs, unify the call of FILENAME_CMP with
	compare_filenames_for_search.  New gdb_asserts for full_path and
	real_path.  Unify the call of compare_filenames_for_search with
	FILENAME_CMP.
	* psymtab.c (partial_map_symtabs_matching_filename): Likewise.
	* symfile.h (struct quick_symbol_functions): Extend the comment for
	map_symtabs_matching_filename.
	* symtab.c (compare_filenames_for_search): Remove the function comment
	relative path requirement.  Handle absolute filenames.
	(iterate_over_some_symtabs): Remove variable is_abs, unify the call of
	FILENAME_CMP with compare_filenames_for_search.  New gdb_asserts for
	full_path and real_path.  Unify the call of
	compare_filenames_for_search with FILENAME_CMP.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11909,8 +11909,6 @@ clear_command (char *arg, int from_tty)
   make_cleanup (VEC_cleanup (breakpoint_p), &found);
   for (i = 0; i < sals.nelts; i++)
     {
-      int is_abs;
-
       /* If exact pc given, clear bpts at that pc.
          If line given (pc == 0), clear all bpts on specified line.
          If defaulting, clear all bpts on default line
@@ -11924,7 +11922,6 @@ clear_command (char *arg, int from_tty)
          1              0             <can't happen> */
 
       sal = sals.sals[i];
-      is_abs = sal.symtab == NULL ? 1 : IS_ABSOLUTE_PATH (sal.symtab->filename);
 
       /* Find all matching breakpoints and add them to 'found'.  */
       ALL_BREAKPOINTS (b)
@@ -11952,12 +11949,8 @@ clear_command (char *arg, int from_tty)
 		      && sal.pspace == loc->pspace
 		      && loc->line_number == sal.line)
 		    {
-		      if (filename_cmp (loc->symtab->filename,
-					sal.symtab->filename) == 0)
-			line_match = 1;
-		      else if (!IS_ABSOLUTE_PATH (sal.symtab->filename)
-			       && compare_filenames_for_search (loc->symtab->filename,
-								sal.symtab->filename))
+		      if (compare_filenames_for_search (loc->symtab->filename,
+							sal.symtab->filename))
 			line_match = 1;
 		    }
 
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3068,7 +3068,6 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 {
   int i;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   dw2_setup (objfile);
 
@@ -3093,8 +3092,7 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	{
 	  const char *this_name = file_data->file_names[j];
 
-	  if (FILENAME_CMP (name, this_name) == 0
-	      || (!is_abs && compare_filenames_for_search (this_name, name)))
+	  if (compare_filenames_for_search (this_name, name))
 	    {
 	      if (dw2_map_expand_apply (objfile, per_cu,
 					name, full_path, real_path,
@@ -3113,11 +3111,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (full_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (full_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
@@ -3131,11 +3128,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (real_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (real_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -168,7 +168,6 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
 {
   struct partial_symtab *pst;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
   {
@@ -181,8 +180,7 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (pst->anonymous)
       continue;
 
-    if (FILENAME_CMP (name, pst->filename) == 0
-	|| (!is_abs && compare_filenames_for_search (pst->filename, name)))
+    if (compare_filenames_for_search (pst->filename, name))
       {
 	if (partial_map_expand_apply (objfile, name, full_path, real_path,
 				      pst, callback, data))
@@ -199,11 +197,11 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
        this symtab and use its absolute path.  */
     if (full_path != NULL)
       {
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
 	if (pst->fullname != NULL
-	    && (FILENAME_CMP (full_path, pst->fullname) == 0
-		|| (!is_abs && compare_filenames_for_search (pst->fullname,
-							     name))))
+	    && FILENAME_CMP (full_path, pst->fullname) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
@@ -214,15 +212,16 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (real_path != NULL)
       {
         char *rp = NULL;
+
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
         if (pst->fullname != NULL)
           {
             rp = gdb_realpath (pst->fullname);
             make_cleanup (xfree, rp);
           }
-	if (rp != NULL
-	    && (FILENAME_CMP (real_path, rp) == 0
-		|| (!is_abs && compare_filenames_for_search (real_path, name))))
+	if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -159,9 +159,11 @@ struct quick_symbol_functions
   /* Expand and iterate over each "partial" symbol table in OBJFILE
      where the source file is named NAME.
 
-     If NAME is not absolute, a match after a '/' in the symbol
-     table's file name will also work.  FULL_PATH is the absolute file
-     name, and REAL_PATH is the same, run through gdb_realpath.
+     If NAME is not absolute, a match after a '/' in the symbol table's
+     file name will also work, both FULL_PATH and REAL_PATH are NULL
+     then.  If NAME is absolute then both FULL_PATH and REAL_PATH are
+     non-NULL absolute file names.  FULL_PATH is NAME resolved via
+     xfullpath, REAL_PATH is NAME resolved via gdb_realpath.
 
      If a match is found, the "partial" symbol table is expanded.
      Then, this calls iterate_over_some_symtabs (or equivalent) over
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -146,8 +146,8 @@ const struct block *block_found;
 
 /* See whether FILENAME matches SEARCH_NAME using the rule that we
    advertise to the user.  (The manual's description of linespecs
-   describes what we advertise).  We assume that SEARCH_NAME is
-   a relative path.  Returns true if they match, false otherwise.  */
+   describes what we advertise).  Returns true if they match, false
+   otherwise.  */
 
 int
 compare_filenames_for_search (const char *filename, const char *search_name)
@@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
      to put the "c:file.c" name into debug info.  Such compatibility
      works only on GDB built for DOS host.  */
   return (len == search_len
-	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
+	  || (!IS_ABSOLUTE_PATH (search_name)
+	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
 	  || (HAS_DRIVE_SPEC (filename)
 	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
 }
@@ -199,18 +200,10 @@ iterate_over_some_symtabs (const char *name,
 {
   struct symtab *s = NULL;
   const char* base_name = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   for (s = first; s != NULL && s != after_last; s = s->next)
     {
-      /* Exact match is always ok.  */
-      if (FILENAME_CMP (name, s->filename) == 0)
-	{
-	  if (callback (s, data))
-	    return 1;
-	}
-
-      if (!is_abs && compare_filenames_for_search (s->filename, name))
+      if (compare_filenames_for_search (s->filename, name))
 	{
 	  if (callback (s, data))
 	    return 1;
@@ -229,17 +222,13 @@ iterate_over_some_symtabs (const char *name,
       {
         const char *fp = symtab_to_fullname (s);
 
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
         if (FILENAME_CMP (full_path, fp) == 0)
           {
 	    if (callback (s, data))
 	      return 1;
           }
-
-	if (!is_abs && compare_filenames_for_search (fp, name))
-	  {
-	    if (callback (s, data))
-	      return 1;
-	  }
       }
 
     if (real_path != NULL)
@@ -248,6 +237,8 @@ iterate_over_some_symtabs (const char *name,
 	char *rp = gdb_realpath (fullname);
 	struct cleanup *cleanups = make_cleanup (xfree, rp);
 
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	if (FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (callback (s, data))
@@ -256,15 +247,6 @@ iterate_over_some_symtabs (const char *name,
 		return 1;
 	      }
 	  }
-
-	if (!is_abs && compare_filenames_for_search (rp, name))
-	  {
-	    if (callback (s, data))
-	      {
-		do_cleanups (cleanups);
-		return 1;
-	      }
-	  }
 	do_cleanups (cleanups);
       }
     }

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Eli Zaretskii]

> Date: Thu, 17 Jan 2013 22:58:46 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> compare_filenames_for_search did not support IS_ABSOLUTE_PATH search_name and
> therefore all its callers had to handle such case specially.

What code in compare_filenames_for_search would not DTRT with absolute
file names?

>  int
>  compare_filenames_for_search (const char *filename, const char *search_name)
> @@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
>       to put the "c:file.c" name into debug info.  Such compatibility
>       works only on GDB built for DOS host.  */
>    return (len == search_len
> -	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
> +	  || (!IS_ABSOLUTE_PATH (search_name)
> +	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
>  	  || (HAS_DRIVE_SPEC (filename)
>  	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
>  }

I don't understand why the "match up to a slash" rule is now limited
to non-absolute file names.  Perhaps this is related to my first
question above.

Thanks.

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Fri, 18 Jan 2013 08:32:25 +0100, Eli Zaretskii wrote:
> >  int
> >  compare_filenames_for_search (const char *filename, const char *search_name)
> > @@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
> >       to put the "c:file.c" name into debug info.  Such compatibility
> >       works only on GDB built for DOS host.  */
> >    return (len == search_len
> > -	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
> > +	  || (!IS_ABSOLUTE_PATH (search_name)
> > +	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
> >  	  || (HAS_DRIVE_SPEC (filename)
> >  	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
> >  }
> 
> I don't understand why the "match up to a slash" rule is now limited
> to non-absolute file names.

FILENAME may contain for example:           /path/to//file.c
Then we may request to put a breakpoint to:          /file.c:main
And without
the '!IS_ABSOLUTE_PATH (search_name) &&' part it would falsely match.

Made a new testcase.


Thanks,
Jan


gdb/
2013-01-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* breakpoint.c (clear_command): Remove variable is_abs, unify the
	call of filename_cmp with compare_filenames_for_search.
	* dwarf2read.c (dw2_map_symtabs_matching_filename): Remove variable
	is_abs, unify the call of FILENAME_CMP with
	compare_filenames_for_search.  New gdb_asserts for full_path and
	real_path.  Unify the call of compare_filenames_for_search with
	FILENAME_CMP.
	* psymtab.c (partial_map_symtabs_matching_filename): Likewise.
	* symfile.h (struct quick_symbol_functions): Extend the comment for
	map_symtabs_matching_filename.
	* symtab.c (compare_filenames_for_search): Remove the function comment
	relative path requirement.  Handle absolute filenames.
	(iterate_over_some_symtabs): Remove variable is_abs, unify the call of
	FILENAME_CMP with compare_filenames_for_search.  New gdb_asserts for
	full_path and real_path.  Unify the call of
	compare_filenames_for_search with FILENAME_CMP.

gdb/testsuite/
2013-01-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/mi-fullname-deleted.exp: Use double last slash for $srcfileabs.
	(compare_filenames_for_search does not match)
	(compare_filenames_for_search does match): New tests.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11909,8 +11909,6 @@ clear_command (char *arg, int from_tty)
   make_cleanup (VEC_cleanup (breakpoint_p), &found);
   for (i = 0; i < sals.nelts; i++)
     {
-      int is_abs;
-
       /* If exact pc given, clear bpts at that pc.
          If line given (pc == 0), clear all bpts on specified line.
          If defaulting, clear all bpts on default line
@@ -11924,7 +11922,6 @@ clear_command (char *arg, int from_tty)
          1              0             <can't happen> */
 
       sal = sals.sals[i];
-      is_abs = sal.symtab == NULL ? 1 : IS_ABSOLUTE_PATH (sal.symtab->filename);
 
       /* Find all matching breakpoints and add them to 'found'.  */
       ALL_BREAKPOINTS (b)
@@ -11952,12 +11949,8 @@ clear_command (char *arg, int from_tty)
 		      && sal.pspace == loc->pspace
 		      && loc->line_number == sal.line)
 		    {
-		      if (filename_cmp (loc->symtab->filename,
-					sal.symtab->filename) == 0)
-			line_match = 1;
-		      else if (!IS_ABSOLUTE_PATH (sal.symtab->filename)
-			       && compare_filenames_for_search (loc->symtab->filename,
-								sal.symtab->filename))
+		      if (compare_filenames_for_search (loc->symtab->filename,
+							sal.symtab->filename))
 			line_match = 1;
 		    }
 
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3034,7 +3034,6 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 {
   int i;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   dw2_setup (objfile);
 
@@ -3059,8 +3058,7 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	{
 	  const char *this_name = file_data->file_names[j];
 
-	  if (FILENAME_CMP (name, this_name) == 0
-	      || (!is_abs && compare_filenames_for_search (this_name, name)))
+	  if (compare_filenames_for_search (this_name, name))
 	    {
 	      if (dw2_map_expand_apply (objfile, per_cu,
 					name, full_path, real_path,
@@ -3079,11 +3077,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (full_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (full_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
@@ -3097,11 +3094,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (real_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (real_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -168,7 +168,6 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
 {
   struct partial_symtab *pst;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
   {
@@ -181,8 +180,7 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (pst->anonymous)
       continue;
 
-    if (FILENAME_CMP (name, pst->filename) == 0
-	|| (!is_abs && compare_filenames_for_search (pst->filename, name)))
+    if (compare_filenames_for_search (pst->filename, name))
       {
 	if (partial_map_expand_apply (objfile, name, full_path, real_path,
 				      pst, callback, data))
@@ -199,11 +197,11 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
        this symtab and use its absolute path.  */
     if (full_path != NULL)
       {
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
 	if (pst->fullname != NULL
-	    && (FILENAME_CMP (full_path, pst->fullname) == 0
-		|| (!is_abs && compare_filenames_for_search (pst->fullname,
-							     name))))
+	    && FILENAME_CMP (full_path, pst->fullname) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
@@ -214,15 +212,16 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (real_path != NULL)
       {
         char *rp = NULL;
+
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
         if (pst->fullname != NULL)
           {
             rp = gdb_realpath (pst->fullname);
             make_cleanup (xfree, rp);
           }
-	if (rp != NULL
-	    && (FILENAME_CMP (real_path, rp) == 0
-		|| (!is_abs && compare_filenames_for_search (real_path, name))))
+	if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -159,9 +159,11 @@ struct quick_symbol_functions
   /* Expand and iterate over each "partial" symbol table in OBJFILE
      where the source file is named NAME.
 
-     If NAME is not absolute, a match after a '/' in the symbol
-     table's file name will also work.  FULL_PATH is the absolute file
-     name, and REAL_PATH is the same, run through gdb_realpath.
+     If NAME is not absolute, a match after a '/' in the symbol table's
+     file name will also work, both FULL_PATH and REAL_PATH are NULL
+     then.  If NAME is absolute then both FULL_PATH and REAL_PATH are
+     non-NULL absolute file names.  FULL_PATH is NAME resolved via
+     xfullpath, REAL_PATH is NAME resolved via gdb_realpath.
 
      If a match is found, the "partial" symbol table is expanded.
      Then, this calls iterate_over_some_symtabs (or equivalent) over
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -146,8 +146,8 @@ const struct block *block_found;
 
 /* See whether FILENAME matches SEARCH_NAME using the rule that we
    advertise to the user.  (The manual's description of linespecs
-   describes what we advertise).  We assume that SEARCH_NAME is
-   a relative path.  Returns true if they match, false otherwise.  */
+   describes what we advertise).  Returns true if they match, false
+   otherwise.  */
 
 int
 compare_filenames_for_search (const char *filename, const char *search_name)
@@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
      to put the "c:file.c" name into debug info.  Such compatibility
      works only on GDB built for DOS host.  */
   return (len == search_len
-	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
+	  || (!IS_ABSOLUTE_PATH (search_name)
+	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
 	  || (HAS_DRIVE_SPEC (filename)
 	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
 }
@@ -199,18 +200,10 @@ iterate_over_some_symtabs (const char *name,
 {
   struct symtab *s = NULL;
   const char* base_name = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   for (s = first; s != NULL && s != after_last; s = s->next)
     {
-      /* Exact match is always ok.  */
-      if (FILENAME_CMP (name, s->filename) == 0)
-	{
-	  if (callback (s, data))
-	    return 1;
-	}
-
-      if (!is_abs && compare_filenames_for_search (s->filename, name))
+      if (compare_filenames_for_search (s->filename, name))
 	{
 	  if (callback (s, data))
 	    return 1;
@@ -229,17 +222,13 @@ iterate_over_some_symtabs (const char *name,
       {
         const char *fp = symtab_to_fullname (s);
 
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
         if (FILENAME_CMP (full_path, fp) == 0)
           {
 	    if (callback (s, data))
 	      return 1;
           }
-
-	if (!is_abs && compare_filenames_for_search (fp, name))
-	  {
-	    if (callback (s, data))
-	      return 1;
-	  }
       }
 
     if (real_path != NULL)
@@ -248,6 +237,8 @@ iterate_over_some_symtabs (const char *name,
 	char *rp = gdb_realpath (fullname);
 	struct cleanup *cleanups = make_cleanup (xfree, rp);
 
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	if (FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (callback (s, data))
@@ -256,15 +247,6 @@ iterate_over_some_symtabs (const char *name,
 		return 1;
 	      }
 	  }
-
-	if (!is_abs && compare_filenames_for_search (rp, name))
-	  {
-	    if (callback (s, data))
-	      {
-		do_cleanups (cleanups);
-		return 1;
-	      }
-	  }
 	do_cleanups (cleanups);
       }
     }
--- a/gdb/testsuite/gdb.mi/mi-fullname-deleted.exp
+++ b/gdb/testsuite/gdb.mi/mi-fullname-deleted.exp
@@ -24,6 +24,12 @@ if [mi_gdb_start] {
 standard_testfile
 set srcfileabs [standard_output_file $srcfile]
 
+# "//$srcfile" It is used for the test of compare_filenames_for_search.
+if { [regsub {/[^/]+$} $srcfileabs {/\0} srcfileabs] != 1 } {
+    xfail "Cannot double the last slash separator"
+    return -1
+}
+
 if { [regsub {^(/[^/]+)/} $srcfileabs {\1subst/} srcfileabssubst] != 1
      || [regsub {^(/[^/]+)/.*$} $srcfileabs {\1} initdir] != 1 } {
     xfail "Missing root subdirectory"
@@ -49,3 +55,12 @@ mi_gdb_test "-interpreter-exec console \"set substitute-path ${initdir} ${initdi
 mi_gdb_test "-file-list-exec-source-file" ".*\",fullname=\".*\".*" "fullname present"
 
 mi_gdb_test "-file-list-exec-source-file" ".*\",fullname=\"[string_to_regexp $srcfileabssubst]\".*" "substituted fullname"
+
+# Test compare_filenames_for_search does not falsely use absolute filename as
+# a relative one.
+mi_gdb_test "-break-insert -t /$srcfile:main" \
+	    "\\^error,msg=\"No source file named /[string_to_regexp $srcfile]\\.\"" \
+	    "compare_filenames_for_search does not match"
+mi_gdb_test "-break-insert -t $srcfile:main" \
+	    {\^done,bkpt=.*} \
+	    "compare_filenames_for_search does match"

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Eli Zaretskii]

> Date: Fri, 18 Jan 2013 19:39:38 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> On Fri, 18 Jan 2013 08:32:25 +0100, Eli Zaretskii wrote:
> > >  int
> > >  compare_filenames_for_search (const char *filename, const char *search_name)
> > > @@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
> > >       to put the "c:file.c" name into debug info.  Such compatibility
> > >       works only on GDB built for DOS host.  */
> > >    return (len == search_len
> > > -	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
> > > +	  || (!IS_ABSOLUTE_PATH (search_name)
> > > +	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
> > >  	  || (HAS_DRIVE_SPEC (filename)
> > >  	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
> > >  }
> > 
> > I don't understand why the "match up to a slash" rule is now limited
> > to non-absolute file names.
> 
> FILENAME may contain for example:           /path/to//file.c
> Then we may request to put a breakpoint to:          /file.c:main
> And without
> the '!IS_ABSOLUTE_PATH (search_name) &&' part it would falsely match.

Thanks for explaining.  However, IMO i's wrong to test for double
slash with IS_ABSOLUTE_PATH, because the same could happen with
foo//bar.c, right?  So why not explicitly test for consecutive
slashes instead?

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Fri, 18 Jan 2013 20:20:24 +0100, Eli Zaretskii wrote:
> > Date: Fri, 18 Jan 2013 19:39:38 +0100
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > Cc: gdb-patches@sourceware.org
> > 
> > On Fri, 18 Jan 2013 08:32:25 +0100, Eli Zaretskii wrote:
> > > >  int
> > > >  compare_filenames_for_search (const char *filename, const char *search_name)
> > > > @@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
> > > >       to put the "c:file.c" name into debug info.  Such compatibility
> > > >       works only on GDB built for DOS host.  */
> > > >    return (len == search_len
> > > > -	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
> > > > +	  || (!IS_ABSOLUTE_PATH (search_name)
> > > > +	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
> > > >  	  || (HAS_DRIVE_SPEC (filename)
> > > >  	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
> > > >  }
> > > 
> > > I don't understand why the "match up to a slash" rule is now limited
> > > to non-absolute file names.
> > 
> > FILENAME may contain for example:           /path/to//file.c
> > Then we may request to put a breakpoint to:          /file.c:main
> > And without
> > the '!IS_ABSOLUTE_PATH (search_name) &&' part it would falsely match.
> 
> Thanks for explaining.  However, IMO i's wrong to test for double
> slash with IS_ABSOLUTE_PATH, because the same could happen with
> foo//bar.c, right?  So why not explicitly test for consecutive
> slashes instead?

I do not understand much what do you suggest.

Remaining problem is that /path/to//file.c does not match /path/to/file.c or
that /path/file.c does not match /path/to/../file.c and so on.

I find these bugs outside of the scope of this patchset, this patchset only
tried to handle correctly /path/to/file.c matching, which was big enough IMO
to post it and upstream.

This specific patch is "Code cleanup" so it should have no visible changes, it
is only simplification of the caller-callee implementation so it mimics in the
callee (compare_filenames_for_search) what every of the callers was doing.


Thanks,
Jan

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Eli Zaretskii]

> Date: Fri, 18 Jan 2013 20:34:57 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> On Fri, 18 Jan 2013 20:20:24 +0100, Eli Zaretskii wrote:
> > > Date: Fri, 18 Jan 2013 19:39:38 +0100
> > > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > > Cc: gdb-patches@sourceware.org
> > > 
> > > On Fri, 18 Jan 2013 08:32:25 +0100, Eli Zaretskii wrote:
> > > > >  int
> > > > >  compare_filenames_for_search (const char *filename, const char *search_name)
> > > > > @@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
> > > > >       to put the "c:file.c" name into debug info.  Such compatibility
> > > > >       works only on GDB built for DOS host.  */
> > > > >    return (len == search_len
> > > > > -	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
> > > > > +	  || (!IS_ABSOLUTE_PATH (search_name)
> > > > > +	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
> > > > >  	  || (HAS_DRIVE_SPEC (filename)
> > > > >  	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
> > > > >  }
> > > > 
> > > > I don't understand why the "match up to a slash" rule is now limited
> > > > to non-absolute file names.
> > > 
> > > FILENAME may contain for example:           /path/to//file.c
> > > Then we may request to put a breakpoint to:          /file.c:main
> > > And without
> > > the '!IS_ABSOLUTE_PATH (search_name) &&' part it would falsely match.
> > 
> > Thanks for explaining.  However, IMO i's wrong to test for double
> > slash with IS_ABSOLUTE_PATH, because the same could happen with
> > foo//bar.c, right?  So why not explicitly test for consecutive
> > slashes instead?
> 
> I do not understand much what do you suggest.

I'm saying that your addition of IS_ABSOLUTE_PATH to the test above
might fail file-name comparisons where they previously succeeded, and
for reasons that seem wrong to me.

IOW, I simply don't see why the test for IS_ABSOLUTE_PATH should be
added there.  What exactly would go wrong if you don't add it?

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks [resent]

[Jan Kratochvil]

On Fri, 18 Jan 2013 08:32:25 +0100, Eli Zaretskii wrote:
> >  int
> >  compare_filenames_for_search (const char *filename, const char *search_name)
> > @@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
> >       to put the "c:file.c" name into debug info.  Such compatibility
> >       works only on GDB built for DOS host.  */
> >    return (len == search_len
> > -	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
> > +	  || (!IS_ABSOLUTE_PATH (search_name)
> > +	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
> >  	  || (HAS_DRIVE_SPEC (filename)
> >  	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
> >  }
> 
> I don't understand why the "match up to a slash" rule is now limited
> to non-absolute file names.

FILENAME may contain for example:           /path/to//file.c
Then we may request to put a breakpoint to:          /file.c:main
And without
the '!IS_ABSOLUTE_PATH (search_name) &&' part it would falsely match.

Made a new testcase.


Thanks,
Jan


gdb/
2013-01-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* breakpoint.c (clear_command): Remove variable is_abs, unify the
	call of filename_cmp with compare_filenames_for_search.
	* dwarf2read.c (dw2_map_symtabs_matching_filename): Remove variable
	is_abs, unify the call of FILENAME_CMP with
	compare_filenames_for_search.  New gdb_asserts for full_path and
	real_path.  Unify the call of compare_filenames_for_search with
	FILENAME_CMP.
	* psymtab.c (partial_map_symtabs_matching_filename): Likewise.
	* symfile.h (struct quick_symbol_functions): Extend the comment for
	map_symtabs_matching_filename.
	* symtab.c (compare_filenames_for_search): Remove the function comment
	relative path requirement.  Handle absolute filenames.
	(iterate_over_some_symtabs): Remove variable is_abs, unify the call of
	FILENAME_CMP with compare_filenames_for_search.  New gdb_asserts for
	full_path and real_path.  Unify the call of
	compare_filenames_for_search with FILENAME_CMP.

gdb/testsuite/
2013-01-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/mi-fullname-deleted.exp: Use double last slash for $srcfileabs.
	(compare_filenames_for_search does not match)
	(compare_filenames_for_search does match): New tests.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11909,8 +11909,6 @@ clear_command (char *arg, int from_tty)
   make_cleanup (VEC_cleanup (breakpoint_p), &found);
   for (i = 0; i < sals.nelts; i++)
     {
-      int is_abs;
-
       /* If exact pc given, clear bpts at that pc.
          If line given (pc == 0), clear all bpts on specified line.
          If defaulting, clear all bpts on default line
@@ -11924,7 +11922,6 @@ clear_command (char *arg, int from_tty)
          1              0             <can't happen> */
 
       sal = sals.sals[i];
-      is_abs = sal.symtab == NULL ? 1 : IS_ABSOLUTE_PATH (sal.symtab->filename);
 
       /* Find all matching breakpoints and add them to 'found'.  */
       ALL_BREAKPOINTS (b)
@@ -11952,12 +11949,8 @@ clear_command (char *arg, int from_tty)
 		      && sal.pspace == loc->pspace
 		      && loc->line_number == sal.line)
 		    {
-		      if (filename_cmp (loc->symtab->filename,
-					sal.symtab->filename) == 0)
-			line_match = 1;
-		      else if (!IS_ABSOLUTE_PATH (sal.symtab->filename)
-			       && compare_filenames_for_search (loc->symtab->filename,
-								sal.symtab->filename))
+		      if (compare_filenames_for_search (loc->symtab->filename,
+							sal.symtab->filename))
 			line_match = 1;
 		    }
 
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3034,7 +3034,6 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 {
   int i;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   dw2_setup (objfile);
 
@@ -3059,8 +3058,7 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	{
 	  const char *this_name = file_data->file_names[j];
 
-	  if (FILENAME_CMP (name, this_name) == 0
-	      || (!is_abs && compare_filenames_for_search (this_name, name)))
+	  if (compare_filenames_for_search (this_name, name))
 	    {
 	      if (dw2_map_expand_apply (objfile, per_cu,
 					name, full_path, real_path,
@@ -3079,11 +3077,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (full_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (full_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
@@ -3097,11 +3094,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (real_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (real_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -168,7 +168,6 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
 {
   struct partial_symtab *pst;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
   {
@@ -181,8 +180,7 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (pst->anonymous)
       continue;
 
-    if (FILENAME_CMP (name, pst->filename) == 0
-	|| (!is_abs && compare_filenames_for_search (pst->filename, name)))
+    if (compare_filenames_for_search (pst->filename, name))
       {
 	if (partial_map_expand_apply (objfile, name, full_path, real_path,
 				      pst, callback, data))
@@ -199,11 +197,11 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
        this symtab and use its absolute path.  */
     if (full_path != NULL)
       {
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
 	if (pst->fullname != NULL
-	    && (FILENAME_CMP (full_path, pst->fullname) == 0
-		|| (!is_abs && compare_filenames_for_search (pst->fullname,
-							     name))))
+	    && FILENAME_CMP (full_path, pst->fullname) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
@@ -214,15 +212,16 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (real_path != NULL)
       {
         char *rp = NULL;
+
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
         if (pst->fullname != NULL)
           {
             rp = gdb_realpath (pst->fullname);
             make_cleanup (xfree, rp);
           }
-	if (rp != NULL
-	    && (FILENAME_CMP (real_path, rp) == 0
-		|| (!is_abs && compare_filenames_for_search (real_path, name))))
+	if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -159,9 +159,11 @@ struct quick_symbol_functions
   /* Expand and iterate over each "partial" symbol table in OBJFILE
      where the source file is named NAME.
 
-     If NAME is not absolute, a match after a '/' in the symbol
-     table's file name will also work.  FULL_PATH is the absolute file
-     name, and REAL_PATH is the same, run through gdb_realpath.
+     If NAME is not absolute, a match after a '/' in the symbol table's
+     file name will also work, both FULL_PATH and REAL_PATH are NULL
+     then.  If NAME is absolute then both FULL_PATH and REAL_PATH are
+     non-NULL absolute file names.  FULL_PATH is NAME resolved via
+     xfullpath, REAL_PATH is NAME resolved via gdb_realpath.
 
      If a match is found, the "partial" symbol table is expanded.
      Then, this calls iterate_over_some_symtabs (or equivalent) over
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -146,8 +146,8 @@ const struct block *block_found;
 
 /* See whether FILENAME matches SEARCH_NAME using the rule that we
    advertise to the user.  (The manual's description of linespecs
-   describes what we advertise).  We assume that SEARCH_NAME is
-   a relative path.  Returns true if they match, false otherwise.  */
+   describes what we advertise).  Returns true if they match, false
+   otherwise.  */
 
 int
 compare_filenames_for_search (const char *filename, const char *search_name)
@@ -171,7 +171,8 @@ compare_filenames_for_search (const char *filename, const char *search_name)
      to put the "c:file.c" name into debug info.  Such compatibility
      works only on GDB built for DOS host.  */
   return (len == search_len
-	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
+	  || (!IS_ABSOLUTE_PATH (search_name)
+	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
 	  || (HAS_DRIVE_SPEC (filename)
 	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
 }
@@ -199,18 +200,10 @@ iterate_over_some_symtabs (const char *name,
 {
   struct symtab *s = NULL;
   const char* base_name = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   for (s = first; s != NULL && s != after_last; s = s->next)
     {
-      /* Exact match is always ok.  */
-      if (FILENAME_CMP (name, s->filename) == 0)
-	{
-	  if (callback (s, data))
-	    return 1;
-	}
-
-      if (!is_abs && compare_filenames_for_search (s->filename, name))
+      if (compare_filenames_for_search (s->filename, name))
 	{
 	  if (callback (s, data))
 	    return 1;
@@ -229,17 +222,13 @@ iterate_over_some_symtabs (const char *name,
       {
         const char *fp = symtab_to_fullname (s);
 
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
         if (FILENAME_CMP (full_path, fp) == 0)
           {
 	    if (callback (s, data))
 	      return 1;
           }
-
-	if (!is_abs && compare_filenames_for_search (fp, name))
-	  {
-	    if (callback (s, data))
-	      return 1;
-	  }
       }
 
     if (real_path != NULL)
@@ -248,6 +237,8 @@ iterate_over_some_symtabs (const char *name,
 	char *rp = gdb_realpath (fullname);
 	struct cleanup *cleanups = make_cleanup (xfree, rp);
 
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	if (FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (callback (s, data))
@@ -256,15 +247,6 @@ iterate_over_some_symtabs (const char *name,
 		return 1;
 	      }
 	  }
-
-	if (!is_abs && compare_filenames_for_search (rp, name))
-	  {
-	    if (callback (s, data))
-	      {
-		do_cleanups (cleanups);
-		return 1;
-	      }
-	  }
 	do_cleanups (cleanups);
       }
     }
--- a/gdb/testsuite/gdb.mi/mi-fullname-deleted.exp
+++ b/gdb/testsuite/gdb.mi/mi-fullname-deleted.exp
@@ -24,6 +24,12 @@ if [mi_gdb_start] {
 standard_testfile
 set srcfileabs [standard_output_file $srcfile]
 
+# "//$srcfile" It is used for the test of compare_filenames_for_search.
+if { [regsub {/[^/]+$} $srcfileabs {/\0} srcfileabs] != 1 } {
+    xfail "Cannot double the last slash separator"
+    return -1
+}
+
 if { [regsub {^(/[^/]+)/} $srcfileabs {\1subst/} srcfileabssubst] != 1
      || [regsub {^(/[^/]+)/.*$} $srcfileabs {\1} initdir] != 1 } {
     xfail "Missing root subdirectory"
@@ -49,3 +55,12 @@ mi_gdb_test "-interpreter-exec console \"set substitute-path ${initdir} ${initdi
 mi_gdb_test "-file-list-exec-source-file" ".*\",fullname=\".*\".*" "fullname present"
 
 mi_gdb_test "-file-list-exec-source-file" ".*\",fullname=\"[string_to_regexp $srcfileabssubst]\".*" "substituted fullname"
+
+# Test compare_filenames_for_search does not falsely use absolute filename as
+# a relative one.
+mi_gdb_test "-break-insert -t /$srcfile:main" \
+	    "\\^error,msg=\"No source file named /[string_to_regexp $srcfile]\\.\"" \
+	    "compare_filenames_for_search does not match"
+mi_gdb_test "-break-insert -t $srcfile:main" \
+	    {\^done,bkpt=.*} \
+	    "compare_filenames_for_search does match"

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Fri, 18 Jan 2013 21:40:11 +0100, Eli Zaretskii wrote:
> I'm saying that your addition of IS_ABSOLUTE_PATH to the test above
> might fail file-name comparisons where they previously succeeded, and
> for reasons that seem wrong to me.

Could you give an example?  Previously it was forbidden/unspecified what
happens when you call compare_filenames_for_search
with IS_ABSOLUTE_PATH (search_name).


> IOW, I simply don't see why the test for IS_ABSOLUTE_PATH should be
> added there.  What exactly would go wrong if you don't add it?

When you revert this change:

-	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
+	  || (!IS_ABSOLUTE_PATH (search_name)
+	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))

then the new testcase FAILs

+mi_gdb_test "-break-insert -t /$srcfile:main" \
+           "\\^error,msg=\"No source file named /[string_to_regexp $srcfile]\\.\"" \
+           "compare_filenames_for_search does not match"

because the breakpoint will be placed there, it should not (error is expected
and correct).

I hope we agree that placing a breakpoint to
	/filename.c:main 
should not be successful even if one of the source files is named:
	/path/to//filename.c


Thanks,
Jan

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Eli Zaretskii]

> Date: Fri, 18 Jan 2013 22:10:02 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> On Fri, 18 Jan 2013 21:40:11 +0100, Eli Zaretskii wrote:
> > I'm saying that your addition of IS_ABSOLUTE_PATH to the test above
> > might fail file-name comparisons where they previously succeeded, and
> > for reasons that seem wrong to me.
> 
> Could you give an example?  Previously it was forbidden/unspecified what
> happens when you call compare_filenames_for_search
> with IS_ABSOLUTE_PATH (search_name).

Any absolute file name would be an example.

> I hope we agree that placing a breakpoint to
> 	/filename.c:main 
> should not be successful even if one of the source files is named:
> 	/path/to//filename.c

Yes, we do.  But IS_ABSOLUTE_PATH tests more than just whether there's
a slash at the beginning of "/filename.c".  I'm saying we should test
explicitly for the slash, not for the file name being absolute.  IOW,
using IS_ABSOLUTE_PATH here obfuscates the real intent of the code.

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Sat, 19 Jan 2013 07:50:13 +0100, Eli Zaretskii wrote:
> > Could you give an example?  Previously it was forbidden/unspecified what
> > happens when you call compare_filenames_for_search
> > with IS_ABSOLUTE_PATH (search_name).
> 
> Any absolute file name would be an example.

Could you state literal SEARCH_NAME, literal FILENAME, what is a result you
expect and what do you you think is the actual result with this patch?

I have double/triple checked this patch and I do not see a bug there.


> > I hope we agree that placing a breakpoint to
> > 	/filename.c:main 
> > should not be successful even if one of the source files is named:
> > 	/path/to//filename.c
> 
> Yes, we do.  But IS_ABSOLUTE_PATH tests more than just whether there's
> a slash at the beginning of "/filename.c".  I'm saying we should test
> explicitly for the slash, not for the file name being absolute.  IOW,
> using IS_ABSOLUTE_PATH here obfuscates the real intent of the code.

I think IS_ABSOLUTE_PATH is there really right, we need to test if the
SEARCH_NAME should match from the beginning of FILENAME or if it can match
only its trailing part.

For example when asking for a breakpoint at:
	c:\filename.c:main
it must not match a debug info filename:
	d:\foo\c:\filename.c
This case would incorrectly match if only 'IS_DIR_SEPARATOR (search_name[0])'
was tested there.

Moreover this patch is a "Code cleanup" and the callers were already using
IS_ABSOLUTE_PATH.  So if IS_ABSOLUTE_PATH is wrong (which IMO so far it is
not) then it is still a new patch / unrelated fix, not the scope of this
patch.


Thanks,
Jan

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Eli Zaretskii]

> Date: Sat, 19 Jan 2013 15:09:14 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> On Sat, 19 Jan 2013 07:50:13 +0100, Eli Zaretskii wrote:
> > > Could you give an example?  Previously it was forbidden/unspecified what
> > > happens when you call compare_filenames_for_search
> > > with IS_ABSOLUTE_PATH (search_name).
> > 
> > Any absolute file name would be an example.
> 
> Could you state literal SEARCH_NAME, literal FILENAME, what is a result you
> expect and what do you you think is the actual result with this patch?
> 
> I have double/triple checked this patch and I do not see a bug there.

It might not be a bug, but the code doesn't tell what it means, and
has no comments to explain its subtleties.  IOW, it isn't clean.

> For example when asking for a breakpoint at:
> 	c:\filename.c:main
> it must not match a debug info filename:
> 	d:\foo\c:\filename.c

Why not?

> Moreover this patch is a "Code cleanup" and the callers were already using
> IS_ABSOLUTE_PATH.  So if IS_ABSOLUTE_PATH is wrong (which IMO so far it is
> not) then it is still a new patch / unrelated fix, not the scope of this
> patch.

Whatever.  I'm still unconvinced, I think the code is not sufficiently
cleaned up.  But I'm tired of arguing.

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Sat, 19 Jan 2013 16:18:57 +0100, Eli Zaretskii wrote:
> > For example when asking for a breakpoint at:
> > 	c:\filename.c:main
> > it must not match a debug info filename:
> > 	d:\foo\c:\filename.c
> 
> Why not?

Not sure how to reply that.

One uses absolute filenames (SEARCH_NAME in this case) to prevent any
ambiguities (resolved as multi-location breakpoints by GDB) due to the
dependence on "current directory" (which does not exist in GDB during linespec
resolution).

If an absolute filename can match trailing part of a longer filename then it
is not absolute filename by definition.

"c:\filename.c" is absolute filename by definition so it must not match
trailing part of a longer filename.


> Whatever.  I'm still unconvinced, I think the code is not sufficiently
> cleaned up.  But I'm tired of arguing.

Me too.  I will try to add some comment there in a next patch.


Thanks,
Jan

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Eli Zaretskii]

> Date: Sat, 19 Jan 2013 16:27:31 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> "c:\filename.c" is absolute filename by definition so it must not match
> trailing part of a longer filename.

I agree, but the code never tests the other file name for being
absolute.  If it did, and then insisted that the other file name must
also be absolute, then I'd agree.

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Sat, 19 Jan 2013 16:27:31 +0100, Jan Kratochvil wrote:
> I will try to add some comment there in a next patch.

Done.


Thanks,
Jan


gdb/
2013-01-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup.
	* breakpoint.c (clear_command): Remove variable is_abs, unify the
	call of filename_cmp with compare_filenames_for_search.
	* dwarf2read.c (dw2_map_symtabs_matching_filename): Remove variable
	is_abs, unify the call of FILENAME_CMP with
	compare_filenames_for_search.  New gdb_asserts for full_path and
	real_path.  Unify the call of compare_filenames_for_search with
	FILENAME_CMP.
	* psymtab.c (partial_map_symtabs_matching_filename): Likewise.
	* symfile.h (struct quick_symbol_functions): Extend the comment for
	map_symtabs_matching_filename.
	* symtab.c (compare_filenames_for_search): Remove the function comment
	relative path requirement.  Handle absolute filenames.
	(iterate_over_some_symtabs): Remove variable is_abs, unify the call of
	FILENAME_CMP with compare_filenames_for_search.  New gdb_asserts for
	full_path and real_path.  Unify the call of
	compare_filenames_for_search with FILENAME_CMP.

gdb/testsuite/
2013-01-18  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.mi/mi-fullname-deleted.exp: Use double last slash for $srcfileabs.
	(compare_filenames_for_search does not match)
	(compare_filenames_for_search does match): New tests.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11909,8 +11909,6 @@ clear_command (char *arg, int from_tty)
   make_cleanup (VEC_cleanup (breakpoint_p), &found);
   for (i = 0; i < sals.nelts; i++)
     {
-      int is_abs;
-
       /* If exact pc given, clear bpts at that pc.
          If line given (pc == 0), clear all bpts on specified line.
          If defaulting, clear all bpts on default line
@@ -11924,7 +11922,6 @@ clear_command (char *arg, int from_tty)
          1              0             <can't happen> */
 
       sal = sals.sals[i];
-      is_abs = sal.symtab == NULL ? 1 : IS_ABSOLUTE_PATH (sal.symtab->filename);
 
       /* Find all matching breakpoints and add them to 'found'.  */
       ALL_BREAKPOINTS (b)
@@ -11952,12 +11949,8 @@ clear_command (char *arg, int from_tty)
 		      && sal.pspace == loc->pspace
 		      && loc->line_number == sal.line)
 		    {
-		      if (filename_cmp (loc->symtab->filename,
-					sal.symtab->filename) == 0)
-			line_match = 1;
-		      else if (!IS_ABSOLUTE_PATH (sal.symtab->filename)
-			       && compare_filenames_for_search (loc->symtab->filename,
-								sal.symtab->filename))
+		      if (compare_filenames_for_search (loc->symtab->filename,
+							sal.symtab->filename))
 			line_match = 1;
 		    }
 
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3034,7 +3034,6 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 {
   int i;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   dw2_setup (objfile);
 
@@ -3059,8 +3058,7 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	{
 	  const char *this_name = file_data->file_names[j];
 
-	  if (FILENAME_CMP (name, this_name) == 0
-	      || (!is_abs && compare_filenames_for_search (this_name, name)))
+	  if (compare_filenames_for_search (this_name, name))
 	    {
 	      if (dw2_map_expand_apply (objfile, per_cu,
 					name, full_path, real_path,
@@ -3079,11 +3077,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (full_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (full_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
@@ -3097,11 +3094,10 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name,
 	      const char *this_real_name = dw2_get_real_path (objfile,
 							      file_data, j);
 
+	      gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	      gdb_assert (IS_ABSOLUTE_PATH (name));
 	      if (this_real_name != NULL
-		  && (FILENAME_CMP (real_path, this_real_name) == 0
-		      || (!is_abs
-			  && compare_filenames_for_search (this_real_name,
-							   name))))
+		  && FILENAME_CMP (real_path, this_real_name) == 0)
 		{
 		  if (dw2_map_expand_apply (objfile, per_cu,
 					    name, full_path, real_path,
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -168,7 +168,6 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
 {
   struct partial_symtab *pst;
   const char *name_basename = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
   {
@@ -181,8 +180,7 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (pst->anonymous)
       continue;
 
-    if (FILENAME_CMP (name, pst->filename) == 0
-	|| (!is_abs && compare_filenames_for_search (pst->filename, name)))
+    if (compare_filenames_for_search (pst->filename, name))
       {
 	if (partial_map_expand_apply (objfile, name, full_path, real_path,
 				      pst, callback, data))
@@ -199,11 +197,11 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
        this symtab and use its absolute path.  */
     if (full_path != NULL)
       {
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
 	if (pst->fullname != NULL
-	    && (FILENAME_CMP (full_path, pst->fullname) == 0
-		|| (!is_abs && compare_filenames_for_search (pst->fullname,
-							     name))))
+	    && FILENAME_CMP (full_path, pst->fullname) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
@@ -214,15 +212,16 @@ partial_map_symtabs_matching_filename (struct objfile *objfile,
     if (real_path != NULL)
       {
         char *rp = NULL;
+
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	psymtab_to_fullname (pst);
         if (pst->fullname != NULL)
           {
             rp = gdb_realpath (pst->fullname);
             make_cleanup (xfree, rp);
           }
-	if (rp != NULL
-	    && (FILENAME_CMP (real_path, rp) == 0
-		|| (!is_abs && compare_filenames_for_search (real_path, name))))
+	if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (partial_map_expand_apply (objfile, name, full_path, real_path,
 					  pst, callback, data))
--- a/gdb/symfile.h
+++ b/gdb/symfile.h
@@ -159,9 +159,11 @@ struct quick_symbol_functions
   /* Expand and iterate over each "partial" symbol table in OBJFILE
      where the source file is named NAME.
 
-     If NAME is not absolute, a match after a '/' in the symbol
-     table's file name will also work.  FULL_PATH is the absolute file
-     name, and REAL_PATH is the same, run through gdb_realpath.
+     If NAME is not absolute, a match after a '/' in the symbol table's
+     file name will also work, both FULL_PATH and REAL_PATH are NULL
+     then.  If NAME is absolute then both FULL_PATH and REAL_PATH are
+     non-NULL absolute file names.  FULL_PATH is NAME resolved via
+     xfullpath, REAL_PATH is NAME resolved via gdb_realpath.
 
      If a match is found, the "partial" symbol table is expanded.
      Then, this calls iterate_over_some_symtabs (or equivalent) over
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -146,8 +146,8 @@ const struct block *block_found;
 
 /* See whether FILENAME matches SEARCH_NAME using the rule that we
    advertise to the user.  (The manual's description of linespecs
-   describes what we advertise).  We assume that SEARCH_NAME is
-   a relative path.  Returns true if they match, false otherwise.  */
+   describes what we advertise).  Returns true if they match, false
+   otherwise.  */
 
 int
 compare_filenames_for_search (const char *filename, const char *search_name)
@@ -166,12 +166,18 @@ compare_filenames_for_search (const char *filename, const char *search_name)
      preceding the trailing SEARCH_NAME segment of FILENAME must be a
      directory separator.
 
+     The check !IS_ABSOLUTE_PATH ensures SEARCH_NAME "/dir/file.c"
+     cannot match FILENAME "/path//dir/file.c" - as user has requested
+     absolute path.  The sama applies for "c:\file.c" possibly
+     incorrectly hypothetically matching "d:\dir\c:\file.c".
+
      The HAS_DRIVE_SPEC purpose is to make FILENAME "c:file.c"
      compatible with SEARCH_NAME "file.c".  In such case a compiler had
      to put the "c:file.c" name into debug info.  Such compatibility
      works only on GDB built for DOS host.  */
   return (len == search_len
-	  || IS_DIR_SEPARATOR (filename[len - search_len - 1])
+	  || (!IS_ABSOLUTE_PATH (search_name)
+	      && IS_DIR_SEPARATOR (filename[len - search_len - 1]))
 	  || (HAS_DRIVE_SPEC (filename)
 	      && STRIP_DRIVE_SPEC (filename) == &filename[len - search_len]));
 }
@@ -199,18 +205,10 @@ iterate_over_some_symtabs (const char *name,
 {
   struct symtab *s = NULL;
   const char* base_name = lbasename (name);
-  int is_abs = IS_ABSOLUTE_PATH (name);
 
   for (s = first; s != NULL && s != after_last; s = s->next)
     {
-      /* Exact match is always ok.  */
-      if (FILENAME_CMP (name, s->filename) == 0)
-	{
-	  if (callback (s, data))
-	    return 1;
-	}
-
-      if (!is_abs && compare_filenames_for_search (s->filename, name))
+      if (compare_filenames_for_search (s->filename, name))
 	{
 	  if (callback (s, data))
 	    return 1;
@@ -229,17 +227,13 @@ iterate_over_some_symtabs (const char *name,
       {
         const char *fp = symtab_to_fullname (s);
 
+	gdb_assert (IS_ABSOLUTE_PATH (full_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
         if (FILENAME_CMP (full_path, fp) == 0)
           {
 	    if (callback (s, data))
 	      return 1;
           }
-
-	if (!is_abs && compare_filenames_for_search (fp, name))
-	  {
-	    if (callback (s, data))
-	      return 1;
-	  }
       }
 
     if (real_path != NULL)
@@ -248,6 +242,8 @@ iterate_over_some_symtabs (const char *name,
 	char *rp = gdb_realpath (fullname);
 	struct cleanup *cleanups = make_cleanup (xfree, rp);
 
+	gdb_assert (IS_ABSOLUTE_PATH (real_path));
+	gdb_assert (IS_ABSOLUTE_PATH (name));
 	if (FILENAME_CMP (real_path, rp) == 0)
 	  {
 	    if (callback (s, data))
@@ -256,15 +252,6 @@ iterate_over_some_symtabs (const char *name,
 		return 1;
 	      }
 	  }
-
-	if (!is_abs && compare_filenames_for_search (rp, name))
-	  {
-	    if (callback (s, data))
-	      {
-		do_cleanups (cleanups);
-		return 1;
-	      }
-	  }
 	do_cleanups (cleanups);
       }
     }
--- a/gdb/testsuite/gdb.mi/mi-fullname-deleted.exp
+++ b/gdb/testsuite/gdb.mi/mi-fullname-deleted.exp
@@ -24,6 +24,12 @@ if [mi_gdb_start] {
 standard_testfile
 set srcfileabs [standard_output_file $srcfile]
 
+# "//$srcfile" It is used for the test of compare_filenames_for_search.
+if { [regsub {/[^/]+$} $srcfileabs {/\0} srcfileabs] != 1 } {
+    xfail "Cannot double the last slash separator"
+    return -1
+}
+
 if { [regsub {^(/[^/]+)/} $srcfileabs {\1subst/} srcfileabssubst] != 1
      || [regsub {^(/[^/]+)/.*$} $srcfileabs {\1} initdir] != 1 } {
     xfail "Missing root subdirectory"
@@ -49,3 +55,12 @@ mi_gdb_test "-interpreter-exec console \"set substitute-path ${initdir} ${initdi
 mi_gdb_test "-file-list-exec-source-file" ".*\",fullname=\".*\".*" "fullname present"
 
 mi_gdb_test "-file-list-exec-source-file" ".*\",fullname=\"[string_to_regexp $srcfileabssubst]\".*" "substituted fullname"
+
+# Test compare_filenames_for_search does not falsely use absolute filename as
+# a relative one.
+mi_gdb_test "-break-insert -t /$srcfile:main" \
+	    "\\^error,msg=\"No source file named /[string_to_regexp $srcfile]\\.\"" \
+	    "compare_filenames_for_search does not match"
+mi_gdb_test "-break-insert -t $srcfile:main" \
+	    {\^done,bkpt=.*} \
+	    "compare_filenames_for_search does match"

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> 	Code cleanup.
Jan> 	* breakpoint.c (clear_command): Remove variable is_abs, unify the
Jan> 	call of filename_cmp with compare_filenames_for_search.
[...]

Jan>      if (full_path != NULL)
Jan>        {
Jan> +	gdb_assert (IS_ABSOLUTE_PATH (full_path));
Jan> +	gdb_assert (IS_ABSOLUTE_PATH (name));

I wonder if these can really never trigger.

I think under some obscure conditions, xfullpath can return a relative
path, since gdb_realpath can.

Tom

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Jan Kratochvil]

On Mon, 21 Jan 2013 19:45:57 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> 	Code cleanup.
> Jan> 	* breakpoint.c (clear_command): Remove variable is_abs, unify the
> Jan> 	call of filename_cmp with compare_filenames_for_search.
> [...]
> 
> Jan>      if (full_path != NULL)
> Jan>        {
> Jan> +	gdb_assert (IS_ABSOLUTE_PATH (full_path));
> Jan> +	gdb_assert (IS_ABSOLUTE_PATH (name));
> 
> I wonder if these can really never trigger.
> 
> I think under some obscure conditions, xfullpath can return a relative
> path, since gdb_realpath can.

xfullpath is gone in a new patchset so let's talk just about gdb_realpath.

May 'gdb_realpath (absolute_path)' really return a relative path?  I assume it
cannot although I do not see it guaranteed for example in POSIX.  No other
cases may happen.

I have restricted when quick_symbol_functions->map_symtabs_matching_filename
may be called, according to the calling cases used by the only existing
caller.


Thanks,
Jan

Re: [patch 2/9] Code cleanup: Drop IS_ABSOLUTE_PATH checks

[Tom Tromey]

Jan> May 'gdb_realpath (absolute_path)' really return a relative path?

I doubt it.
I didn't realize it was only called with absolute paths.

Tom