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

[Doug Evans <dje at google dot com>]

On Sun, Jun 24, 2012 at 11:33 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 22 Jun 2012 22:51:41 +0200, Doug Evans wrote:
>> But remember that the static block ?(or global block) doesn't exist
>> until the call to end_symtab, and, barring even more work, you need to
>> get the range into pending_addrmap before the call to make_blockvector
>> where pending_addrmap is turned into a fixed addrmap.
>
> One could use either callback for end_sym there or split end_sym (which I have
> chosen).
>
> Unfortunately I do not have time now to finish a proper testcase also
> exploiting the discontiguous CU so posting just the fix here. ?I should get to
> the testcase back on Tuesday. ?In fact I do not know if this fix works at all
> for the discontiguous CU case.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedorarawhide-linux-gnu against the
> patch:
> ? ? ? ?http://sourceware.org/ml/gdb-patches/2012-06/msg00109.html
> ? ? ? ?Subject: [RFA] Fix inconsistency in blockvector addrmap vs non-addrmap handling
>
>
> Thanks,
> Jan
>
>
> gdb/
> 2012-06-24 ?Jan Kratochvil ?<jan.kratochvil@redhat.com>
>
> ? ? ? ?* buildsym.c (end_symtab): Split it to ...
> ? ? ? ?(end_symtab_get_static_block): ... this ...
> ? ? ? ?(end_symtab_from_static_block): ... and this function.
> ? ? ? ?(end_symtab): New function.
> ? ? ? ?* buildsym.h (end_symtab_get_static_block)
> ? ? ? ?(end_symtab_from_static_block): New declarations.
> ? ? ? ?* dwarf2read.c (process_full_comp_unit): New variable static_block.
> ? ? ? ?Set its valid CU ranges.
>
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index f1fb4be..2de2ef8 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -923,38 +923,20 @@ block_compar (const void *ap, const void *bp)
> ? ? ? ? ?- (BLOCK_START (b) < BLOCK_START (a)));
> ?}
>
> -/* Finish the symbol definitions for one main source file, close off
> - ? all the lexical contexts for that file (creating struct block's for
> - ? them), then make the struct symtab for that file and put it in the
> - ? list of all such.
> -
> - ? END_ADDR is the address of the end of the file's text. ?SECTION is
> - ? the section number (in objfile->section_offsets) of the blockvector
> - ? and linetable.
> +/* Implementation of the first part of end_symtab. ?It allows modifying
> + ? STATIC_BLOCK before it gets finalized by end_symtab_from_static_block.
> + ? If the returned value is NULL there is no blockvector created for
> + ? this symtab (you still must call end_symtab_from_static_block). ?*/
>
> - ? Note that it is possible for end_symtab() to return NULL. ?In
> - ? particular, for the DWARF case at least, it will return NULL when
> - ? it finds a compilation unit that has exactly one DIE, a
> - ? TAG_compile_unit DIE. ?This can happen when we link in an object
> - ? file that was compiled from an empty source file. ?Returning NULL
> - ? is probably not the correct thing to do, because then gdb will
> - ? never know about this empty file (FIXME). ?*/
> -
> -struct symtab *
> -end_symtab (CORE_ADDR end_addr, struct objfile *objfile, int section)
> +struct block *
> +end_symtab_get_static_block (CORE_ADDR end_addr, struct objfile *objfile)
> ?{
> - ?struct symtab *symtab = NULL;
> - ?struct blockvector *blockvector;
> - ?struct subfile *subfile;
> - ?struct context_stack *cstk;
> - ?struct subfile *nextsub;
> -
> ? /* Finish the lexical context of the last function in the file; pop
> ? ? ?the context stack. ?*/
>
> ? if (context_stack_depth > 0)
> ? ? {
> - ? ? ?cstk = pop_context ();
> + ? ? ?struct context_stack *cstk = pop_context ();
> ? ? ? /* Make a block for the local symbols within. ?*/
> ? ? ? finish_block (cstk->name, &local_symbols, cstk->old_blocks,
> ? ? ? ? ? ? ? ? ? ?cstk->start_addr, end_addr, objfile);
> @@ -1021,15 +1003,41 @@ end_symtab (CORE_ADDR end_addr, struct objfile *objfile, int section)
> ? ? {
> ? ? ? /* Ignore symtabs that have no functions with real debugging
> ? ? ? ? ?info. ?*/
> + ? ? ?return NULL;
> + ? ?}
> + ?else
> + ? ?{
> + ? ? ?/* Define the STATIC_BLOCK. ?*/
> + ? ? ?return finish_block (NULL, &file_symbols, NULL, last_source_start_addr,
> + ? ? ? ? ? ? ? ? ? ? ? ? ?end_addr, objfile);
> + ? ?}
> +}
> +
> +/* Implementation of the second part of end_symtab. ?Pass STATIC_BLOCK
> + ? as value returned by end_symtab_get_static_block. ?*/
> +
> +struct symtab *
> +end_symtab_from_static_block (struct block *static_block,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct objfile *objfile, int section)
> +{
> + ?struct symtab *symtab = NULL;
> + ?struct blockvector *blockvector;
> + ?struct subfile *subfile;
> + ?struct subfile *nextsub;
> +
> + ?if (static_block == NULL)
> + ? ?{
> + ? ? ?/* Ignore symtabs that have no functions with real debugging
> + ? ? ? ? info. ?*/
> ? ? ? blockvector = NULL;
> ? ? }
> ? else
> ? ? {
> - ? ? ?/* Define the STATIC_BLOCK & GLOBAL_BLOCK, and build the
> + ? ? ?CORE_ADDR end_addr = BLOCK_END (static_block);
> +
> + ? ? ?/* Define after STATIC_BLOCK also GLOBAL_BLOCK, and build the
> ? ? ? ? ?blockvector. ?*/
> - ? ? ?finish_block (0, &file_symbols, 0, last_source_start_addr,
> - ? ? ? ? ? ? ? ? ? end_addr, objfile);
> - ? ? ?finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
> + ? ? ?finish_block_internal (NULL, &global_symbols, NULL, last_source_start_addr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? end_addr, objfile, 1);
> ? ? ? blockvector = make_blockvector (objfile);
> ? ? }
> @@ -1219,6 +1227,36 @@ end_symtab (CORE_ADDR end_addr, struct objfile *objfile, int section)
> ? return symtab;
> ?}
>
> +/* Finish the symbol definitions for one main source file, close off
> + ? all the lexical contexts for that file (creating struct block's for
> + ? them), then make the struct symtab for that file and put it in the
> + ? list of all such.
> +
> + ? END_ADDR is the address of the end of the file's text. ?SECTION is
> + ? the section number (in objfile->section_offsets) of the blockvector
> + ? and linetable.
> +
> + ? Note that it is possible for end_symtab() to return NULL. ?In
> + ? particular, for the DWARF case at least, it will return NULL when
> + ? it finds a compilation unit that has exactly one DIE, a
> + ? TAG_compile_unit DIE. ?This can happen when we link in an object
> + ? file that was compiled from an empty source file. ?Returning NULL
> + ? is probably not the correct thing to do, because then gdb will
> + ? never know about this empty file (FIXME).
> +
> + ? If you need to modify STATIC_BLOCK before it is finalized you should
> + ? call end_symtab_get_static_block and end_symtab_from_static_block
> + ? yourself. ?*/
> +
> +struct symtab *
> +end_symtab (CORE_ADDR end_addr, struct objfile *objfile, int section)
> +{
> + ?struct block *static_block;
> +
> + ?static_block = end_symtab_get_static_block (end_addr, objfile);
> + ?return end_symtab_from_static_block (static_block, objfile, section);
> +}
> +
> ?/* Push a context block. ?Args are an identifying nesting level
> ? ?(checkable when you pop it), and the starting PC address of this
> ? ?context. ?*/
> diff --git a/gdb/buildsym.h b/gdb/buildsym.h
> index 4448267..89324e9 100644
> --- a/gdb/buildsym.h
> +++ b/gdb/buildsym.h
> @@ -260,6 +260,11 @@ extern char *pop_subfile (void);
>
> ?extern struct symtab *end_symtab (CORE_ADDR end_addr,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct objfile *objfile, int section);
> +extern struct block *end_symtab_get_static_block (CORE_ADDR end_addr,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct objfile *objfile);
> +extern struct symtab *end_symtab_from_static_block (struct block *static_block,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct objfile *objfile,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int section);
>
> ?/* Defined in stabsread.c. ?*/
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index aa42b4c..62119ad 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -5790,6 +5790,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
> ? struct symtab *symtab;
> ? struct cleanup *back_to, *delayed_list_cleanup;
> ? CORE_ADDR baseaddr;
> + ?struct block *static_block;
>
> ? baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>
> @@ -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]

- If it does actually fix the bug in question it seems more accidental
than on purpose.
  - e.g. if the DW_TAG_compile_unit's DW_AT_ranges doesn't include some minsym,
    what happens?

- If we really want to take this step then start,end on global,static
blocks needs to go.
  Until then we're just satisfying a desire to apply dwarf correctly
without really accomplishing anything.