[patch] Fix avoiding of unnecessary psymtabs in dwarf_decode_lines

[patch] Fix avoiding of unnecessary psymtabs in dwarf_decode_lines

[Doug Evans]

Hi.

dwarf2read.c:dwarf_decode_lines goes to some effort to avoid
calling dwarf2_create_include_psymtab for the main file.
It compares the path of the include file with the path of pst->filename
and if they are identical it does not call dwarf2_create_include_psymtab.
The current test is incomplete however.

Here's what I'm seeing (amd64-linux):

bash$ cd /tmp
bash$ cat hello.c
#include <stdio.h>

int
main ()
{
  printf ("Hello.\n");
  return 0;
}
bash$ gcc -g hello.c # gcc version = 4.5.0
bash$ gdb a.out
GNU gdb (GDB) 7.2.50.20100824-cvs
[...]
(gdb) maint info psymtab
{ objfile /tmp/a.out ((struct objfile *) 0xb32da0)
  { psymtab elf-init.c ((struct partial_symtab *) 0xb3f3d0)
    readin no
    fullname (null)
    text addresses 0x0 -- 0x0
    globals (none)
    statics (none)
    dependencies {
      psymtab elf-init.c ((struct partial_symtab *) 0xb3f2b0)
    }
  }
  { psymtab elf-init.c ((struct partial_symtab *) 0xb3f2b0)
    readin no
    fullname (null)
    text addresses 0x400540 -- 0x4005d9
    globals (* (struct partial_symbol **) 0xb3e080 @ 2)
    statics (* (struct partial_symbol **) 0xb3e440 @ 5)
    dependencies (none)
  }
  { psymtab hello.c ((struct partial_symtab *) 0xb3f220)
    readin no
    fullname (null)
    text addresses 0x0 -- 0x0
    globals (none)
    statics (none)
    dependencies {
      psymtab hello.c ((struct partial_symtab *) 0xb3f198)
    }
  }
  { psymtab hello.c ((struct partial_symtab *) 0xb3f198)
    readin no
    fullname (null)
    text addresses 0x400524 -- 0x400539
    globals (* (struct partial_symbol **) 0xb3e078 @ 1)
    statics (* (struct partial_symbol **) 0xb3e3f8 @ 9)
    dependencies (none)
  }
  { psymtab init.c ((struct partial_symtab *) 0xb3f080)
    readin no
    fullname (null)
    text addresses 0x400474 -- 0x400474
    globals (* (struct partial_symbol **) 0xb3e070 @ 1)
    statics (* (struct partial_symbol **) 0xb3e3b0 @ 9)
    dependencies (none)
  }
  { psymtab ../sysdeps/x86_64/elf/start.S ((struct partial_symtab *) 0xb3eec0)
    readin no
    fullname (null)
    text addresses 0x0 -- 0x0
    globals (none)
    statics (none)
    dependencies {
      psymtab ../sysdeps/x86_64/elf/start.S ((struct partial_symtab *) 0xb3ec08)
    }
  }
  { psymtab ../sysdeps/x86_64/elf/start.S ((struct partial_symtab *) 0xb3ec08)
    readin no
    fullname (null)
    text addresses 0x4003f0 -- 0x400473
    globals (none)
    statics (none)
    dependencies (none)
  }
}
(gdb) 

Notice the duplicates.

This is because when we get to this strcmp in dwarf_decode_lines:

            if (strcmp (include_name, pst_filename) != 0)
              dwarf2_create_include_psymtab (include_name, pst, objfile);

include_name is, e.g. hello.c, and pst_filename is, e.g., /tmp/hello.c.

Having noticed failures in mb-inline.exp on my first attempt :-),
the name one records in the psymtab has to match the name recorded in the
symtab, and the name recorded in the symtab doesn't have DW_AT_comp_dir.
But if we are to compare the paths using full path names we need to
include DW_AT_comp_dir in the comparison.

This patch moves the test of whether to call dwarf2_create_include_psymtab
to a separate function, and enhances the test to include DW_AT_comp_dir
when doing the comparison.

regression tested on amd64-linux.

I will check this in in a few days if there are no objections.

2010-08-24  Doug Evans  <dje@google.com>

	* dwarf2read.c (dwarf2_build_include_psymtabs): Remove unnecessary
	forward decl.  Pass pst->dirname to dwarf_decode_lines.
	(psymtab_include_file_name): New function.
	(dwarf_decode_lines): Call it.  Update comments.

Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.441
diff -u -p -r1.441 dwarf2read.c
--- dwarf2read.c	23 Aug 2010 21:49:26 -0000	1.441
+++ dwarf2read.c	24 Aug 2010 22:06:49 -0000
@@ -873,10 +873,6 @@ static void dwarf2_locate_sections (bfd 
 static void dwarf2_create_include_psymtab (char *, struct partial_symtab *,
                                            struct objfile *);
 
-static void dwarf2_build_include_psymtabs (struct dwarf2_cu *,
-                                           struct die_info *,
-                                           struct partial_symtab *);
-
 static void dwarf2_build_psymtabs_hard (struct objfile *);
 
 static void scan_partial_symbols (struct partial_die_info *,
@@ -2757,7 +2753,8 @@ dwarf2_build_include_psymtabs (struct dw
   if (lh == NULL)
     return;  /* No linetable, so no includes.  */
 
-  dwarf_decode_lines (lh, NULL, abfd, cu, pst);
+  /* NOTE: pst->dirname is DW_AT_comp_dir (if present).  */
+  dwarf_decode_lines (lh, pst->dirname, abfd, cu, pst);
 
   free_line_header (lh);
 }
@@ -9892,23 +9889,104 @@ check_cu_functions (CORE_ADDR address, s
   return fn->lowpc;
 }
 
+/* Subroutine of dwarf_decode_lines to simplify it.
+   Return the file name of the psymtab for included file FILE_INDEX
+   in line header LH of PST.
+   COMP_DIR is the compilation directory (DW_AT_comp_dir) or NULL if unknown.
+   If space for the result is malloc'd, it will be freed by a cleanup.
+   Returns NULL if FILE_INDEX should be ignored, i.e., it is pst->filename.  */
+
+static char *
+psymtab_include_file_name (const struct line_header *lh, int file_index,
+			   const struct partial_symtab *pst,
+			   const char *comp_dir)
+{
+  const struct file_entry fe = lh->file_names [file_index];
+  char *include_name = fe.name;
+  char *include_name_to_compare = include_name;
+  char *dir_name = NULL;
+  char *pst_filename;
+  int file_is_pst;
+
+  if (fe.dir_index)
+    dir_name = lh->include_dirs[fe.dir_index - 1];
+
+  if (!IS_ABSOLUTE_PATH (include_name)
+      && (dir_name != NULL || comp_dir != NULL))
+    {
+      /* Avoid creating a duplicate psymtab for PST.
+	 We do this by comparing INCLUDE_NAME and PST_FILENAME.
+	 Before we do the comparison, however, we need to account
+	 for DIR_NAME and COMP_DIR.
+	 First prepend dir_name (if non-NULL).  If we still don't
+	 have an absolute path prepend comp_dir (if non-NULL).
+	 However, the directory we record in the include-file's
+	 psymtab does not contain COMP_DIR (to match the
+	 corresponding symtab(s)).
+
+	 Example:
+
+	 bash$ cd /tmp
+	 bash$ gcc -g ./hello.c
+	 include_name = "hello.c"
+	 dir_name = "."
+	 DW_AT_comp_dir = comp_dir = "/tmp"
+	 DW_AT_name = "./hello.c"  */
+
+      if (dir_name != NULL)
+	{
+	  include_name = concat (dir_name, SLASH_STRING,
+				 include_name, (char *)NULL);
+	  include_name_to_compare = include_name;
+	  make_cleanup (xfree, include_name);
+	}
+      if (!IS_ABSOLUTE_PATH (include_name) && comp_dir != NULL)
+	{
+	  include_name_to_compare = concat (comp_dir, SLASH_STRING,
+					    include_name, (char *)NULL);
+	}
+    }
+
+  pst_filename = pst->filename;
+  if (!IS_ABSOLUTE_PATH (pst_filename) && pst->dirname != NULL)
+    {
+      pst_filename = concat (pst->dirname, SLASH_STRING,
+			     pst_filename, (char *)NULL);
+    }
+
+  file_is_pst = strcmp (include_name_to_compare, pst_filename) == 0;
+
+  if (include_name_to_compare != include_name)
+    xfree (include_name_to_compare);
+  if (pst_filename != pst->filename)
+    xfree (pst_filename);
+
+  if (file_is_pst)
+    return NULL;
+  return include_name;
+}
+
 /* Decode the Line Number Program (LNP) for the given line_header
    structure and CU.  The actual information extracted and the type
    of structures created from the LNP depends on the value of PST.
 
    1. If PST is NULL, then this procedure uses the data from the program
       to create all necessary symbol tables, and their linetables.
-      The compilation directory of the file is passed in COMP_DIR,
-      and must not be NULL.
 
    2. If PST is not NULL, this procedure reads the program to determine
       the list of files included by the unit represented by PST, and
-      builds all the associated partial symbol tables.  In this case,
-      the value of COMP_DIR is ignored, and can thus be NULL (the COMP_DIR
-      is not used to compute the full name of the symtab, and therefore
-      omitting it when building the partial symtab does not introduce
-      the potential for inconsistency - a partial symtab and its associated
-      symbtab having a different fullname -).  */
+      builds all the associated partial symbol tables.
+
+   COMP_DIR is the compilation directory (DW_AT_comp_dir) or NULL if unknown.
+   It is used for relative paths in the line table.
+   NOTE: When processing partial symtabs (pst != NULL),
+   comp_dir == pst->dirname.
+
+   NOTE: It is important that psymtabs have the same file name (via strcmp)
+   as the corresponding symtab.  Since COMP_DIR is not used in the name of the
+   symtab we don't use it in the name of the psymtabs we create.
+   E.g. expand_line_sal requires this when finding psymtabs to expand.
+   A good testcase for this is mb-inline.exp.  */
 
 static void
 dwarf_decode_lines (struct line_header *lh, char *comp_dir, bfd *abfd,
@@ -10191,29 +10269,9 @@ dwarf_decode_lines (struct line_header *
       for (file_index = 0; file_index < lh->num_file_names; file_index++)
         if (lh->file_names[file_index].included_p == 1)
           {
-            const struct file_entry fe = lh->file_names [file_index];
-            char *include_name = fe.name;
-            char *dir_name = NULL;
-            char *pst_filename = pst->filename;
-
-            if (fe.dir_index)
-              dir_name = lh->include_dirs[fe.dir_index - 1];
-
-            if (!IS_ABSOLUTE_PATH (include_name) && dir_name != NULL)
-              {
-                include_name = concat (dir_name, SLASH_STRING,
-				       include_name, (char *)NULL);
-                make_cleanup (xfree, include_name);
-              }
-
-            if (!IS_ABSOLUTE_PATH (pst_filename) && pst->dirname != NULL)
-              {
-                pst_filename = concat (pst->dirname, SLASH_STRING,
-				       pst_filename, (char *)NULL);
-                make_cleanup (xfree, pst_filename);
-              }
-
-            if (strcmp (include_name, pst_filename) != 0)
+	    char *include_name =
+	      psymtab_include_file_name (lh, file_index, pst, comp_dir);
+	    if (include_name != NULL)
               dwarf2_create_include_psymtab (include_name, pst, objfile);
           }
     }

Re: [patch] Fix avoiding of unnecessary psymtabs in dwarf_decode_lines

[Joel Brobecker]

[I am soooo far behind on gdb patches :-(]

> +  file_is_pst = strcmp (include_name_to_compare, pst_filename) == 0;

I know that the original code was already using strcmp to do the
comparison, but shouldn't we be using FILENAME_CMP instead.  That should
take care of platforms where the filesystem is case insensitive...

Since the patch was sent so long ago, I can make the change and do
the testing, if people agree.

-- 
Joel

[RFA/commit] replace strcmp by FILENAME_CMP for filename comparison

[Joel Brobecker]

This is a change that I suggested in a (very late) comment:
    http://www.sourceware.org/ml/gdb-patches/2010-08/msg00427.html
    http://www.sourceware.org/ml/gdb-patches/2010-10/msg00363.html

gdb/ChangeLog:

        * dwarf2read.c (psymtab_include_file_name): Replace call to strcmp
        by call to FILENAME_CMP.

Tested on x86_64-linux. Does it look OK?

Thanks,
-- 
Joel

---
 gdb/dwarf2read.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 7c78454..e613d90 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -10087,7 +10087,7 @@ psymtab_include_file_name (const struct line_header *lh, int file_index,
       pst_filename = copied_name;
     }
 
-  file_is_pst = strcmp (include_name_to_compare, pst_filename) == 0;
+  file_is_pst = FILENAME_CMP (include_name_to_compare, pst_filename) == 0;
 
   if (include_name_to_compare != include_name)
     xfree (include_name_to_compare);
-- 
1.7.1

Re: [patch] Fix avoiding of unnecessary psymtabs in dwarf_decode_lines

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> I know that the original code was already using strcmp to do the
Joel> comparison, but shouldn't we be using FILENAME_CMP instead.  That should
Joel> take care of platforms where the filesystem is case insensitive...

Joel> Since the patch was sent so long ago, I can make the change and do
Joel> the testing, if people agree.

It seems reasonable to me.

Tom

Re: [patch] Fix avoiding of unnecessary psymtabs in dwarf_decode_lines

[Eli Zaretskii]

> Date: Tue, 26 Oct 2010 09:39:28 -0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> [I am soooo far behind on gdb patches :-(]
> 
> > +  file_is_pst = strcmp (include_name_to_compare, pst_filename) == 0;
> 
> I know that the original code was already using strcmp to do the
> comparison, but shouldn't we be using FILENAME_CMP instead.  That should
> take care of platforms where the filesystem is case insensitive...

Yes, definitely.  Thanks for catching this.

Re: [RFA/commit] replace strcmp by FILENAME_CMP for filename comparison

[Joel Brobecker]

> gdb/ChangeLog:
> 
>         * dwarf2read.c (psymtab_include_file_name): Replace call to strcmp
>         by call to FILENAME_CMP.
> 
> Tested on x86_64-linux. Does it look OK?

Given the agreement on this (thanks!), I just checked this patch in.

-- 
Joel