FYI: fix latent bug in dw2_find_symbol_file

FYI: fix latent bug in dw2_find_symbol_file

[Tom Tromey]

I'm checking this in.

dw2_find_symbol_file has a latent bug.  It assumes that the last file in
file name table is the primary file name for the CU.

However, there's no reason this must be true.  The primary file name
could appear anywhere.

And, since this method returns just the file name, and not the directory
name, there is no reason to even bother reading the line number program
-- we can just use the CU DIE's DW_AT_name.

I don't have a test case for this; but the situation came up with the
dwz multifile code.

Built and regtested on x86-64 Fedora 16.

Tom

2012-06-11  Tom Tromey  <tromey@redhat.com>

	* dwarf2read.c (dw2_get_primary_filename_reader): New function.
	(dw2_find_symbol_file): Use it.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 589361e..7e25d08 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2836,12 +2836,34 @@ dw2_expand_symtabs_with_filename (struct objfile *objfile,
     }
 }
 
+/* A helper function for dw2_find_symbol_file that finds the primary
+   file name for a given CU.  This is a die_reader_func.  */
+
+static void
+dw2_get_primary_filename_reader (const struct die_reader_specs *reader,
+				 gdb_byte *info_ptr,
+				 struct die_info *comp_unit_die,
+				 int has_children,
+				 void *data)
+{
+  const char **result_ptr = data;
+  struct dwarf2_cu *cu = reader->cu;
+  struct attribute *attr;
+
+  attr = dwarf2_attr (comp_unit_die, DW_AT_name, cu);
+  if (attr == NULL)
+    *result_ptr = NULL;
+  else
+    *result_ptr = DW_STRING (attr);
+}
+
 static const char *
 dw2_find_symbol_file (struct objfile *objfile, const char *name)
 {
   struct dwarf2_per_cu_data *per_cu;
   offset_type *vec;
   struct quick_file_names *file_data;
+  const char *filename;
 
   dw2_setup (objfile);
 
@@ -2873,12 +2895,17 @@ dw2_find_symbol_file (struct objfile *objfile, const char *name)
   /* vec[0] is the length, which must always be >0.  */
   per_cu = dw2_get_cu (MAYBE_SWAP (vec[1]));
 
-  file_data = dw2_get_file_names (objfile, per_cu);
-  if (file_data == NULL
-      || file_data->num_file_names == 0)
-    return NULL;
+  if (per_cu->v.quick->symtab != NULL)
+    return per_cu->v.quick->symtab->filename;
+
+  if (per_cu->is_debug_types)
+    init_cutu_and_read_dies (per_cu, 0, 0, dw2_get_primary_filename_reader,
+			     &filename);
+  else
+    init_cutu_and_read_dies_simple (per_cu, dw2_get_primary_filename_reader,
+				    &filename);
 
-  return file_data->file_names[file_data->num_file_names - 1];
+  return filename;
 }
 
 static void

Re: FYI: fix latent bug in dw2_find_symbol_file

[Doug Evans]

On Mon, Jun 11, 2012 at 11:42 AM, Tom Tromey <tromey@redhat.com> wrote:
> I'm checking this in.
>
> dw2_find_symbol_file has a latent bug.  It assumes that the last file in
> file name table is the primary file name for the CU.
>
> However, there's no reason this must be true.  The primary file name
> could appear anywhere.
>
> And, since this method returns just the file name, and not the directory
> name, there is no reason to even bother reading the line number program
> -- we can just use the CU DIE's DW_AT_name.
>
> I don't have a test case for this; but the situation came up with the
> dwz multifile code.
>
> Built and regtested on x86-64 Fedora 16.
>
> Tom
>
> 2012-06-11  Tom Tromey  <tromey@redhat.com>
>
>        * dwarf2read.c (dw2_get_primary_filename_reader): New function.
>        (dw2_find_symbol_file): Use it.
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 589361e..7e25d08 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -2836,12 +2836,34 @@ dw2_expand_symtabs_with_filename (struct objfile *objfile,
>     }
>  }
>
> +/* A helper function for dw2_find_symbol_file that finds the primary
> +   file name for a given CU.  This is a die_reader_func.  */
> +
> +static void
> +dw2_get_primary_filename_reader (const struct die_reader_specs *reader,
> +                                gdb_byte *info_ptr,
> +                                struct die_info *comp_unit_die,
> +                                int has_children,
> +                                void *data)
> +{
> +  const char **result_ptr = data;
> +  struct dwarf2_cu *cu = reader->cu;
> +  struct attribute *attr;
> +
> +  attr = dwarf2_attr (comp_unit_die, DW_AT_name, cu);
> +  if (attr == NULL)
> +    *result_ptr = NULL;
> +  else
> +    *result_ptr = DW_STRING (attr);
> +}
> +
>  static const char *
>  dw2_find_symbol_file (struct objfile *objfile, const char *name)
>  {
>   struct dwarf2_per_cu_data *per_cu;
>   offset_type *vec;
>   struct quick_file_names *file_data;
> +  const char *filename;
>
>   dw2_setup (objfile);
>
> @@ -2873,12 +2895,17 @@ dw2_find_symbol_file (struct objfile *objfile, const char *name)
>   /* vec[0] is the length, which must always be >0.  */
>   per_cu = dw2_get_cu (MAYBE_SWAP (vec[1]));
>
> -  file_data = dw2_get_file_names (objfile, per_cu);
> -  if (file_data == NULL
> -      || file_data->num_file_names == 0)
> -    return NULL;
> +  if (per_cu->v.quick->symtab != NULL)
> +    return per_cu->v.quick->symtab->filename;
> +
> +  if (per_cu->is_debug_types)
> +    init_cutu_and_read_dies (per_cu, 0, 0, dw2_get_primary_filename_reader,
> +                            &filename);
> +  else
> +    init_cutu_and_read_dies_simple (per_cu, dw2_get_primary_filename_reader,
> +                                   &filename);
>
> -  return file_data->file_names[file_data->num_file_names - 1];
> +  return filename;
>  }
>
>  static void

You need to call init_cutu_and_read_dies in both debug-types and
non-debug-types cases.
When DWO files are in use DW_AT_name lives in the DWO file.
OTOH TUs typically don't have DW_AT_name (you need to look at DW_AT_decl_file).
OTOOH this function is only called by find_main_filename.

So what to do?

I'm tempted to rename the routine {,*_}find_function_file (or some
such) and have dw2_find_symbol_file ignore TUs.

Re: FYI: fix latent bug in dw2_find_symbol_file

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> You need to call init_cutu_and_read_dies in both debug-types and
Doug> non-debug-types cases.  When DWO files are in use DW_AT_name lives
Doug> in the DWO file.  OTOH TUs typically don't have DW_AT_name (you
Doug> need to look at DW_AT_decl_file).  OTOOH this function is only
Doug> called by find_main_filename.

What gcc branch should I use to test this?
And do I need gold?

Doug> I'm tempted to rename the routine {,*_}find_function_file (or some
Doug> such) and have dw2_find_symbol_file ignore TUs.

Fine by me, but I don't plan to do it.

Tom

Re: FYI: fix latent bug in dw2_find_symbol_file

[Doug Evans]

On Mon, Jun 11, 2012 at 1:57 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> You need to call init_cutu_and_read_dies in both debug-types and
> Doug> non-debug-types cases.  When DWO files are in use DW_AT_name lives
> Doug> in the DWO file.  OTOH TUs typically don't have DW_AT_name (you
> Doug> need to look at DW_AT_decl_file).  OTOOH this function is only
> Doug> called by find_main_filename.
>
> What gcc branch should I use to test this?
> And do I need gold?
>
> Doug> I'm tempted to rename the routine {,*_}find_function_file (or some
> Doug> such) and have dw2_find_symbol_file ignore TUs.
>
> Fine by me, but I don't plan to do it.

Well, there's only 5 or so places you need to change for the renaming. :-)

What I'm saying is the patch is wrong for TUs but TUs are irrelevant
here anyway.
What did you want to test?

Re: FYI: fix latent bug in dw2_find_symbol_file

[Doug Evans]

On Mon, Jun 11, 2012 at 2:08 PM, Doug Evans <dje@google.com> wrote:
> On Mon, Jun 11, 2012 at 1:57 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>>
>> Doug> You need to call init_cutu_and_read_dies in both debug-types and
>> Doug> non-debug-types cases.  When DWO files are in use DW_AT_name lives
>> Doug> in the DWO file.  OTOH TUs typically don't have DW_AT_name (you
>> Doug> need to look at DW_AT_decl_file).  OTOOH this function is only
>> Doug> called by find_main_filename.
>>
>> What gcc branch should I use to test this?
>> And do I need gold?
>>
>> Doug> I'm tempted to rename the routine {,*_}find_function_file (or some
>> Doug> such) and have dw2_find_symbol_file ignore TUs.
>>
>> Fine by me, but I don't plan to do it.
>
> Well, there's only 5 or so places you need to change for the renaming. :-)
>
> What I'm saying is the patch is wrong for TUs but TUs are irrelevant
> here anyway.
> What did you want to test?

Heh.  Monday blues.
[The patch is also wrong for CUs.]

To test with DWO files you'll need a fission gcc+binutils.
I will let Sterling or Cary suggest what's the best to use.

Re: FYI: fix latent bug in dw2_find_symbol_file

[Cary Coutant]

>>> Doug> You need to call init_cutu_and_read_dies in both debug-types and
>>> Doug> non-debug-types cases.  When DWO files are in use DW_AT_name lives
>>> Doug> in the DWO file.  OTOH TUs typically don't have DW_AT_name (you
>>> Doug> need to look at DW_AT_decl_file).  OTOOH this function is only
>>> Doug> called by find_main_filename.
>>>
>>> What gcc branch should I use to test this?
>>> And do I need gold?
>>>
>>> Doug> I'm tempted to rename the routine {,*_}find_function_file (or some
>>> Doug> such) and have dw2_find_symbol_file ignore TUs.
>>>
>>> Fine by me, but I don't plan to do it.
>>
>> Well, there's only 5 or so places you need to change for the renaming. :-)
>>
>> What I'm saying is the patch is wrong for TUs but TUs are irrelevant
>> here anyway.
>> What did you want to test?
>
> Heh.  Monday blues.
> [The patch is also wrong for CUs.]
>
> To test with DWO files you'll need a fission gcc+binutils.
> I will let Sterling or Cary suggest what's the best to use.

Since only the first of the fission patches has been checked in
upstream so far, I'd suggest that the best thing to try is the
google/gcc-4_6 branch. There, the fission option is spelled
"-gfission" (it'll be "-gsplit-dwarf" when checked in on trunk). I
have a branch in my own git repo that's a lot closer to top of trunk,
though, and I could push that to the google/debugfission branch in the
git mirror if you'd like.

-cary

Re: FYI: fix latent bug in dw2_find_symbol_file

[Tom Tromey]

>>>>> "Cary" == Cary Coutant <ccoutant@google.com> writes:

Cary> Since only the first of the fission patches has been checked in
Cary> upstream so far, I'd suggest that the best thing to try is the
Cary> google/gcc-4_6 branch. There, the fission option is spelled
Cary> "-gfission" (it'll be "-gsplit-dwarf" when checked in on trunk). I
Cary> have a branch in my own git repo that's a lot closer to top of trunk,
Cary> though, and I could push that to the google/debugfission branch in the
Cary> git mirror if you'd like.

I don't mind about the option spelling.
Are the linker patches I need in CVS HEAD gold?

Tom

Re: FYI: fix latent bug in dw2_find_symbol_file

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> What I'm saying is the patch is wrong for TUs but TUs are irrelevant
Doug> here anyway.
Doug> What did you want to test?

A fix, plus the other changes I am planning.

Tom

Re: FYI: fix latent bug in dw2_find_symbol_file

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> You need to call init_cutu_and_read_dies in both debug-types and
Doug> non-debug-types cases.

Here's the patch I'm checking in.

Doug patiently walked me through the setup.  Thanks, Doug.

For the record, you must:

* Build the google/gcc-4_6 branch of gcc
* Build CVS HEAD binutils and arrange for gold to be built and be
  the installed 'ld'
* Run the test suite like so:

    runtest {C,CXX}FLAGS_FOR_TARGET='-g -gdwarf-4 -gfission -Wl,--gdb-index' GDB='../gdb --use-deprecated-index-sections'

Even with this I couldn't see a regression, I think because I forgot to
build the Objective C compiler (for the dwz branch, the failure that
motivated the initial patch came from the gdb.objc).

Doug showed me a simple reproducer, though, and I've incorporated this
into the patch.

Built and regtested on x86-64 Fedora 16.

Tom

2012-06-15  Tom Tromey  <tromey@redhat.com>

	* dwarf2read.c (dw2_find_symbol_file): Unconditionally use
	init_cutu_and_read_dies.

2012-06-15  Tom Tromey  <tromey@redhat.com>

	* gdb.cp/namespace.exp: Add "show lang" test.

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 1fdd819..a8cd158 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2906,12 +2906,8 @@ dw2_find_symbol_file (struct objfile *objfile, const char *name)
   if (per_cu->v.quick->symtab != NULL)
     return per_cu->v.quick->symtab->filename;
 
-  if (per_cu->is_debug_types)
-    init_cutu_and_read_dies (per_cu, 0, 0, dw2_get_primary_filename_reader,
-			     &filename);
-  else
-    init_cutu_and_read_dies_simple (per_cu, dw2_get_primary_filename_reader,
-				    &filename);
+  init_cutu_and_read_dies (per_cu, 0, 0, dw2_get_primary_filename_reader,
+			   &filename);
 
   return filename;
 }
diff --git a/gdb/testsuite/gdb.cp/namespace.exp b/gdb/testsuite/gdb.cp/namespace.exp
index cf5a858..82018c6 100644
--- a/gdb/testsuite/gdb.cp/namespace.exp
+++ b/gdb/testsuite/gdb.cp/namespace.exp
@@ -66,6 +66,7 @@ gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
+gdb_test "show lang" "auto; currently c\\+\\+.*"
 
 #
 # set it up at a breakpoint so we can play with the variable values