[patch] Fix for PR gdb/12573

[patch] Fix for PR gdb/12573

[Paul Pluzhnikov]

Greetings,

Attached patch fixes http://sourceware.org/bugzilla/show_bug.cgi?id=12573
by removing (I believe incorrect) assertion.

The assertion is incorrect, because skip_prologue_sal may in fact put the
sal at a different line than what original_pc describes (if there are
inlined functions in the picture).

Also, we don't perform the same check if expanded.nelts == 1, which is why
the test from that PR does not cause a crash when built with GCC-4.6
(GCC-4.6 does not emit two copies -- C1 and C2 -- of a constructor).

If it did, we'd see the same crash there as well.

Thanks,

--
Paul Pluzhnikov

2011-03-16  Paul Pluzhnikov  <ppluzhnikov@google.com>

	PR gdb/12573
	* breakpoint.c (expand_line_sal_maybe): Remove bogus assertion.




Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.552
diff -u -p -r1.552 breakpoint.c
--- breakpoint.c	14 Mar 2011 16:09:52 -0000	1.552
+++ breakpoint.c	16 Mar 2011 22:31:08 -0000
@@ -7447,18 +7447,6 @@ expand_line_sal_maybe (struct symtab_and
       return expanded;      
     }
 
-  if (original_pc)
-    {
-      found = 0;
-      for (i = 0; i < expanded.nelts; ++i)
-	if (expanded.sals[i].pc == original_pc)
-	  {
-	    found = 1;
-	    break;
-	  }
-      gdb_assert (found);
-    }
-
   return expanded;
 }
 

Re: [patch] Fix for PR gdb/12573

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Attached patch fixes http://sourceware.org/bugzilla/show_bug.cgi?id=12573
Paul> by removing (I believe incorrect) assertion.

See also:

https://bugzilla.redhat.com/show_bug.cgi?id=612253

And Jan's patch:

http://sourceware.org/ml/gdb-patches/2010-07/msg00533.html

I can't say I fully understand Jan's.
It includes yours, though, and comes with a test case.

Tom

Re: [patch] Fix for PR gdb/12573

[Paul Pluzhnikov]

On Fri, Mar 18, 2011 at 9:29 AM, Tom Tromey <tromey@redhat.com> wrote:

>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
> Paul> Attached patch fixes http://sourceware.org/bugzilla/show_bug.cgi?id=12573
> Paul> by removing (I believe incorrect) assertion.
>
> See also:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=612253
>
> And Jan's patch:
>
> http://sourceware.org/ml/gdb-patches/2010-07/msg00533.html
>
> I can't say I fully understand Jan's.

The patch looks good to me (and I *think* I understand it).

> It includes yours, though, and comes with a test case.

I was going to include a test case when pinging my patch in a week ;-)

Jan,

Could you reference PR gdb/12573 in your patch, and maybe commit it?

+      /* Place breakpoint only to the first PC in a function, even if some of
+	 them are in a lexical sub-block.  Put it too all the function
+	 instances incl. the inlined ones.  */

Suggest:

  Put it on all function instances as well, including inlined ones.

Thanks,
-- 
Paul Pluzhnikov

Re: [patch] Fix for PR gdb/12573

[Jan Kratochvil]

On Fri, 18 Mar 2011 18:01:50 +0100, Paul Pluzhnikov wrote:
> On Fri, Mar 18, 2011 at 9:29 AM, Tom Tromey <tromey@redhat.com> wrote:
> >>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
> >
> > Paul> Attached patch fixes http://sourceware.org/bugzilla/show_bug.cgi?id=12573
> > Paul> by removing (I believe incorrect) assertion.
> >
> > See also:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=612253
> >
> > And Jan's patch:
> >
> > http://sourceware.org/ml/gdb-patches/2010-07/msg00533.html
> >
> > I can't say I fully understand Jan's.
> 
> The patch looks good to me (and I *think* I understand it).

I have to review it again but for example it would hide another bug but I do
not have a fix for it, it should be pretty straightforward if anyone would
like to write it:

https://bugzilla.redhat.com/show_bug.cgi?id=612253#c6

One of the problems is that GDB tries to skip prologue even for -O2 -g code.
There is no such reason as with -O2 -g the debug info is correct for each
instructions.  With -O0 -g there are frame-related absolute addresses of
autovariables which is the reason GDB needs to skip the prologue to have valid
location of such -O0 -g autovariables.

-O2 -g code can be detected for a Compilation Unit if there is referenced any
location list from that CU (suggested by GCC hackers).  In such case skipping
prologues should be disabled.

  Compilation Unit @ offset 0x90398e:
 <0><903999>: Abbrev Number: 1 (DW_TAG_compile_unit)
    <90399f>   DW_AT_name        : ../../gcc/gcc/tree-into-ssa.c
    <9039a3>   DW_AT_comp_dir    : /user/inria/fsf/160832/bld-2/gcc
 <1><91073a>: Abbrev Number: 69 (DW_TAG_subprogram)
    <91073b>   DW_AT_name        : add_new_name_mapping
 <2><910751>: Abbrev Number: 75 (DW_TAG_formal_parameter)
    <910752>   DW_AT_name        : new_tree
    <910759>   DW_AT_type        : <0x903ea5>
    <91075d>   DW_AT_location    : 0x59d960     (location list)
                                                ^^^^^^^^^^^^^^^

(gdb) break add_new_name_mapping
->
(gdb) p/x original_pc
$1 = 0x85be0bd

085be0b0 <add_new_name_mapping>:
 85be0b0:       55                      push   %ebp
 85be0b1:       89 e5                   mov    %esp,%ebp
 85be0b3:       57                      push   %edi
 85be0b4:       56                      push   %esi
 85be0b5:       89 c6                   mov    %eax,%esi
 85be0b7:       53                      push   %ebx
 85be0b8:       89 d3                   mov    %edx,%ebx
 85be0ba:       83 ec 4c                sub    $0x4c,%esp
 85be0bd:       80 3d a0 8c ca 08 00    cmpb   $0x0,0x8ca8ca0
 ^^^^^^^

There are other problems but this is the first step which should make the
problems at least no longer affecting this specific case.


Thanks,
Jan

[patch] Do not skip prologue for -O2 -g with GCC VTA Re: [patch] Fix for PR gdb/12573

[Jan Kratochvil]

On Fri, 18 Mar 2011 18:12:46 +0100, Jan Kratochvil wrote:
> One of the problems is that GDB tries to skip prologue even for -O2 -g code.
> There is no such reason as with -O2 -g the debug info is correct for each
> instructions.  With -O0 -g there are frame-related absolute addresses of
> autovariables which is the reason GDB needs to skip the prologue to have valid
> location of such -O0 -g autovariables.
> 
> -O2 -g code can be detected for a Compilation Unit if there is referenced any
> location list from that CU (suggested by GCC hackers).  In such case skipping
> prologues should be disabled.

Implementation attached.  I was told systemtap is using the same idea (I have
ot checked the systemtap implementation).

It does not fix the artificial testcase of mine and it does not fix the
problem in general.  But it fixes the binary you (Paul) attached to PR 12573
and the binary in the Red Hat BZ.  It is a mess GDB now chooses "random"
addresses in the -O2 -g code and with this clean up the problem does not
happen in the cases I am aware of.

I am not sure if there should not be also a GCC DW_AT_producer check but
I hope no compiler produces DW_AT_location as a location list not covering the
functions prologues.

No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu.

This is not suitable for 7.3 so late.


Thanks,
Jan


gdb/
2011-03-28  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c: Include ctype.h.
	(struct dwarf2_cu): New field has_loclist.
	(process_full_comp_unit): Set also symtab->locations_valid.  Move the
	symtab->language code.
	(var_decode_location): Set cu->has_loclist.
	* symtab.c (skip_prologue_sal): Return on LOCATIONS_VALID.
	* symtab.h (struct symtab): Make the primary field a bitfield.  New
	field locations_valid.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -57,6 +57,7 @@
 #include "vec.h"
 #include "c-lang.h"
 #include "valprint.h"
+#include <ctype.h>
 
 #include <fcntl.h>
 #include "gdb_string.h"
@@ -395,6 +396,11 @@ struct dwarf2_cu
      DIEs for namespaces, we don't need to try to infer them
      from mangled names.  */
   unsigned int has_namespace_info : 1;
+
+  /* This CU references .debug_loc.  It means it has been compiled with
+     optimizations and debug info together so that GDB may stop skipping the
+     prologue.  */
+  unsigned int has_loclist : 1;
 };
 
 /* Persistent data held for a compilation unit, even when not
@@ -4626,13 +4632,15 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
 
   symtab = end_symtab (highpc + baseaddr, objfile, SECT_OFF_TEXT (objfile));
 
-  /* Set symtab language to language from DW_AT_language.
-     If the compilation is from a C file generated by language preprocessors,
-     do not set the language if it was already deduced by start_subfile.  */
-  if (symtab != NULL
-      && !(cu->language == language_c && symtab->language != language_c))
+  if (symtab != NULL)
     {
-      symtab->language = cu->language;
+      /* Set symtab language to language from DW_AT_language.  If the
+	 compilation is from a C file generated by language preprocessors, do
+	 not set the language if it was already deduced by start_subfile.  */
+      if (!(cu->language == language_c && symtab->language != language_c))
+	symtab->language = cu->language;
+
+      symtab->locations_valid = cu->has_loclist;
     }
 
   if (dwarf2_per_objfile->using_index)
@@ -10839,6 +10847,9 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
 
   dwarf2_symbol_mark_computed (attr, sym, cu);
   SYMBOL_CLASS (sym) = LOC_COMPUTED;
+
+  if (SYMBOL_COMPUTED_OPS (sym) == &dwarf2_loclist_funcs)
+    cu->has_loclist = 1;
 }
 
 /* Given a pointer to a DWARF information entry, figure out if we need
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -2470,6 +2470,9 @@ skip_prologue_sal (struct symtab_and_line *sal)
   if (sal->explicit_pc)
     return;
 
+  if (sal->symtab != NULL && sal->symtab->locations_valid)
+    return;
+
   old_chain = save_current_space_and_thread ();
   switch_to_program_space_and_thread (sal->pspace);
 
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -762,7 +762,12 @@ struct symtab
      should be designated the primary, so that the blockvector
      is relocated exactly once by objfile_relocate.  */
 
-  int primary;
+  unsigned int primary : 1;
+
+  /* GDB does not need to skip prologues as the variable locations are valid
+     for all the PC values.  */
+
+  unsigned int locations_valid : 1;
 
   /* The macro table for this symtab.  Like the blockvector, this
      may be shared between different symtabs --- and normally is for

Re: [patch] Do not skip prologue for -O2 -g with GCC VTA Re: [patch] Fix for PR gdb/12573

[Paul Pluzhnikov]

Ping?

(This fixes a crash we are actually hitting, so an upstream fix is
very desirable.)

Thanks!

On Mon, Mar 28, 2011 at 8:18 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 18 Mar 2011 18:12:46 +0100, Jan Kratochvil wrote:
>> One of the problems is that GDB tries to skip prologue even for -O2 -g code.
>> There is no such reason as with -O2 -g the debug info is correct for each
>> instructions.  With -O0 -g there are frame-related absolute addresses of
>> autovariables which is the reason GDB needs to skip the prologue to have valid
>> location of such -O0 -g autovariables.
>>
>> -O2 -g code can be detected for a Compilation Unit if there is referenced any
>> location list from that CU (suggested by GCC hackers).  In such case skipping
>> prologues should be disabled.
>
> Implementation attached.  I was told systemtap is using the same idea (I have
> ot checked the systemtap implementation).
>
> It does not fix the artificial testcase of mine and it does not fix the
> problem in general.  But it fixes the binary you (Paul) attached to PR 12573
> and the binary in the Red Hat BZ.  It is a mess GDB now chooses "random"
> addresses in the -O2 -g code and with this clean up the problem does not
> happen in the cases I am aware of.
>
> I am not sure if there should not be also a GCC DW_AT_producer check but
> I hope no compiler produces DW_AT_location as a location list not covering the
> functions prologues.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu.
>
> This is not suitable for 7.3 so late.
>
>
> Thanks,
> Jan
>
>
> gdb/
> 2011-03-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        * dwarf2read.c: Include ctype.h.
>        (struct dwarf2_cu): New field has_loclist.
>        (process_full_comp_unit): Set also symtab->locations_valid.  Move the
>        symtab->language code.
>        (var_decode_location): Set cu->has_loclist.
>        * symtab.c (skip_prologue_sal): Return on LOCATIONS_VALID.
>        * symtab.h (struct symtab): Make the primary field a bitfield.  New
>        field locations_valid.
>
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -57,6 +57,7 @@
>  #include "vec.h"
>  #include "c-lang.h"
>  #include "valprint.h"
> +#include <ctype.h>
>
>  #include <fcntl.h>
>  #include "gdb_string.h"
> @@ -395,6 +396,11 @@ struct dwarf2_cu
>      DIEs for namespaces, we don't need to try to infer them
>      from mangled names.  */
>   unsigned int has_namespace_info : 1;
> +
> +  /* This CU references .debug_loc.  It means it has been compiled with
> +     optimizations and debug info together so that GDB may stop skipping the
> +     prologue.  */
> +  unsigned int has_loclist : 1;
>  };
>
>  /* Persistent data held for a compilation unit, even when not
> @@ -4626,13 +4632,15 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
>
>   symtab = end_symtab (highpc + baseaddr, objfile, SECT_OFF_TEXT (objfile));
>
> -  /* Set symtab language to language from DW_AT_language.
> -     If the compilation is from a C file generated by language preprocessors,
> -     do not set the language if it was already deduced by start_subfile.  */
> -  if (symtab != NULL
> -      && !(cu->language == language_c && symtab->language != language_c))
> +  if (symtab != NULL)
>     {
> -      symtab->language = cu->language;
> +      /* Set symtab language to language from DW_AT_language.  If the
> +        compilation is from a C file generated by language preprocessors, do
> +        not set the language if it was already deduced by start_subfile.  */
> +      if (!(cu->language == language_c && symtab->language != language_c))
> +       symtab->language = cu->language;
> +
> +      symtab->locations_valid = cu->has_loclist;
>     }
>
>   if (dwarf2_per_objfile->using_index)
> @@ -10839,6 +10847,9 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
>
>   dwarf2_symbol_mark_computed (attr, sym, cu);
>   SYMBOL_CLASS (sym) = LOC_COMPUTED;
> +
> +  if (SYMBOL_COMPUTED_OPS (sym) == &dwarf2_loclist_funcs)
> +    cu->has_loclist = 1;
>  }
>
>  /* Given a pointer to a DWARF information entry, figure out if we need
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -2470,6 +2470,9 @@ skip_prologue_sal (struct symtab_and_line *sal)
>   if (sal->explicit_pc)
>     return;
>
> +  if (sal->symtab != NULL && sal->symtab->locations_valid)
> +    return;
> +
>   old_chain = save_current_space_and_thread ();
>   switch_to_program_space_and_thread (sal->pspace);
>
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -762,7 +762,12 @@ struct symtab
>      should be designated the primary, so that the blockvector
>      is relocated exactly once by objfile_relocate.  */
>
> -  int primary;
> +  unsigned int primary : 1;
> +
> +  /* GDB does not need to skip prologues as the variable locations are valid
> +     for all the PC values.  */
> +
> +  unsigned int locations_valid : 1;
>
>   /* The macro table for this symtab.  Like the blockvector, this
>      may be shared between different symtabs --- and normally is for
>



-- 
Paul Pluzhnikov

obsolete: Re: [patch] Do not skip prologue for -O2 -g with GCC VTA Re: [patch] Fix for PR gdb/12573

[Jan Kratochvil]

obsoleted by:
	[rfc, 7.3?] -O2 -g breakpoints internal error + prologue skipping
	http://sourceware.org/ml/gdb-patches/2011-04/msg00229.html