Re: [RFC] Avoid calling gdb_realpath if basenames are different

[asmwarrior <asmwarrior at gmail dot com>]

On 2011-11-6 14:30, Doug Evans wrote:
> Hi.
> This patch has been brought up before (by others).
> E.g., http://sourceware.org/ml/gdb-patches/2010-04/msg00466.html
> I'm hoping we can get this in now.
> We're paying a real and significant cost for what is mostly a
> theoretical concern.
> [E.g., How often is one source file referred to by the user using a basename
> that is different than what's recorded in the debug info?]
> 
> If people are concerned about breaking someone's usage,
> we could default basenames-may-differ to true in 7.4,
> with a warning that it will be set to false in 7.5 (or some such).
> [We could leave the default set to true, especially if someone knew
> of at least some minimally common usage this would break.
> I'd hate to otherwise penalize the vast majority of users if not.]
> 
> I'll write docs,NEWS if the basics of the patch are approved.
> 
> Comments?
> 
> 2011-11-05  Doug Evans<dje@google.com>
> 
> 	* dwarf2read.c (dw2_lookup_symtab): Avoid calling gdb_realpath if
> 	! basenames_may_differ.
> 	* psymtab.c (lookup_partial_symtab): Ditto.
> 	* symtab.c (lookup_partial_symtab): Ditto.
> 	* symtab.c (lookup_symtab): Ditto.
> 	(basenames_may_differ): New global.
> 	(_initialize_symtab): New parameter basenames-may-differ.
> 	* symtab.h (basenames_may_differ): Declare.
> 
> Index: dwarf2read.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/dwarf2read.c,v
> retrieving revision 1.578
> diff -u -p -r1.578 dwarf2read.c
> --- dwarf2read.c	20 Oct 2011 23:13:01 -0000	1.578
> +++ dwarf2read.c	6 Nov 2011 06:25:11 -0000
> @@ -2444,7 +2444,8 @@ dw2_lookup_symtab (struct objfile *objfi
>   		   struct symtab **result)
>   {
>     int i;
> -  int check_basename = lbasename (name) == name;
> +  const char *name_basename = lbasename (name);
> +  int check_basename = name_basename == name;
>     struct dwarf2_per_cu_data *base_cu = NULL;
> 
>     dw2_setup (objfile);
> @@ -2477,6 +2478,12 @@ dw2_lookup_symtab (struct objfile *objfi
>   	&&  FILENAME_CMP (lbasename (this_name), name) == 0)
>   	    base_cu = per_cu;
> 
> +	  /* Before we invoke realpath, which can get expensive when many
> +	     files are involved, do a quick comparison of the basenames.  */
> +	  if (! basenames_may_differ
> +	&&  FILENAME_CMP (lbasename (this_name), name_basename) != 0)
> +	    continue;
> +
>   	  if (full_path != NULL)
>   	    {
>   	      const char *this_real_name = dw2_get_real_path (objfile,
> Index: psymtab.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/psymtab.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 psymtab.c
> --- psymtab.c	28 Oct 2011 17:29:37 -0000	1.31
> +++ psymtab.c	6 Nov 2011 06:25:11 -0000
> @@ -134,6 +134,7 @@ lookup_partial_symtab (struct objfile *o
>   		       const char *full_path, const char *real_path)
>   {
>     struct partial_symtab *pst;
> +  const char *name_basename = lbasename (name);
> 
>     ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
>     {
> @@ -142,6 +143,12 @@ lookup_partial_symtab (struct objfile *o
>   	return (pst);
>         }
> 
> +    /* Before we invoke realpath, which can get expensive when many
> +       files are involved, do a quick comparison of the basenames.  */
> +    if (! basenames_may_differ
> +	&&  FILENAME_CMP (name_basename, 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)
> @@ -172,7 +179,7 @@ lookup_partial_symtab (struct objfile *o
> 
>     /* Now, search for a matching tail (only if name doesn't have any dirs).  */
> 
> -  if (lbasename (name) == name)
> +  if (name_basename == name)
>       ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, pst)
>       {
>         if (FILENAME_CMP (lbasename (pst->filename), name) == 0)
> Index: symtab.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symtab.c,v
> retrieving revision 1.285
> diff -u -p -r1.285 symtab.c
> --- symtab.c	29 Oct 2011 07:26:07 -0000	1.285
> +++ symtab.c	6 Nov 2011 06:25:11 -0000
> @@ -112,6 +112,8 @@ void _initialize_symtab (void);
> 
>   /* */
> 
> +int basenames_may_differ = 1;
> +
>   /* Allow the user to configure the debugger behavior with respect
>      to multiple-choice menus when more than one symbol matches during
>      a symbol lookup.  */
> @@ -155,6 +157,7 @@ lookup_symtab (const char *name)
>     char *real_path = NULL;
>     char *full_path = NULL;
>     struct cleanup *cleanup;
> +  const char* base_name = lbasename (name);
> 
>     cleanup = make_cleanup (null_cleanup, NULL);
> 
> @@ -180,6 +183,12 @@ got_symtab:
>   	return s;
>         }
> 
> +    /* Before we invoke realpath, which can get expensive when many
> +       files are involved, do a quick comparison of the basenames.  */
> +    if (! basenames_may_differ
> +	&&  FILENAME_CMP (base_name, lbasename (s->filename)) != 0)
> +      continue;
> +
>       /* If the user gave us an absolute path, try to find the file in
>          this symtab and use its absolute path.  */
> 
> @@ -4883,5 +4892,20 @@ Show how the debugger handles ambiguitie
>   Valid values are \"ask\", \"all\", \"cancel\", and the default is \"all\"."),
>                           NULL, NULL,&setlist,&showlist);
> 
> +  add_setshow_boolean_cmd ("basenames-may-differ", class_files,
> +			&basenames_may_differ, _("\
> +Set whether a source file may have multiple base names."), _("\
> +Show whether a source file may have multiple names."), _("\
> +When doing file name based lookups, gdb will canonicalize file names\n\
> +(e.g., expand symlinks) before comparing them, which is an expensive\n\
> +operation.\n\
> +If set, gdb cannot assume a file is known by one base name, and thus\n\
> +it cannot optimize file name comparisions by skipping the canonicalization\n\
> +step if the base names are different.\n\
> +If not set, all source files must be known by one base name,\n\
> +and gdb will do file name comparisons more efficiently."),
> +			   NULL, NULL,
> +			&setlist,&showlist);
> +
>     observer_attach_executable_changed (symtab_observer_executable_changed);
>   }
> Index: symtab.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/symtab.h,v
> retrieving revision 1.190
> diff -u -p -r1.190 symtab.h
> --- symtab.h	9 Oct 2011 19:34:18 -0000	1.190
> +++ symtab.h	6 Nov 2011 06:25:11 -0000
> @@ -1307,4 +1307,6 @@ void fixup_section (struct general_symbo
> 
>   struct objfile *lookup_objfile_from_block (const struct block *block);
> 
> +extern int basenames_may_differ;
> +
>   #endif /* !defined(SYMTAB_H) */
> 

Hi, Doug.
I have applied your patch in my Windows XP local copy of gdb source, and build the gdb under msys+mingw gcc.

also change: 

int basenames_may_differ = 0;

I think from my point, I don't have symbol link under windows XP of the source file.

I just test a large projects(which has many dlls loading when start up). I just set the breakpoint before start the app, so the breakpoint is just pending when the app starts.

Without your patch, I need more than 90 seconds to start the app. 
With your patch, It takes only about 15 seconds.

BTW: if I do not set any pending breakpoints before start the app, starting time is about about 15 seconds. Many of the windows gdb user have complain the time lag issue caused by pending breakpoints.

So, the conclusion is that your patches are great!!!, it avoid many unnecessary file IO operations under Windows XP.

asmwarrior
ollydbg from codeblocks' forum