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

[Doug Evans <dje@google.com>]

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".