SEGV in dwarf2read.c -- gdb-7.2

SEGV in dwarf2read.c -- gdb-7.2

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

I ran into a SEGV in in gdb-7.2 in dwarf_expr_eval() (actually one of the
functions it calls) which is called from dwarf2_evaluate_loc_desc_full().
This was caused by the per_cu->cu==NULL.

The circumstances which triggered this SEGV is printing a huge structure
(four or five pages of output) followed by a backtrace.  While printing
the struct, dwarf2_fetch_die_location_block() is called multiple times,
which calls age_cached_comp_units() which removes CU data from the cache.
When doing the backtrace, location descriptions are referenced and eventually
a NULL per_cu->cu is dereferenced.

I created the attached patch fixes the problem in gdb-7.2 by adding a new
function dwarf2_read_comp_unit_if_needed() which reloads the CU data and
adds it to the cache.  I also modified dwarf2_per_cu_addr_size() and
dwarf2_per_cu_addr_size() to use this function rather than read the CU
header into a temporary.

When I tried to apply this to the head, I discovered that these functions
had been refactored adding a function per_cu_header_read_in() which read
in the CU header into a temporary.

I can rework the patch to exclude the conflicting changes, but this raises
a question:  Is there a reason to read the CU header into a temporary data
area rather than reload it using load_full_comp_unit() which will add it to
the CU cache?

I noticed that a TRY_CATCH was added to dwarf2_evaluate_loc_desc_full()
which would catch this SEGV, but (it seems to me, incorrectly) indicate that
debug data was not available.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

[-- Attachment #2: gdb-cu-segv.patch --]
[-- Type: text/x-patch, Size: 3370 bytes --]

diff -u gdb/dwarf2loc.c /users/meager/ws/gdb-bi-endian/src/gdb-7.2/gdb/gdb/dwarf2loc.c
--- gdb/dwarf2loc.c	Fri Oct 28 11:43:19 2011
+++ /users/meager/ws/gdb-bi-endian/src/gdb-7.2/gdb/gdb/dwarf2loc.c	Fri Oct 28 11:03:40 2011
@@ -919,6 +919,7 @@
   ctx->get_tls_address = dwarf_expr_tls_address;
   ctx->dwarf_call = dwarf_expr_dwarf_call;
 
+  dwarf2_read_comp_unit_if_needed (per_cu);
   dwarf_expr_eval (ctx, data, size);
   if (ctx->num_pieces > 0)
     {
diff -u gdb/dwarf2loc.h /users/meager/ws/gdb-bi-endian/src/gdb-7.2/gdb/gdb/dwarf2loc.h
--- gdb/dwarf2loc.h	Wed Nov 17 17:23:58 2010
+++ /users/meager/ws/gdb-bi-endian/src/gdb-7.2/gdb/gdb/dwarf2loc.h	Fri Oct 28 11:03:05 2011
@@ -33,6 +33,10 @@
    returned.  */
 struct objfile *dwarf2_per_cu_objfile (struct dwarf2_per_cu_data *cu);
 
+/* Read in CU if needed.  */
+
+void dwarf2_read_comp_unit_if_needed (struct dwarf2_per_cu_data *per_cu);
+
 /* Return the address size given in the compilation unit header for CU.  */
 CORE_ADDR dwarf2_per_cu_addr_size (struct dwarf2_per_cu_data *cu);
 
diff -u gdb/dwarf2read.c /users/meager/ws/gdb-bi-endian/src/gdb-7.2/gdb/gdb/dwarf2read.c
--- gdb/dwarf2read.c	Fri Oct 28 11:43:19 2011
+++ /users/meager/ws/gdb-bi-endian/src/gdb-7.2/gdb/gdb/dwarf2read.c	Fri Oct 28 11:11:58 2011
@@ -12116,48 +12116,34 @@
   return objfile;
 }
 
-/* Return the address size given in the compilation unit header for CU.  */
+/* Read in CU if needed.  */
 
-CORE_ADDR
-dwarf2_per_cu_addr_size (struct dwarf2_per_cu_data *per_cu)
+void
+dwarf2_read_comp_unit_if_needed (struct dwarf2_per_cu_data *per_cu)
 {
-  if (per_cu->cu)
-    return per_cu->cu->header.addr_size;
-  else
+  if (!per_cu->cu)
     {
-      /* If the CU is not currently read in, we re-read its header.  */
       struct objfile *objfile = per_cu->psymtab->objfile;
-      struct dwarf2_per_objfile *per_objfile
-	= objfile_data (objfile, dwarf2_objfile_data_key);
-      gdb_byte *info_ptr = per_objfile->info.buffer + per_cu->offset;
-      struct comp_unit_head cu_header;
-
-      memset (&cu_header, 0, sizeof cu_header);
-      read_comp_unit_head (&cu_header, info_ptr, objfile->obfd);
-      return cu_header.addr_size;
+      load_full_comp_unit (per_cu, objfile);
     }
 }
 
+/* Return the address size given in the compilation unit header for CU.  */
+
+CORE_ADDR
+dwarf2_per_cu_addr_size (struct dwarf2_per_cu_data *per_cu)
+{
+  dwarf2_read_comp_unit_if_needed (per_cu);
+  return per_cu->cu->header.addr_size;
+}
+
 /* Return the offset size given in the compilation unit header for CU.  */
 
 int
 dwarf2_per_cu_offset_size (struct dwarf2_per_cu_data *per_cu)
 {
-  if (per_cu->cu)
-    return per_cu->cu->header.offset_size;
-  else
-    {
-      /* If the CU is not currently read in, we re-read its header.  */
-      struct objfile *objfile = per_cu->psymtab->objfile;
-      struct dwarf2_per_objfile *per_objfile
-	= objfile_data (objfile, dwarf2_objfile_data_key);
-      gdb_byte *info_ptr = per_objfile->info.buffer + per_cu->offset;
-      struct comp_unit_head cu_header;
-
-      memset (&cu_header, 0, sizeof cu_header);
-      read_comp_unit_head (&cu_header, info_ptr, objfile->obfd);
-      return cu_header.offset_size;
-    }
+  dwarf2_read_comp_unit_if_needed (per_cu);
+  return per_cu->cu->header.offset_size;
 }
 
 /* Return the text offset of the CU.  The returned offset comes from

Re: SEGV in dwarf2read.c -- gdb-7.2

[Jan Kratochvil]

Hello Michael,

sorry but I cannot comment more without a reproducer, such possible fix should
have a regression testcase anyway.  Could you provide a reproducer in some
form even just off-list (so that I can reduce it if it is not completely
public and not so critically secure)?


On Thu, 03 Nov 2011 17:12:08 +0100, Michael Eager wrote:
> I ran into a SEGV in in gdb-7.2

The stable release is 7.3.1, moreover for such development I would find only
FSF GDB HEAD relevant.  I remember several fixes which it may be related to.


> Is there a reason to read the CU header into a temporary data
> area rather than reload it using load_full_comp_unit() which will add it to
> the CU cache?

I guess for performance reasons, the CU header read-in vs. load_full_comp_unit
is a big difference.  There are already some PRs (such as 12828 (a)) where GDB
needlessly expands too many CUs "locking itself" by inacceptable performance.


Thanks,
Jan

Re: SEGV in dwarf2read.c -- gdb-7.2

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I guess for performance reasons, the CU header read-in
Jan> vs. load_full_comp_unit is a big difference.  There are already
Jan> some PRs (such as 12828 (a)) where GDB needlessly expands too many
Jan> CUs "locking itself" by inacceptable performance.

Does the CU cache serve a useful purpose these days?
I wonder whether we could just remove it.

Tom

Re: SEGV in dwarf2read.c -- gdb-7.2

[Jan Kratochvil]

On Thu, 03 Nov 2011 17:43:20 +0100, Tom Tromey wrote:
> Does the CU cache serve a useful purpose these days?

You already need to pre-read all the CUs (=load_cu) before starting to parse
them (=process_queue).  So some multi-CU storage needs to be there.  It is
true the cache should be at least significantly larger
(now dwarf2_max_cache_age == 5 generations) to have any effect, if it can have
any effect.


> I wonder whether we could just remove it.

But the code does not get much simplified without the cache IMO (sure one
should benchmark it), there is still needed tracking of the multipled read in
CUs etc.


Regards,
Jan

Re: SEGV in dwarf2read.c -- gdb-7.2

[Michael Eager]

On 11/03/2011 09:55 AM, Jan Kratochvil wrote:
> On Thu, 03 Nov 2011 17:43:20 +0100, Tom Tromey wrote:
>> Does the CU cache serve a useful purpose these days?
>
> You already need to pre-read all the CUs (=load_cu) before starting to parse
> them (=process_queue).  So some multi-CU storage needs to be there.  It is
> true the cache should be at least significantly larger
> (now dwarf2_max_cache_age == 5 generations) to have any effect, if it can have
> any effect.

With the addition of the call to age_cached_comp_units() in
dwarf2_fetch_die_location_block() and dw2_do_instantiate_symtab(),
I think that the cache is aged very aggressively.  With
dwarf2_max_cache_age == 5, it looks to me that it is likely that
CU data will not be in the cache when needed.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: SEGV in dwarf2read.c -- gdb-7.2

[Jan Kratochvil]

On Thu, 03 Nov 2011 18:13:04 +0100, Michael Eager wrote:
> With the addition of the call to age_cached_comp_units() in
> dwarf2_fetch_die_location_block() and dw2_do_instantiate_symtab(),
> I think that the cache is aged very aggressively.

> With dwarf2_max_cache_age == 5, it looks to me that it is likely that CU
> data will not be in the cache when needed.

There was for example this fix:
	http://sourceware.org/ml/gdb-patches/2011-07/msg00495.html

which is not yet even in 7.3.1 as I see now, it is only in FSF GDB HEAD.

wrt CUs aging therefore only FSF GDB HEAD matters, older releases are known to
be buggy - at least due to this bug.

With FSF GDB HEAD I am not aware of any problems because any call to
age_cached_comp_units is done when CUs are no longer needed anywhere.

I do not discuss here about efficiency of the FSF GDB HEAD solution but I am
not aware of how it could crash any way.


Thanks,
Jan

Re: SEGV in dwarf2read.c -- gdb-7.2

[Michael Eager]

On 11/03/2011 10:22 AM, Jan Kratochvil wrote:
> On Thu, 03 Nov 2011 18:13:04 +0100, Michael Eager wrote:
>> With the addition of the call to age_cached_comp_units() in
>> dwarf2_fetch_die_location_block() and dw2_do_instantiate_symtab(),
>> I think that the cache is aged very aggressively.
>
>> With dwarf2_max_cache_age == 5, it looks to me that it is likely that CU
>> data will not be in the cache when needed.
>
> There was for example this fix:
> 	http://sourceware.org/ml/gdb-patches/2011-07/msg00495.html
>
> which is not yet even in 7.3.1 as I see now, it is only in FSF GDB HEAD.

Thanks.  The patch looks similar to the one I came up with.  I added
a similar check higher in the call tree.

> wrt CUs aging therefore only FSF GDB HEAD matters, older releases are known to
> be buggy - at least due to this bug.
>
> With FSF GDB HEAD I am not aware of any problems because any call to
> age_cached_comp_units is done when CUs are no longer needed anywhere.
>
> I do not discuss here about efficiency of the FSF GDB HEAD solution but I am
> not aware of how it could crash any way.

I'll see if it works with my application.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: SEGV in dwarf2read.c -- gdb-7.2

[Michael Eager]

On 11/03/2011 09:20 AM, Jan Kratochvil wrote:
> Hello Michael,
>
> sorry but I cannot comment more without a reproducer, such possible fix should
> have a regression testcase anyway.  Could you provide a reproducer in some
> form even just off-list (so that I can reduce it if it is not completely
> public and not so critically secure)?

I don't think I can.  The application is proprietary.  I not sure that I
can create an artificial test case with a structure containing hundreds of
members and multiple compilation units which might trigger the SEGV.

> On Thu, 03 Nov 2011 17:12:08 +0100, Michael Eager wrote:
>> I ran into a SEGV in in gdb-7.2
>
> The stable release is 7.3.1, moreover for such development I would find only
> FSF GDB HEAD relevant.  I remember several fixes which it may be related to.

Can't help that.  Bugs are found on the tools in use, not necessarily
the latest release.

>> Is there a reason to read the CU header into a temporary data
>> area rather than reload it using load_full_comp_unit() which will add it to
>> the CU cache?
>
> I guess for performance reasons, the CU header read-in vs. load_full_comp_unit
> is a big difference.  There are already some PRs (such as 12828 (a)) where GDB
> needlessly expands too many CUs "locking itself" by inacceptable performance.

It seems that this would cause the CU header to be read many times, when
it would otherwise use the cache.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

Re: SEGV in dwarf2read.c -- gdb-7.2

[Jan Kratochvil]

On Thu, 03 Nov 2011 18:05:33 +0100, Michael Eager wrote:
> Can't help that.  Bugs are found on the tools in use, not necessarily
> the latest release.

In such case I have to unfortunately say the bug is probably already fixed,
see the other mail.


> > I guess for performance reasons, the CU header read-in vs. load_full_comp_unit
> > is a big difference.  There are already some PRs (such as 12828 (a)) where GDB
> > needlessly expands too many CUs "locking itself" by inacceptable performance.
> 
> It seems that this would cause the CU header to be read many times, when
> it would otherwise use the cache.

It still may be more effective this way, it is difficult to decide without
benchmarking specific cases.


Thanks,
Jan