This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Fix inconsistency in blockvector addrmap vs non-addrmap handling


On Mon, 25 Jun 2012 13:38:53 +0200, Doug Evans wrote:
> On Sun, Jun 24, 2012 at 11:33 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > --- a/gdb/dwarf2read.c
> > +++ b/gdb/dwarf2read.c
[...]
> > @@ -5820,7 +5821,15 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
> > Â Â Âit, by scanning the DIE's below the compilation unit. Â*/
> > Â get_scope_pc_bounds (cu->dies, &lowpc, &highpc, cu);
> >
> > - Âsymtab = end_symtab (highpc + baseaddr, objfile, SECT_OFF_TEXT (objfile));
> > + Âstatic_block = end_symtab_get_static_block (highpc + baseaddr, objfile);
> > +
> > + Â/* This CU may have discontiguous DW_AT_ranges and the CU may have addresses
> > + Â Â not belonging to any of its child DIEs (such as virtual method tables).
> > + Â Â Assign such addresses to STATIC_BLOCK's addrmap. Â*/
> > + Âdwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
> > +
> > + Âsymtab = end_symtab_from_static_block (static_block, objfile,
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂSECT_OFF_TEXT (objfile));
> >
> > Â if (symtab != NULL)
> > Â Â {
> 
> This is on the right track, but it's incomplete (IMO).
> 
> - Why call get_scope_pc_bounds if we're going to then explicitly
> attach DW_TAG_compile_unit's low_pc/high_pc/ranges attributes to
> STATIC_BLOCK? [we're processing these attributes twice, blech]

I wrongly considered DW_AT_ranges CUs to be rare enough to have to care about
their performance.

libc.so 81x DW_AT_ranges vs. 1291x DW_AT_high_pc
but libwebkitgtk.so 1874x DW_AT_ranges vs. 384x DW_AT_high_pc.

readelf -wi x.debug|perl -ne 'if(!/^  +</){$f=0;print $x;$x="";};$f=1 if/^ <0>/;$x.=$_ if $f;'|grep DW_AT_ranges

But I was looking at the code now and I do not find easy to implement
single-pass scan of DW_AT_ranges as you suggest.  We need HIGHPC already for
various issues before we get STATIC_BLOCK and only then we can start recording
the ranges for it.

I did not measure it but I do not find reading the several ranges of CU (if
any) to be too significant compared to the whole CU expansion.


> - If it does actually fix the bug in question it seems more accidental
> than on purpose.

I do not see why you think so.


>   - e.g. if the DW_TAG_compile_unit's DW_AT_ranges doesn't include some minsym,
>     what happens?

In such case process_psymtab_comp_unit_reader -> dwarf2_get_pc_bounds ->
-> dwarf2_ranges_read -> addrmap_set_empty is not called so no bug occurs as
nothing points to that CU at all.


> - If we really want to take this step then start,end on global,static
> blocks needs to go.

<abstract chat>
GDB needs some BLOCK_START / BLOCK_END abstraction for things like sorting
blocks and finding the most narrow block etc.

GDB needs some abstraction of the code.  For -O2 -g code in fact there are no
longer any source statements boundaries, there is no execution order etc.  The
abstraction of inferior for the purposes of GDB user interface will be
probably always broken in some way for -O2 -g code as for real -O2 -g code one
cannot simplify it more than what 'disas' shows.

This patch tries to exactly match process_psymtab_comp_unit_reader and
dw2_do_instantiate_symtab.  I call it a bugfix.  I do not try to redesign GDB
in this patch.

Whether BLOCK_END or BLOCK_START could be removed is more a GDB design change.
Currently it is a fallback for GDB code not yet prepared to use
BLOCKVECTOR_MAP.  As usual in GDB there are kept both the older and modern
ways how to do things as nobody wants to update all the code in GDB at once so
we could drop the old code.
</abstract chat>


>   Until then we're just satisfying a desire to apply dwarf correctly
> without really accomplishing anything.

I tried to exploit some case where the patch of mine behaves more correctly
than the patch of yours but I cannot.  Any code which accesses STATIC_BLOCK's
assigned ranges already checks first objfile->psymtabs_addrmap which already
has exactly correct CU's DW_AT_ranges map.

So I am really fine also with your patch:
	[RFA] Fix inconsistency in blockvector addrmap vs non-addrmap handling
	http://sourceware.org/ml/gdb-patches/2012-06/msg00109.html

although in such case there should be put a comment at both STATIC_BLOCK and
BLOCKVECTOR_MAP that it may cover excessive addresses and during its reading
one must intersect it with objfile->psymtabs_addrmap.  The code will be then
more both cpu and memory effective for the cost of this unclean data
structure.


As a summary:
	[RFA] Fix gdb segv in dw2_find_pc_sect_symtab
	http://sourceware.org/ml/gdb-patches/2012-05/msg00958.html
	= STATIC_BLOCK covers LESS addresses than objfile->psymtabs_addrmap.
 
	[RFA] Fix inconsistency in blockvector addrmap vs non-addrmap handling
	http://sourceware.org/ml/gdb-patches/2012-06/msg00109.html
	= STATIC_BLOCK covers MORE addresses than objfile->psymtabs_addrmap.

	Re: [RFA] Fix inconsistency in blockvector addrmap vs non-addrmap handling
	http://sourceware.org/ml/gdb-patches/2012-06/msg00756.html
	= STATIC_BLOCK covers EXACTLY the addresses of objfile->psymtabs_addrmap.


Thanks,
Jan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]