[patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Jan Kratochvil]

Hi,

as discussed on #gdb when you set max-cache-age 0 DW_OP_call{2,4} crashed GDB.

I admit I rather did not test max-cache-age 0 globally.

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

OK to check-in?

The problem is not reproducible with max-cache-age 1.  Without the
dw2_do_instantiate_symtab patch part GDB no longer crashes on max-cache-age 0
but it will then error out on DW_OP_call{2,4} with that:
+    error (_("Dwarf Error: Cannot read CU for DIE at 0x%x referenced "
+	     "in module %s"),

There is a bit weird that functions with parameter per_cu have also parameter
objfile when there is per_cu->objfile.  It is because per_cu->objfile is there
only since cf9fe5cf2be8b76820a05c966cdca6df5c2eee24 (Tom Tromey 2010-07-13).


Thanks,
Jan


gdb/
2010-08-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* dwarf2read.c (dw2_do_instantiate_symtab): Move the
	age_cached_comp_units call to the top, extend its comment.
	(dwarf2_fetch_die_location_block): Initialize cu later.  Call
	dw2_setup and dw2_do_instantiate_symtab if PER_CU->CU is NULL.

gdb/testsuite/
2010-08-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.dwarf2/dw2-op-call.exp (maintenance set dwarf2 max-cache-age 0):
	New test.

--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1636,6 +1636,11 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
 {
   struct cleanup *back_to;
 
+  /* Age the cache, releasing compilation units that have not been used
+     recently.  Age them first so that we do not age out the requested PER_CU
+     unit if DWARF2_MAX_CACHE_AGE is too low.  */
+  age_cached_comp_units ();
+
   back_to = make_cleanup (dwarf2_release_queue, NULL);
 
   queue_comp_unit (per_cu, objfile);
@@ -1647,10 +1652,6 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
 
   process_queue (objfile);
 
-  /* Age the cache, releasing compilation units that have not
-     been used recently.  */
-  age_cached_comp_units ();
-
   do_cleanups (back_to);
 }
 
@@ -12720,11 +12721,22 @@ struct dwarf2_locexpr_baton
 dwarf2_fetch_die_location_block (unsigned int offset,
 				 struct dwarf2_per_cu_data *per_cu)
 {
-  struct dwarf2_cu *cu = per_cu->cu;
+  struct dwarf2_cu *cu;
   struct die_info *die;
   struct attribute *attr;
   struct dwarf2_locexpr_baton retval;
 
+  if (per_cu->cu == NULL)
+    {
+      dw2_setup (per_cu->objfile);
+      dw2_do_instantiate_symtab (per_cu->objfile, per_cu);
+    }
+  if (per_cu->cu == NULL)
+    error (_("Dwarf Error: Cannot read CU for DIE at 0x%x referenced "
+	     "in module %s"),
+	   offset, per_cu->objfile->name);
+
+  cu = per_cu->cu;
   die = follow_die_offset (offset, &cu);
   if (!die)
     error (_("Dwarf Error: Cannot find DIE at 0x%x referenced in module %s"),
--- a/gdb/testsuite/gdb.dwarf2/dw2-op-call.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-op-call.exp
@@ -36,6 +36,9 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objdir}/${subdir}/${execu
 
 clean_restart $executable
 
+# Additional test to verify the referenced CU is not aged out.
+gdb_test_no_output "maintenance set dwarf2 max-cache-age 0"
+
 gdb_test "p array1" " = 1"
 gdb_test "p array2" " = 2" "array2 using DW_OP_call2"
 gdb_test "p array3" " = 3" "array3 using DW_OP_call4"

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Doug Evans]

Hi.  Comments inline.

On Mon, Aug 23, 2010 at 11:50 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi,
>
> as discussed on #gdb when you set max-cache-age 0 DW_OP_call{2,4} crashed GDB.
>
> I admit I rather did not test max-cache-age 0 globally.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.
>
> OK to check-in?
>
> The problem is not reproducible with max-cache-age 1.  Without the
> dw2_do_instantiate_symtab patch part GDB no longer crashes on max-cache-age 0
> but it will then error out on DW_OP_call{2,4} with that:
> +    error (_("Dwarf Error: Cannot read CU for DIE at 0x%x referenced "
> +            "in module %s"),
>
> There is a bit weird that functions with parameter per_cu have also parameter
> objfile when there is per_cu->objfile.  It is because per_cu->objfile is there
> only since cf9fe5cf2be8b76820a05c966cdca6df5c2eee24 (Tom Tromey 2010-07-13).
>
>
> Thanks,
> Jan
>
>
> gdb/
> 2010-08-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        * dwarf2read.c (dw2_do_instantiate_symtab): Move the
>        age_cached_comp_units call to the top, extend its comment.
>        (dwarf2_fetch_die_location_block): Initialize cu later.  Call
>        dw2_setup and dw2_do_instantiate_symtab if PER_CU->CU is NULL.
>
> gdb/testsuite/
> 2010-08-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        * gdb.dwarf2/dw2-op-call.exp (maintenance set dwarf2 max-cache-age 0):
>        New test.
>
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1636,6 +1636,11 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
>  {
>   struct cleanup *back_to;
>
> +  /* Age the cache, releasing compilation units that have not been used
> +     recently.  Age them first so that we do not age out the requested PER_CU
> +     unit if DWARF2_MAX_CACHE_AGE is too low.  */
> +  age_cached_comp_units ();

Aging cached units first feels weird (if not wrong at least weird); we
may toss out something we're about to want.
At the least IWBN to elaborate on why this fixes things.

> +
>   back_to = make_cleanup (dwarf2_release_queue, NULL);
>
>   queue_comp_unit (per_cu, objfile);
> @@ -1647,10 +1652,6 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
>
>   process_queue (objfile);
>
> -  /* Age the cache, releasing compilation units that have not
> -     been used recently.  */
> -  age_cached_comp_units ();
> -
>   do_cleanups (back_to);
>  }
>
> @@ -12720,11 +12721,22 @@ struct dwarf2_locexpr_baton
>  dwarf2_fetch_die_location_block (unsigned int offset,
>                                 struct dwarf2_per_cu_data *per_cu)
>  {
> -  struct dwarf2_cu *cu = per_cu->cu;
> +  struct dwarf2_cu *cu;
>   struct die_info *die;
>   struct attribute *attr;
>   struct dwarf2_locexpr_baton retval;
>
> +  if (per_cu->cu == NULL)
> +    {
> +      dw2_setup (per_cu->objfile);
> +      dw2_do_instantiate_symtab (per_cu->objfile, per_cu);
> +    }
> +  if (per_cu->cu == NULL)
> +    error (_("Dwarf Error: Cannot read CU for DIE at 0x%x referenced "
> +            "in module %s"),
> +          offset, per_cu->objfile->name);
> +
> +  cu = per_cu->cu;
>   die = follow_die_offset (offset, &cu);
>   if (!die)
>     error (_("Dwarf Error: Cannot find DIE at 0x%x referenced in module %s"),
> --- a/gdb/testsuite/gdb.dwarf2/dw2-op-call.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-op-call.exp
> @@ -36,6 +36,9 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objdir}/${subdir}/${execu
>
>  clean_restart $executable
>
> +# Additional test to verify the referenced CU is not aged out.
> +gdb_test_no_output "maintenance set dwarf2 max-cache-age 0"
> +
>  gdb_test "p array1" " = 1"
>  gdb_test "p array2" " = 2" "array2 using DW_OP_call2"
>  gdb_test "p array3" " = 3" "array3 using DW_OP_call4"
>

The comment reads better to me if "Additional test" is removed.  I.e.
"Verify the ...".
It's not really an "additional test".

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Jan Kratochvil]

On Mon, 23 Aug 2010 21:30:06 +0200, Doug Evans wrote:
> On Mon, Aug 23, 2010 at 11:50 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > as discussed on #gdb when you set max-cache-age 0 DW_OP_call{2,4} crashed GDB.
[...]
> > --- a/gdb/dwarf2read.c
> > +++ b/gdb/dwarf2read.c
> > @@ -1636,6 +1636,11 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
> >  {
> >   struct cleanup *back_to;
> >
> > +  /* Age the cache, releasing compilation units that have not been used
> > +     recently.  Age them first so that we do not age out the requested PER_CU
> > +     unit if DWARF2_MAX_CACHE_AGE is too low.  */
> > +  age_cached_comp_units ();
> 
> Aging cached units first feels weird (if not wrong at least weird); we
> may toss out something we're about to want.
> At the least IWBN to elaborate on why this fixes things.

As otherwise we will age out what we have found (on max-cache-age 0).

One could forbid value zero for max-cache-age but that also does not seem
right to me.

There is such a general cleanup moment when GDB is fully idle
- prepare_execute_command() - shouldn't age_cached_comp_units be called there?

But that way sooner or later we will age out every CU.  This may occur a bit
even nowadays, the default value 5 is also very low.  max-cache-age as "how
long" is IMO not userful to the user.  There could be more a setting "how
many" CUs can be loaded at once.  CU age would be then just an internal
indicator to maintain the count under the "how many" limit.

I would change "max-cache-age" to "max-cache-size" and call it from
prepare_execute_command() instead.  I will provide a patch if not replied.


Thanks,
Jan

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Doug Evans]

On Thu, Sep 2, 2010 at 9:02 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Mon, 23 Aug 2010 21:30:06 +0200, Doug Evans wrote:
>> On Mon, Aug 23, 2010 at 11:50 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
>> > as discussed on #gdb when you set max-cache-age 0 DW_OP_call{2,4} crashed GDB.
> [...]
>> > --- a/gdb/dwarf2read.c
>> > +++ b/gdb/dwarf2read.c
>> > @@ -1636,6 +1636,11 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
>> >  {
>> >   struct cleanup *back_to;
>> >
>> > +  /* Age the cache, releasing compilation units that have not been used
>> > +     recently.  Age them first so that we do not age out the requested PER_CU
>> > +     unit if DWARF2_MAX_CACHE_AGE is too low.  */
>> > +  age_cached_comp_units ();
>>
>> Aging cached units first feels weird (if not wrong at least weird); we
>> may toss out something we're about to want.
>> At the least IWBN to elaborate on why this fixes things.
>
> As otherwise we will age out what we have found (on max-cache-age 0).

Ah.
Still, dw2_do_instantiate_symtab seems like the wrong tool for the job here.
Its job is to instantiate a symtab, it currently doesn't guarantee it
will leave the CU read in when finished, and adding that guarantee
doesn't feel right.

Assuming (and I don't know dwarf2_fetch_die_location_block well) just
needs the dies and not a symtab, how about moving this bit of code to
its own function, and calling it from both dw2_do_instantiate_symtab
and dwarf2_fetch_die_location_block.

    if (per_cu->from_debug_types)
      read_signatured_type_at_offset (objfile, per_cu->offset);
    else
      load_full_comp_unit (per_cu, objfile);

I haven't thought it through (e.g. it may need a bit of glue), but it
feels like a better approach.

> One could forbid value zero for max-cache-age but that also does not seem
> right to me.

max-cache-age == 0 is defined to disable the cache.  It's a useful
test vehicle, and I don't see any reason to disallow it either.

> There is such a general cleanup moment when GDB is fully idle
> - prepare_execute_command() - shouldn't age_cached_comp_units be called there?

I don't know.  Or as a cleanup (either via a cleanup itself, or as
part of some top level thing akin to whatever you'd do in
prepare_execute_command.  making use of an existing facility
(make_cleanup) would be preferable of course, assuming it's the way to
go)? It feels better to do this at the end of a command, not before.

> But that way sooner or later we will age out every CU.  This may occur a bit
> even nowadays, the default value 5 is also very low.  max-cache-age as "how
> long" is IMO not userful to the user.  There could be more a setting "how
> many" CUs can be loaded at once.  CU age would be then just an internal
> indicator to maintain the count under the "how many" limit.

A better measure may be memory used (e.g. lots of CUs are ok if
they're all relatively small).  IWBN to find/collect stats on the
distribution of #CUs and sizes.  [e.g. can we make some reasonable
assumptions so that we don't have to track die memory usage?]

> I would change "max-cache-age" to "max-cache-size" and call it from
> prepare_execute_command() instead.  I will provide a patch if not replied.

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Tom Tromey]

Jan> I would change "max-cache-age" to "max-cache-size" and call it from
Jan> prepare_execute_command() instead.  I will provide a patch if not replied.

Changing it to be size-based makes sense to me.
I don't get the rationale for putting it in prepare_execute_command.
If we are flushing the cache based on memory use, then we only need to
consider flushing it just before we read a CU.

Tom

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Jan Kratochvil]

On Fri, 03 Sep 2010 17:35:50 +0200, Tom Tromey wrote:
> I don't get the rationale for putting it in prepare_execute_command.
> If we are flushing the cache based on memory use, then we only need to
> consider flushing it just before we read a CU.

There is currently no way of "locking" CUs.  Some processing may arbitrarily
access more and more CUs, and even the previous ones.  Processing may need
generally unlimited number of CUs, therefore it can reach the limit and flush
still referenced CU.

Therefore I find prepare_execute_command as the only safe place to flush any
CU.

(I may miss there exist some more strict rules than I am aware of.)


Thanks,
Jan

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Doug Evans]

On Fri, Sep 3, 2010 at 8:39 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 03 Sep 2010 17:35:50 +0200, Tom Tromey wrote:
>> I don't get the rationale for putting it in prepare_execute_command.
>> If we are flushing the cache based on memory use, then we only need to
>> consider flushing it just before we read a CU.
>
> There is currently no way of "locking" CUs.  Some processing may arbitrarily
> access more and more CUs, and even the previous ones.  Processing may need
> generally unlimited number of CUs, therefore it can reach the limit and flush
> still referenced CU.

Unless code that needs a CU reads it in as necessary, and the API into
the dwarf reader only ages the cache at well defined points that no
longer need CUs (e.g. when we're done psymtab->symtab expansion).
(right?)

> Therefore I find prepare_execute_command as the only safe place to flush any
> CU.

OOC, If we did move cache aging to a higher level, is there a reason
it can't be done at cleanup time?
[For reference sake, it's actually done today for free_stack_comp_unit.]

> (I may miss there exist some more strict rules than I am aware of.)

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Jan Kratochvil]

On Fri, 03 Sep 2010 17:59:16 +0200, Doug Evans wrote:
> Unless code that needs a CU reads it in as necessary, and the API into
> the dwarf reader only ages the cache at well defined points that no
> longer need CUs (e.g. when we're done psymtab->symtab expansion).
> (right?)

This means only one CU is guaranteed to be read in at one time.  And when you
hold that CU you must not call any GDB function as this function can (upon
a change in the future) request its own CU and thus invalidate CU at the
caller.  I find it as a too fragile policy.

Still I find it even ineffective.  Regular aging means CUs get flushed even if
only a few of them is now read-in.


> > Therefore I find prepare_execute_command as the only safe place to flush any
> > CU.
> 
> OOC, If we did move cache aging to a higher level, is there a reason
> it can't be done at cleanup time?
> [For reference sake, it's actually done today for free_stack_comp_unit.]

The aging currently affects all CUs read-in.  There can be several nested
calls each requesting its own CU and doing aging at the (inner) cleanup time.
That means a cleanup in the inner call may age-out CU in the outer call still
before the outer cleanup.

Without any CU locking in place I still do not see a safe point other than at
the top level idle time (that is prepare_execute_command).


Thanks,
Jan

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Doug Evans]

On Mon, Sep 6, 2010 at 2:48 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 03 Sep 2010 17:59:16 +0200, Doug Evans wrote:
>> Unless code that needs a CU reads it in as necessary, and the API into
>> the dwarf reader only ages the cache at well defined points that no
>> longer need CUs (e.g. when we're done psymtab->symtab expansion).
>> (right?)
>
> This means only one CU is guaranteed to be read in at one time.  And when you
> hold that CU you must not call any GDB function as this function can (upon
> a change in the future) request its own CU and thus invalidate CU at the
> caller.  I find it as a too fragile policy.
>
> Still I find it even ineffective.  Regular aging means CUs get flushed even if
> only a few of them is now read-in.
>
>
>> > Therefore I find prepare_execute_command as the only safe place to flush any
>> > CU.
>>
>> OOC, If we did move cache aging to a higher level, is there a reason
>> it can't be done at cleanup time?
>> [For reference sake, it's actually done today for free_stack_comp_unit.]
>
> The aging currently affects all CUs read-in.  There can be several nested
> calls each requesting its own CU and doing aging at the (inner) cleanup time.
> That means a cleanup in the inner call may age-out CU in the outer call still
> before the outer cleanup.
>
> Without any CU locking in place I still do not see a safe point other than at
> the top level idle time (that is prepare_execute_command).

How about we take a step back and enumerate the entry points into the
dwarf subsystem (at least those that need CUs).  Then we can see
if/when a CU needs to survive calls across the dwarf API.  If there's
a correctness issue here, perhaps we want to separate it from the
internal optimization detail of CU flushing.  E.g. we may find that
only flushing CUs in prepare_execute_command has issues as well.

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0

[Tom Tromey]

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

Jan> This means only one CU is guaranteed to be read in at one time.
Jan> And when you hold that CU you must not call any GDB function as
Jan> this function can (upon a change in the future) request its own CU
Jan> and thus invalidate CU at the caller.  I find it as a too fragile
Jan> policy.

Ok, I understand what you are saying.

You could also have a global to suppress aging, that is set on entry to
the DWARF module and reset on exit.

Your way is probably clearer.

Jan> Still I find it even ineffective.  Regular aging means CUs get
Jan> flushed even if only a few of them is now read-in.

I agree.

Tom

[patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2

[Jan Kratochvil]

On Thu, 02 Sep 2010 20:39:58 +0200, Doug Evans wrote:
> Assuming (and I don't know dwarf2_fetch_die_location_block well) just
> needs the dies and not a symtab,

Yes, it needs CU but not symtab.  I guess I probably messed up these two
before.


> how about moving this bit of code to
> its own function, and calling it from both dw2_do_instantiate_symtab
> and dwarf2_fetch_die_location_block.
> 
>     if (per_cu->from_debug_types)
>       read_signatured_type_at_offset (objfile, per_cu->offset);
>     else
>       load_full_comp_unit (per_cu, objfile);

Done.


> max-cache-age == 0 is defined to disable the cache.  It's a useful
> test vehicle, and I don't see any reason to disallow it either.

The testsuite now PASSes with max-cache-age == 0.  I haven't made it a global
default.


That `if' there is in fact a workaround, `load_cu' could be called
unconditionally:
+  if (per_cu->cu == NULL)
+    load_cu (per_cu);

But such conditional for load_full_comp_unit is already present there as in
follow_die_offset and it faces a change in:
	Re: [RFA] template names with arguments out of DW_AT_name
	http://sourceware.org/ml/gdb-patches/2010-08/msg00161.html
which implemented RealView compatibility without a testcase and RealView
download is only for MS-Windows (I haven't tried if it is Wine compatible).

Anyway I find that conditional need there as a different Bug.

No regressions on {x86_64,x86_64-m32,i686}-fedora15-linux-gnu.  Also with no
global "maintenance set dwarf2 max-cache-age 0".


Thanks,
Jan


gdb/
2011-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix crash if referenced CU is aged out.
	* dwarf2loc.c (per_cu_dwarf_call): New variable back_to, use to for
	xfree of block.data.
	(indirect_pieced_value): New variable back_to, use to for xfree of
	baton.data.
	(dwarf2_compile_expr_to_ax): New variable back_to, use to for xfree of
	block.data.
	* dwarf2read.c (dwarf2_find_base_address): New prototype.
	(load_cu): New function from ...
	(dw2_do_instantiate_symtab): ... the code here ...
	(process_full_comp_unit): ... and here.
	(dwarf2_fetch_die_location_block): Call load_cu first.  Call xmemdup on
	retval.data.

gdb/testsuite/
2011-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.dwarf2/dw2-op-call.exp (maintenance set dwarf2 max-cache-age 0):
	New.
	* gdb.dwarf2/implptr.exp: Likewise.

--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -265,14 +268,19 @@ per_cu_dwarf_call (struct dwarf_expr_context *ctx, size_t die_offset,
 		   void *baton)
 {
   struct dwarf2_locexpr_baton block;
+  struct cleanup *back_to;
 
   block = dwarf2_fetch_die_location_block (die_offset, per_cu,
 					   get_frame_pc, baton);
 
+  back_to = make_cleanup (xfree, (void *) block.data);
+
   /* DW_OP_call_ref is currently not supported.  */
   gdb_assert (block.per_cu == per_cu);
 
   dwarf_expr_eval (ctx, block.data, block.size);
+
+  do_cleanups (back_to);
 }
 
 /* Helper interface of per_cu_dwarf_call for dwarf2_evaluate_loc_desc.  */
@@ -966,6 +974,7 @@ indirect_pieced_value (struct value *value)
   struct dwarf_expr_piece *piece = NULL;
   struct value *result;
   LONGEST byte_offset;
+  struct cleanup *back_to;
 
   type = value_type (value);
   if (TYPE_CODE (type) != TYPE_CODE_PTR)
@@ -1013,10 +1022,14 @@ indirect_pieced_value (struct value *value)
 					   get_frame_address_in_block_wrapper,
 					   frame);
 
+  back_to = make_cleanup (xfree, (void *) baton.data);
+
   result = dwarf2_evaluate_loc_desc_full (TYPE_TARGET_TYPE (type), frame,
 					  baton.data, baton.size, baton.per_cu,
 					  byte_offset);
 
+  do_cleanups (back_to);
+
   return result;
 }
 
@@ -2108,12 +2121,14 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	  {
 	    struct dwarf2_locexpr_baton block;
 	    int size = (op == DW_OP_call2 ? 2 : 4);
+	    struct cleanup *back_to;
 
 	    uoffset = extract_unsigned_integer (op_ptr, size, byte_order);
 	    op_ptr += size;
 
 	    block = dwarf2_fetch_die_location_block (uoffset, per_cu,
 						     get_ax_pc, expr);
+	    back_to = make_cleanup (xfree, (void *) block.data);
 
 	    /* DW_OP_call_ref is currently not supported.  */
 	    gdb_assert (block.per_cu == per_cu);
@@ -2121,6 +2136,8 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	    dwarf2_compile_expr_to_ax (expr, loc, arch, addr_size,
 				       block.data, block.data + block.size,
 				       per_cu);
+
+	    do_cleanups (back_to);
 	  }
 	  break;
 
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -884,6 +884,9 @@ static void dwarf2_locate_sections (bfd *, asection *, void *);
 static void dwarf2_create_include_psymtab (char *, struct partial_symtab *,
                                            struct objfile *);
 
+static void dwarf2_find_base_address (struct die_info *die,
+				      struct dwarf2_cu *cu);
+
 static void dwarf2_build_psymtabs_hard (struct objfile *);
 
 static void scan_partial_symbols (struct partial_die_info *,
@@ -1788,6 +1791,23 @@ create_quick_file_names_table (unsigned int nr_initial_entries)
 			    delete_file_name_entry, xcalloc, xfree);
 }
 
+/* Read in PER_CU->CU.  This function is unrelated to symtabs, symtab would
+   have to be created afterwards.  You should call age_cached_comp_units after
+   processing PER_CU->CU.  dw2_setup must have been already called.  */
+
+static void
+load_cu (struct dwarf2_per_cu_data *per_cu)
+{
+  if (per_cu->from_debug_types)
+    read_signatured_type_at_offset (per_cu->objfile, per_cu->offset);
+  else
+    load_full_comp_unit (per_cu, per_cu->objfile);
+
+  dwarf2_find_base_address (per_cu->cu->dies, per_cu->cu);
+
+  gdb_assert (per_cu->cu != NULL);
+}
+
 /* Read in the symbols for PER_CU.  OBJFILE is the objfile from which
    this CU came.  */
 
@@ -1801,10 +1821,7 @@ dw2_do_instantiate_symtab (struct objfile *objfile,
 
   queue_comp_unit (per_cu, objfile);
 
-  if (per_cu->from_debug_types)
-    read_signatured_type_at_offset (objfile, per_cu->offset);
-  else
-    load_full_comp_unit (per_cu, objfile);
+  load_cu (per_cu);
 
   process_queue (objfile);
 
@@ -4714,8 +4731,6 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu)
 
   cu->list_in_scope = &file_symbols;
 
-  dwarf2_find_base_address (cu->dies, cu);
-
   /* Do line number decoding in read_file_scope () */
   process_die (cu->dies, cu);
 
@@ -13813,7 +13828,8 @@ follow_die_ref (struct die_info *src_die, struct attribute *attr,
 }
 
 /* Return DWARF block and its CU referenced by OFFSET at PER_CU.  Returned
-   value is intended for DW_OP_call*.  */
+   value is intended for DW_OP_call*.  You must call xfree on returned
+   dwarf2_locexpr_baton->data.  */
 
 struct dwarf2_locexpr_baton
 dwarf2_fetch_die_location_block (unsigned int offset,
@@ -13821,13 +13837,17 @@ dwarf2_fetch_die_location_block (unsigned int offset,
 				 CORE_ADDR (*get_frame_pc) (void *baton),
 				 void *baton)
 {
-  struct dwarf2_cu *cu = per_cu->cu;
+  struct dwarf2_cu *cu;
   struct die_info *die;
   struct attribute *attr;
   struct dwarf2_locexpr_baton retval;
 
   dw2_setup (per_cu->objfile);
 
+  if (per_cu->cu == NULL)
+    load_cu (per_cu);
+  cu = per_cu->cu;
+
   die = follow_die_offset (offset, &cu);
   if (!die)
     error (_("Dwarf Error: Cannot find DIE at 0x%x referenced in module %s"),
@@ -13864,6 +13884,12 @@ dwarf2_fetch_die_location_block (unsigned int offset,
       retval.size = DW_BLOCK (attr)->size;
     }
   retval.per_cu = cu->per_cu;
+
+  if (retval.data)
+    retval.data = xmemdup (retval.data, retval.size, retval.size);
+
+  age_cached_comp_units ();
+
   return retval;
 }
 
--- a/gdb/testsuite/gdb.dwarf2/dw2-op-call.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-op-call.exp
@@ -31,6 +31,9 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objdir}/${subdir}/${execu
 
 clean_restart $executable
 
+# Additional test to verify the referenced CU is not aged out.
+gdb_test_no_output "maintenance set dwarf2 max-cache-age 0"
+
 gdb_test "p array1" " = 1"
 gdb_test "p array2" " = 2" "array2 using DW_OP_call2"
 gdb_test "p array3" " = 3" "array3 using DW_OP_call4"
--- a/gdb/testsuite/gdb.dwarf2/implptr.exp
+++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
@@ -34,6 +34,9 @@ if {[prepare_for_testing ${testfile}.exp ${testfile}.x $srcfile]} {
     return -1
 }
 
+# Additional test to verify the referenced CU is not aged out.
+gdb_test_no_output "maintenance set dwarf2 max-cache-age 0"
+
 if ![runto_main] {
     return -1
 }

Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2

[Jan Kratochvil]

On Wed, 13 Jul 2011 16:27:59 +0200, Jan Kratochvil wrote:
> gdb/
> 2011-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix crash if referenced CU is aged out.
> 	* dwarf2loc.c (per_cu_dwarf_call): New variable back_to, use to for
> 	xfree of block.data.
> 	(indirect_pieced_value): New variable back_to, use to for xfree of
> 	baton.data.
> 	(dwarf2_compile_expr_to_ax): New variable back_to, use to for xfree of
> 	block.data.
> 	* dwarf2read.c (dwarf2_find_base_address): New prototype.
> 	(load_cu): New function from ...
> 	(dw2_do_instantiate_symtab): ... the code here ...
> 	(process_full_comp_unit): ... and here.
> 	(dwarf2_fetch_die_location_block): Call load_cu first.  Call xmemdup on
> 	retval.data.
> 
> gdb/testsuite/
> 2011-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.dwarf2/dw2-op-call.exp (maintenance set dwarf2 max-cache-age 0):
> 	New.
> 	* gdb.dwarf2/implptr.exp: Likewise.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-07/msg00166.html


Thanks,
Jan