[RFC] Fix source path lookup immediately after substitute-path

[RFC] Fix source path lookup immediately after substitute-path

[Daniel Jacobowitz]

I've been frustrated several times by this routine.  For the test I
compiled 'main.c' to 'main', then moved main.c into another directory.

+list main
1       main.c: No such file or directory.
        in main.c

All is well so far.

+set substitute-path /scratch/dan/eabi44 /scratch/dan/eabi44/backup
+list main
1       in main.c

Where's my file?  Maybe if I prod GDB it'll look again.

+dir
+list main
1       in main.c

Nope!  Still can't find it!

+dir /no
Warning: /no: No such file or directory.
+list main
1       int main(){}

I discovered, by reading the source, that "dir" with a directory or
listing another source file would do the trick.  There are two
different functions to invalidate cached source directory locations.
Some places call one but not the other; some call both; some
("set substitute-path" for instance) call neither.  This patch
combines the two functions and makes them be reliably called.

I spent a little while trying to write a portable test case for this
and eventually gave up.

Any thoughts on this patch?  May I include it in 7.0?

-- 
Daniel Jacobowitz
CodeSourcery

2009-09-22  Daniel Jacobowitz  <dan@codesourcery.com>

	gdb/
	* source.c (forget_cached_source_info): Clear last_source_visited.
	(init_last_source_visited): Delete.
	(directory_command): Do not clear last_source_visited.  Call
	forget_cached_source_info only if required.
	(unset_substitute_path_command, set_substitute_path_command): Call
	forget_cached_source_info.
	* mi/mi-cmd-env.c (mi_cmd_env_dir): Do not call
	init_last_source_visited.
	* defs.h (init_last_source_visited): Delete declaration.

Index: source.c
===================================================================
--- source.c	(revision 262056)
+++ source.c	(working copy)
@@ -345,6 +345,8 @@ forget_cached_source_info (void)
 	  }
       }
     }
+
+  last_source_visited = NULL;
 }
 
 void
@@ -357,12 +359,6 @@ init_source_path (void)
   forget_cached_source_info ();
 }
 
-void
-init_last_source_visited (void)
-{
-  last_source_visited = NULL;
-}
-
 /* Add zero or more directories to the front of the source path.  */
 
 void
@@ -381,11 +377,10 @@ directory_command (char *dirname, int fr
   else
     {
       mod_path (dirname, &source_path);
-      last_source_visited = NULL;
+      forget_cached_source_info ();
     }
   if (from_tty)
     show_directories ((char *) 0, from_tty);
-  forget_cached_source_info ();
 }
 
 /* Add a path given with the -d command line switch.
@@ -1884,6 +1879,8 @@ unset_substitute_path_command (char *arg
 
   if (from != NULL && !rule_found)
     error (_("No substitution rule defined for `%s'"), from);
+
+  forget_cached_source_info ();
 }
 
 /* Add a new source path substitution rule.  */
@@ -1922,6 +1919,7 @@ set_substitute_path_command (char *args,
   /* Insert the new substitution rule.  */
 
   add_substitute_path_rule (argv[0], argv[1]);
+  forget_cached_source_info ();
 }
 
 \f
Index: mi/mi-cmd-env.c
===================================================================
--- mi/mi-cmd-env.c	(revision 262056)
+++ mi/mi-cmd-env.c	(working copy)
@@ -232,7 +232,6 @@ mi_cmd_env_dir (char *command, char **ar
 
   for (i = argc - 1; i >= 0; --i)
     env_mod_path (argv[i], &source_path);
-  init_last_source_visited ();
 
   ui_out_field_string (uiout, "source-path", source_path);
   forget_cached_source_info ();
Index: defs.h
===================================================================
--- defs.h	(revision 262056)
+++ defs.h	(working copy)
@@ -636,8 +636,6 @@ extern char *source_path;
 
 extern void init_source_path (void);
 
-extern void init_last_source_visited (void);
-
 /* From exec.c */
 
 /* Take over the 'find_mapped_memory' vector from exec.c. */

Re: [RFC] Fix source path lookup immediately after substitute-path

[Joel Brobecker]

Hi Daniel,

> Any thoughts on this patch?  May I include it in 7.0?

My only thought was that last_source_visited is really only useful
to print_source_lines_base, and it's used as a mechanism to prevent
a warning from being printed every time we try to print source lines
for the same file.  So I thought it might be cleaner if that global
was made static to print_source_lines_base.  However, this wouldn't
work as is, as it turns out this is also used as a cache for source
file lookup! I'm guessing this is only a side-effect of the original
intent.

I believe it would be cleaner to adjust the use of that variable
so as to make it static to print_source_lines_base, but this can
be addressed independently of your patch is someone is interested.
In the meantime, I think your patch is progress, and it seems fine
to put it in 7.0 as well.

> 2009-09-22  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	gdb/
> 	* source.c (forget_cached_source_info): Clear last_source_visited.
> 	(init_last_source_visited): Delete.
> 	(directory_command): Do not clear last_source_visited.  Call
> 	forget_cached_source_info only if required.
> 	(unset_substitute_path_command, set_substitute_path_command): Call
> 	forget_cached_source_info.
> 	* mi/mi-cmd-env.c (mi_cmd_env_dir): Do not call
> 	init_last_source_visited.
> 	* defs.h (init_last_source_visited): Delete declaration.
-- 
Joel

Re: [RFC] Fix source path lookup immediately after substitute-path

[Daniel Jacobowitz]

On Tue, Sep 22, 2009 at 02:20:42PM -0700, Joel Brobecker wrote:
> Hi Daniel,
> 
> > Any thoughts on this patch?  May I include it in 7.0?
> 
> My only thought was that last_source_visited is really only useful
> to print_source_lines_base, and it's used as a mechanism to prevent
> a warning from being printed every time we try to print source lines
> for the same file.  So I thought it might be cleaner if that global
> was made static to print_source_lines_base.  However, this wouldn't
> work as is, as it turns out this is also used as a cache for source
> file lookup! I'm guessing this is only a side-effect of the original
> intent.
> 
> I believe it would be cleaner to adjust the use of that variable
> so as to make it static to print_source_lines_base, but this can
> be addressed independently of your patch is someone is interested.
> In the meantime, I think your patch is progress, and it seems fine
> to put it in 7.0 as well.

last_source_visited is the direct cause of the problem I've fixed with
this patch.  External circumstances, like "dir" or "cd" or "set
substitute-path" have to be able to invalidate the cache, so it has to
be accessible outside the function.  Have I misunderstood?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Fix source path lookup immediately after substitute-path

[Joel Brobecker]

Before I answer your question, I just wanted to point out that I did
review the patch, and that it seemed good to me (progress, even) :).

> last_source_visited is the direct cause of the problem I've fixed with
> this patch.  External circumstances, like "dir" or "cd" or "set
> substitute-path" have to be able to invalidate the cache, so it has to
> be accessible outside the function.  Have I misunderstood?

The comments for last_source_visited are saying that this variable
is meant to help us avoid printing the file-does-not-exist error
message more than once if we repeateadly try to list that same file.
However, in practice, this variable is also used to cache the result
of the lookup. If we deem the caching action unnecessary, and limit
its use to exclusively preventing the error message from being printed
more than once, then this variable can be made static; it would be
easier to understand the purpose of this variable and follow its life
cycle.  That being said, the "if" here is whether the caching is
sufficiently desirable that we want to use that variable for both
aspects.

Again, this can be discussed seperately from your patch.


-- 
Joel

Re: [RFC] Fix source path lookup immediately after substitute-path

[Daniel Jacobowitz]

On Tue, Sep 22, 2009 at 03:03:02PM -0700, Joel Brobecker wrote:
> Before I answer your question, I just wanted to point out that I did
> review the patch, and that it seemed good to me (progress, even) :).

Yes, thank you :-)

> > last_source_visited is the direct cause of the problem I've fixed with
> > this patch.  External circumstances, like "dir" or "cd" or "set
> > substitute-path" have to be able to invalidate the cache, so it has to
> > be accessible outside the function.  Have I misunderstood?
> 
> The comments for last_source_visited are saying that this variable
> is meant to help us avoid printing the file-does-not-exist error
> message more than once if we repeateadly try to list that same file.
> However, in practice, this variable is also used to cache the result
> of the lookup.

Oh, I understand now.  In the specific case of a failed lookup, it's
used as a cache.  It's never used to cache a successful lookup - if
!last_source_error, we'll always call openp again.

I agree that this is not particularly important either way.  Thanks
for explaining!

-- 
Daniel Jacobowitz
CodeSourcery