Re: [Patch] Improve path lookup of absolute source file

Re: [Patch] Improve path lookup of absolute source file

[Francois Rigault]

Eli Zaretskii <eliz@gnu.org> wrote on 24/04/2009 17:11:50:

> > Cc: gdb-patches@sourceware.org, Daniel Jacobowitz <drow@false.
> org>,         Joel Brobecker <brobecker@gmail.com>
> > From: Tom Tromey <tromey@redhat.com>
> > Date: Fri, 24 Apr 2009 08:52:17 -0600
> > 
> > Do you have a copyright assignment on file?  This patch is big enough
> > to require one.
> 
> Today's copyright.list doesn't seem to show Francois's name, FWIW.

No need to consider copyright, although this patch "looks" big it is
actually just a simple refactoring. I'll be happy without my name
inside copyright.list file.

F

Re: [Patch] Improve path lookup of absolute source file

[Tom Tromey]

>>>>> "Francois" == Francois Rigault <francois.rigault@amadeus.com> writes:

>> > Do you have a copyright assignment on file?  This patch is big enough
>> > to require one.
>> 
>> Today's copyright.list doesn't seem to show Francois's name, FWIW.

Francois> No need to consider copyright, although this patch "looks" big it is
Francois> actually just a simple refactoring. I'll be happy without my name
Francois> inside copyright.list file.

Unfortunately it isn't just a matter of attribution or something like
that.  The FSF requires us to get assignment paperwork for any patch
over a certain size.

If you are willing to do this I can get you started on taht.

Tom

Re: [Patch] Improve path lookup of absolute source file

[Francois Rigault]

Daniel Jacobowitz <drow@false.org> wrote on 26/09/2008 15:01:55:
> I'm nervous about "unlikely".  What happens before and after if they
> do have different basenames?  e.g. a symlink foo.c to foo_x86.c; if
> GDB or GCC resolves the symlink at some point we'll mismatch and now
> completely fail to locate the file.

Hi,

would you accept a patch that uses a macro somewhere so that
users can choose to compile gdb with or without the modified lookup 
routine ? 

Francois

Re: [Patch] Improve path lookup of absolute source file

[Joel Brobecker]

> would you accept a patch that uses a macro somewhere so that users can
> choose to compile gdb with or without the modified lookup routine ? 

Not sure what the other maintainers think, but for the type of change
that you're suggestion, we generally don't. This tends to complicate
the code, and can also confuse uses as to why the same version of GDB
would "work" in one case and "not work" in the other. There's also
the question of testing if your change is disabled by default.

A softer approach would be based on using a "set ..." command to
determine one approach or the other. But I am not sure it would make
sense in this case, as my understanding is that the setting would
allow the user to switch between two approaches that are similar,
but with different holes in them.

-- 
Joel

Re: [Patch] Improve path lookup of absolute source file

[Francois Rigault]

Hello

I think we can come up with a solution that does not
introduce the holes from the previous one.

In the patch below, the file name lookup is done in 2 times :

The first pass will try to resolve the file name only if basenames 
match. The second pass will try to resolve the file name as before,
resolving all the files path from the symtab.

This guarantee that the files will be found as before even if the 
basenames differ. Also, there is a strong probability that the lookup 
will end after the first pass, which is a huge time saver when the 
lookup is done on a slow IO file system.

In the patch below, the lookup routine has been put in a new
function, and is called 2 times (one in each passes). The lookup
routine itself has been optimized so that psymtab_to_fullname
is not called twice both for full_path lookup and real_path lookup.

Here, our binary being compiled with all the sources on NFS,
this approach just suppress the 30s freeze when putting the first
breakpoint.

Please see the patch against CVS head below.

Best regards,

Francois

----------


--- gdb/symtab.c        2009-03-13 22:02:58.000000000 +0100
+++ gdb/symtab.c        2009-03-17 15:06:28.000011000 +0100
@@ -110,6 +110,10 @@ struct symbol *lookup_symbol_aux_psymtab
 
 static int file_matches (char *, char **, int);
 
+static int
+file_matches_symbol(const char *full_path, const char *real_path,
+                    struct partial_symtab *pst);
+
 static void print_symbol_info (domain_enum,
                               struct symtab *, struct symbol *, int, char 
*);
 
@@ -251,6 +255,45 @@ got_symtab:
   goto got_symtab;
 }
 
+/* Check if a file identified by its full_path (possibly NULL) and
+   its real_path (possibly NULL) matches the given symbol.  */
+
+static int
+file_matches_symbol(const char *full_path, const char *real_path,
+                    struct partial_symtab *pst)
+{
+  if ( (full_path != NULL) || (real_path != NULL) )
+    {
+      psymtab_to_fullname(pst);
+    }
+
+
+  if (full_path != NULL)
+    {
+      if (pst->fullname != NULL
+          && FILENAME_CMP (full_path, pst->fullname) == 0)
+        {
+          return 1;
+        }
+    }
+
+  if (real_path != NULL)
+    {
+      char *rp = NULL;
+      if (pst->fullname != NULL)
+        {
+          rp = gdb_realpath (pst->fullname);
+          make_cleanup (xfree, rp);
+        }
+      if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
+        {
+          return 1;
+        }
+    }
+
+  return 0;
+}
+
 /* Lookup the partial symbol table of a source file named NAME.
    *If* there is no '/' in the name, a match after a '/'
    in the psymtab filename will also work.  */
@@ -273,6 +316,12 @@ lookup_partial_symtab (const char *name)
       make_cleanup (xfree, real_path);
     }
 
+  /* If the user gave us an absolute path, try to find the file in
+     this symtab and use its absolute path.  */
+
+  /* Do a first quick pass where we try to match symbol and files
+     using their names.  This phase is inexpensive in IOs, and will
+     match for most of the symbols.  */
   ALL_PSYMTABS (objfile, pst)
   {
     if (FILENAME_CMP (name, pst->filename) == 0)
@@ -280,31 +329,23 @@ lookup_partial_symtab (const char *name)
        return (pst);
       }
 
-    /* If the user gave us an absolute path, try to find the file in
-       this symtab and use its absolute path.  */
-    if (full_path != NULL)
+    if (FILENAME_CMP (lbasename(name), lbasename(pst->filename)) == 0)
       {
-       psymtab_to_fullname (pst);
-       if (pst->fullname != NULL
-           && FILENAME_CMP (full_path, pst->fullname) == 0)
-         {
-           return pst;
-         }
+        if (file_matches_symbol(full_path, real_path, pst))
+          {
+            return (pst);
+          }
       }
+  }
 
-    if (real_path != NULL)
+  /* Do a second pass. Here we will try to match the absolute file
+     path with the absolute file path from the symtab. Since there can
+     be a lot of files in the symtab, this pass is expensive in IOs. */
+  ALL_PSYMTABS (objfile, pst)
+  {
+    if (file_matches_symbol(full_path, real_path, pst))
       {
-        char *rp = NULL;
-       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)
-         {
-           return pst;
-         }
+        return (pst);
       }
   }

Re: [Patch] Improve path lookup of absolute source file

[Tom Tromey]

>>>>> "Francois" == Francois Rigault <francois.rigault@amadeus.com> writes:

This particular problem has 3 pending patches... the one from this
thread, and also:

http://sourceware.org/ml/gdb-patches/2008-06/msg00027.html
http://sourceware.org/ml/gdb-patches/2009-01/msg00325.html

Of these I prefer the approach taken by the patch I'm replying to,
because it solves the symlink problem.  The first patch also changes
lookup_symtab, so I'm not sure whether the patch I'm responding to is
sufficient.

Francois> In the patch below, the file name lookup is done in 2 times :
Francois> The first pass will try to resolve the file name only if basenames 
Francois> match. The second pass will try to resolve the file name as before,
Francois> resolving all the files path from the symtab.

This sounds reasonable to me.  It may change the order in which files
are found, but I don't think we provide any guarantees about that.


Do you have a copyright assignment on file?  This patch is big enough
to require one.

The patch itself looks reasonable, though it has a number of
formatting nits to pick.  We can work these out once the paperwork
issue is dealt with.

Alternatively, if one of the other patch submitters wants to rewrite
his patch to take the 2-pass approach, that would be fine too.

It seems to me that the 2-pass approach means that a typo in the
command will still make gdb pause for a long time while it searches
all the psymtabs (and fails).  Perhaps the loop needs a QUIT?  I don't
know if that is safe here or not, though the calls to make_cleanup
make me think that is probably is.

Tom

Re: [Patch] Improve path lookup of absolute source file

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org, Daniel Jacobowitz <drow@false.org>,         Joel Brobecker <brobecker@gmail.com>
> From: Tom Tromey <tromey@redhat.com>
> Date: Fri, 24 Apr 2009 08:52:17 -0600
> 
> Do you have a copyright assignment on file?  This patch is big enough
> to require one.

Today's copyright.list doesn't seem to show Francois's name, FWIW.

[Patch] Improve path lookup of absolute source file

[Francois Rigault]

When setting breakpoints on a source file using an absolute path,
lookup_symtab will try to match the target source file path with each
symbol file path. For each of these paths, the realpath function is
called, potentially leading to a big number of IOs.

On a slow IO filesystem, like a network file system, this can slow down
the performances. gdb takes here 15s to complete the setting of the
first breakpoint on an absolute source file path.

In order to let gdb find the files without generating IOs, a simple
trick is to check that symbol source file and target source file have
the same basename, as source file used at compilation time and the one
used for debugging are unlikely to have different basenames. See the
patch below against gdb-6.8.

Regards




*** gdb/symtab.c        Thu Sep 11 11:10:13 2008
--- gdb/symtab.c        Thu Sep 11 11:31:46 2008
*************** lookup_partial_symtab (const char *name)
*** 259,264 ****
--- 259,270 ----
        return (pst);
        }
 
+     /* Skip this symbol if basenames differ. */
+     if (FILENAME_CMP (lbasename(name), lbasename(pst->filename)) != 0)
+       {
+       continue;
+       } 
+ 
      /* If the user gave us an absolute path, try to find the file in
         this symtab and use its absolute path.  */
      if (full_path != NULL)

Re: [Patch] Improve path lookup of absolute source file

[Thiago Jung Bauermann]

Hi,

Francois Rigault schrieb:
> On a slow IO filesystem, like a network file system, this can slow down
> the performances. gdb takes here 15s to complete the setting of the
> first breakpoint on an absolute source file path.
> 
> In order to let gdb find the files without generating IOs, a simple
> trick is to check that symbol source file and target source file have
> the same basename, as source file used at compilation time and the one
> used for debugging are unlikely to have different basenames. See the
> patch below against gdb-6.8.

This patch is a good idea, thanks. Some comments:

We need it against current CVS HEAD of GDB. Also, could you run the 
testsuite before and after applying your patch, to be sure it introduces 
no regression? It doesn't seem to  me it will, but this is good practice 
anyway.

Also, some nits regarding formatting, since GDB uses the GNU Coding 
Standard:

> *** gdb/symtab.c        Thu Sep 11 11:10:13 2008
> --- gdb/symtab.c        Thu Sep 11 11:31:46 2008
> *************** lookup_partial_symtab (const char *name)
> *** 259,264 ****
> --- 259,270 ----
>         return (pst);
>         }
>  
> +     /* Skip this symbol if basenames differ. */

s/symbol/symbol table/
Also, you need two spaces after the full stop.

> +     if (FILENAME_CMP (lbasename(name), lbasename(pst->filename)) != 0)

There should be a space between the function name and the parenthesis.

> +       {
> +       continue;
> +       } 

You don't need the curly braces here, since there's only one statement 
inside the if.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [Patch] Improve path lookup of absolute source file

[Daniel Jacobowitz]

On Thu, Sep 11, 2008 at 02:47:09PM +0200, Francois Rigault wrote:
> In order to let gdb find the files without generating IOs, a simple
> trick is to check that symbol source file and target source file have
> the same basename, as source file used at compilation time and the one
> used for debugging are unlikely to have different basenames. See the
> patch below against gdb-6.8.

I'm nervous about "unlikely".  What happens before and after if they
do have different basenames?  e.g. a symlink foo.c to foo_x86.c; if
GDB or GCC resolves the symlink at some point we'll mismatch and now
completely fail to locate the file.

-- 
Daniel Jacobowitz
CodeSourcery