[PATCH] Avoid most open and stat syscalls when setting a breakpoint

[PATCH] Avoid most open and stat syscalls when setting a breakpoint

[Martin.Runge]

Hi all,

I want to present a patch that helps avoiding many unneccessary filesystem 
accesses when setting breakpoints. This leads to a remarkable perfomance 
improvement when sources are on network shares.

The effect that gdb opens a lot of unneccessary files when setting a 
breakpoint via its absulute source path was already discussed here 
http://sourceware.org/ml/gdb/2010-03/msg00195.html.
gdb iterates over all source files mentioned in all object file's symtabs 
(== all source and header files used during the build). For each of these 
source files, gdb tries to find the file in the filesystem by applying the 
source path. The source path is used to debug in different locations than 
the build was done or to find the sources at all if the compiler does not 
include the compile dir in the debug infos.  If there is a hit, e,g, the 
source file from the debug info with one of the entries in soure path 
applied is there ( stat and open succeed ), then gdb checks if that is the 
file mentioned in the break command and if so, takes the code address from 
the symtab to set the breakpoint.

I think that most of these checks are not neccessary, e.g. to compare if

/home/me/project/source/main.cpp

becomes the same file as  <iostream>  when trying all source paths on 
main.cpp. The patch below leaves the described search loop when the 
basename from the symtab and the basename from the break command do not 
match, as they wont match by appliing different paths either.


diff -Naur gdb-7.1.orig/gdb/symtab.c gdb-7.1.mod/gdb/symtab.c
--- gdb-7.1.orig/gdb/symtab.c   2010-01-26 16:48:25.000000000 +0100
+++ gdb-7.1.mod/gdb/symtab.c    2010-03-24 16:54:37.000000000 +0100
@@ -280,7 +280,7 @@
 
    /* 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 (full_path != NULL && FILENAME_CMP( lbasename(full_path), 
lbasename(pst->filename)) == 0)
    {
        psymtab_to_fullname (pst);
        if (pst->fullname != NULL
@@ -290,7 +290,7 @@
        }
    }

-   if (real_path != NULL)
+   if (real_path != NULL && FILENAME_CMP( lbasename(real_path), 
lbasename(pst->filename)) == 0)
    { 
        char *rp = NULL;
        psymtab_to_fullname (pst);


please let me know what you think and if this is relevant for inclusion in 
future gdb releases.

best regards
Martin
 

Re: [PATCH] Avoid most open and stat syscalls when setting a breakpoint

[Tom Tromey]

>>>>> "Martin" == Martin Runge <Martin.Runge@rohde-schwarz.com> writes:

Martin> I want to present a patch that helps avoiding many unneccessary
Martin> filesystem accesses when setting breakpoints. This leads to a
Martin> remarkable perfomance improvement when sources are on network
Martin> shares.

Martin> I think that most of these checks are not neccessary, e.g. to
Martin> compare if
Martin> /home/me/project/source/main.cpp
Martin> becomes the same file as  <iostream>  when trying all source paths on 
Martin> main.cpp. The patch below leaves the described search loop when the 
Martin> basename from the symtab and the basename from the break command do not 
Martin> match, as they wont match by appliing different paths either.

ISTR seeing this same patch in the past.

I think the problem is that one of the file names might be a symlink.
The two names might refer to the same file even if the basenames differ.
That's what the gdb_realpath logic is for.

So, I think this patch is not correct, or at least, it changes gdb
incompatibly.  I don't know how much this symlink thing matters in
practice.  Maybe there is some other way to solve this problem, but I
haven't looked into it deeply.

Tom

Re: [PATCH] Avoid most open and stat syscalls when setting a breakpoint

[Joel Brobecker]

> Martin> I think that most of these checks are not neccessary, e.g. to
> Martin> compare if
> Martin> /home/me/project/source/main.cpp
> Martin> becomes the same file as  <iostream>  when trying all source paths on 
> Martin> main.cpp. The patch below leaves the described search loop when the 
> Martin> basename from the symtab and the basename from the break command do not 
> Martin> match, as they wont match by appliing different paths either.
> 
> ISTR seeing this same patch in the past.

Me too - I just got a little surprise today while looking at AdaCore's
diff, finding the exact type of patch in AdaCore's tree as well :-o.

> So, I think this patch is not correct, or at least, it changes gdb
> incompatibly.  I don't know how much this symlink thing matters in
> practice.  Maybe there is some other way to solve this problem, but I
> haven't looked into it deeply.

I agree on the conclusion - the change can be disruptive if the user
uses symlinks.

However, I am wondering if this is worth considering, given how slow
the IO on some filesystems can be (some users even use a remote FS,
which is even worse). But the compatibility is a real issue.

It's not trivial to do the archeology and try to figure out how long
we've had this change in AdaCore's tree, but I think it's been many
years, and I believe from customer feedback that this has made a
noticeable impact (we have some customers who are very sensitive to
filesystem access explosions).

I'm not sure how common (or uncommon) it is to use symlinks in source
trees, and I'm not willing to take a bet on this. Peharps the only way
forward is to has a user setting that allows us to turn the optimization
off in the case when there are symlinks?

(just thinking out loud)

-- 
Joel

Antwort: Re: [PATCH] Avoid most open and stat syscalls when setting a breakpoint

[Martin.Runge]

> > So, I think this patch is not correct, or at least, it changes gdb
> > incompatibly.  I don't know how much this symlink thing matters in
> > practice.  Maybe there is some other way to solve this problem, but I
> > haven't looked into it deeply.
>
> I agree on the conclusion - the change can be disruptive if the user
> uses symlinks.
> 
> However, I am wondering if this is worth considering, given how slow
> the IO on some filesystems can be (some users even use a remote FS,
> which is even worse). But the compatibility is a real issue.
> 

I agree that my patch breaks compatibility in case of symlinks. But I 
wonder if 
gdb can work correctly in all of these situations, anyway:

As gdb tries all source path entries, cwd and cdir applied to the given 
file and takes the 
first one that points to an existing file, this may easily fail in 
projects 
with duplicate filenames that only differ in their path. 

Second issue I see is the difference in how breakpoints are resolved when 
the source file is given with full path and just the filename. I 
am not sure if I understood the code completely, but I think the symlink 
case 
does not work when setting breakpoints without absolute path, either.

> It's not trivial to do the archeology and try to figure out how long
> we've had this change in AdaCore's tree, but I think it's been many
> years, and I believe from customer feedback that this has made a
> noticeable impact (we have some customers who are very sensitive to
> filesystem access explosions).
>
> I'm not sure how common (or uncommon) it is to use symlinks in source
> trees, and I'm not willing to take a bet on this. Peharps the only way
> forward is to has a user setting that allows us to turn the optimization
> off in the case when there are symlinks?
> 
> (just thinking out loud)

In frondends to gdb there is usually a small checkbox "set breakpoint with 
full sourcepath". 
When the user checks it he changes the behviour of the debugger to a much 
larger extend than 
he would expect.


Martin