* [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 @ 2010-08-23 18:50 Jan Kratochvil 2010-08-23 19:30 ` Doug Evans 0 siblings, 1 reply; 12+ messages in thread From: Jan Kratochvil @ 2010-08-23 18:50 UTC (permalink / raw) To: gdb-patches 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" ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-08-23 18:50 [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 Jan Kratochvil @ 2010-08-23 19:30 ` Doug Evans 2010-09-02 17:13 ` Jan Kratochvil 0 siblings, 1 reply; 12+ messages in thread From: Doug Evans @ 2010-08-23 19:30 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches 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". ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-08-23 19:30 ` Doug Evans @ 2010-09-02 17:13 ` Jan Kratochvil 2010-09-02 19:33 ` Doug Evans 2010-09-03 15:42 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 Tom Tromey 0 siblings, 2 replies; 12+ messages in thread From: Jan Kratochvil @ 2010-09-02 17:13 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-02 17:13 ` Jan Kratochvil @ 2010-09-02 19:33 ` Doug Evans 2011-07-13 15:21 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2 Jan Kratochvil 2010-09-03 15:42 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 Tom Tromey 1 sibling, 1 reply; 12+ messages in thread From: Doug Evans @ 2010-09-02 19:33 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2 2010-09-02 19:33 ` Doug Evans @ 2011-07-13 15:21 ` Jan Kratochvil 2011-07-19 20:55 ` Jan Kratochvil 0 siblings, 1 reply; 12+ messages in thread From: Jan Kratochvil @ 2011-07-13 15:21 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches 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 } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2 2011-07-13 15:21 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2 Jan Kratochvil @ 2011-07-19 20:55 ` Jan Kratochvil 0 siblings, 0 replies; 12+ messages in thread From: Jan Kratochvil @ 2011-07-19 20:55 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-02 17:13 ` Jan Kratochvil 2010-09-02 19:33 ` Doug Evans @ 2010-09-03 15:42 ` Tom Tromey 2010-09-03 16:14 ` Jan Kratochvil 1 sibling, 1 reply; 12+ messages in thread From: Tom Tromey @ 2010-09-03 15:42 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-03 15:42 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 Tom Tromey @ 2010-09-03 16:14 ` Jan Kratochvil 2010-09-03 18:06 ` Doug Evans 0 siblings, 1 reply; 12+ messages in thread From: Jan Kratochvil @ 2010-09-03 16:14 UTC (permalink / raw) To: Tom Tromey; +Cc: Doug Evans, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-03 16:14 ` Jan Kratochvil @ 2010-09-03 18:06 ` Doug Evans 2010-09-06 11:29 ` Jan Kratochvil 0 siblings, 1 reply; 12+ messages in thread From: Doug Evans @ 2010-09-03 18:06 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches 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.) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-03 18:06 ` Doug Evans @ 2010-09-06 11:29 ` Jan Kratochvil 2010-09-06 22:29 ` Doug Evans 2010-09-08 12:26 ` Tom Tromey 0 siblings, 2 replies; 12+ messages in thread From: Jan Kratochvil @ 2010-09-06 11:29 UTC (permalink / raw) To: Doug Evans; +Cc: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-06 11:29 ` Jan Kratochvil @ 2010-09-06 22:29 ` Doug Evans 2010-09-08 12:26 ` Tom Tromey 1 sibling, 0 replies; 12+ messages in thread From: Doug Evans @ 2010-09-06 22:29 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Tom Tromey, gdb-patches 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 2010-09-06 11:29 ` Jan Kratochvil 2010-09-06 22:29 ` Doug Evans @ 2010-09-08 12:26 ` Tom Tromey 1 sibling, 0 replies; 12+ messages in thread From: Tom Tromey @ 2010-09-08 12:26 UTC (permalink / raw) To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-07-19 20:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-08-23 18:50 [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 Jan Kratochvil 2010-08-23 19:30 ` Doug Evans 2010-09-02 17:13 ` Jan Kratochvil 2010-09-02 19:33 ` Doug Evans 2011-07-13 15:21 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 #2 Jan Kratochvil 2011-07-19 20:55 ` Jan Kratochvil 2010-09-03 15:42 ` [patch] Fix DW_OP_call2 and DW_OP_call4 for max-cache-age 0 Tom Tromey 2010-09-03 16:14 ` Jan Kratochvil 2010-09-03 18:06 ` Doug Evans 2010-09-06 11:29 ` Jan Kratochvil 2010-09-06 22:29 ` Doug Evans 2010-09-08 12:26 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).