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

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

[Doug Evans]

Hi.

I was seeing the assert in dw2_find_pc_sect_psymtab trigger
and traced it to the fact that when pending_addrmap_interesting gets
set blockvector.map is used instead of blockvector.block.
The difference is that blockvector.block contains entries for the global
and static blocks whereas pending_addrmap doesn't.

This patch fixes this by making them consistent.
I suspect more work is necessary (e.g. can symtabs "overlap" even though
the individual pieces do not?).
But I first want to fix the regression introduced by the change
to dw2_find_pc_sect_psymtab: There is more code in a symtab than is
documented by function and lexical block pc ranges (e.g. C++ method thunks).

Regression tested on amd64-linux, and by verifying the assert no longer
triggers in the testcase I was using.

Ok to commit?

Note that this obviates the need for the patch in:
http://sourceware.org/ml/gdb-patches/2012-05/msg00958.html

Also note that this accompanies this patch:
http://sourceware.org/ml/gdb-patches/2012-06/msg00105.html

2012-06-04  Doug Evans  <dje@google.com>

	* buildsym.c (end_symtab): Add the range of the static block to
	the pending addrmap.

Index: buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.97
diff -u -p -r1.97 buildsym.c
--- buildsym.c	29 May 2012 20:23:17 -0000	1.97
+++ buildsym.c	5 Jun 2012 00:26:01 -0000
@@ -1024,8 +1027,15 @@ end_symtab (CORE_ADDR end_addr, struct o
     {
       /* Define the STATIC_BLOCK & GLOBAL_BLOCK, and build the
          blockvector.  */
-      finish_block (0, &file_symbols, 0, last_source_start_addr,
-		    end_addr, objfile);
+      struct block *static_block;
+
+      static_block = finish_block (0, &file_symbols, 0,
+				   last_source_start_addr, end_addr,
+				   objfile);
+      /* Mark the range of the static block so that if we end up using
+	 blockvector.map then find_block_in_blockvector behaves identically
+	 regardless of whether the addrmap is present.  */
+      record_block_range (static_block, last_source_start_addr, end_addr - 1);
       finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
 			     end_addr, objfile, 1);
       blockvector = make_blockvector (objfile);

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

[Pedro Alves]

On 06/05/2012 02:14 AM, Doug Evans wrote:

> Hi.
> 
> I was seeing the assert in dw2_find_pc_sect_psymtab trigger
> and traced it to the fact that when pending_addrmap_interesting gets
> set blockvector.map is used instead of blockvector.block.
> The difference is that blockvector.block contains entries for the global
> and static blocks whereas pending_addrmap doesn't.
> 
> This patch fixes this by making them consistent.
> I suspect more work is necessary (e.g. can symtabs "overlap" even though
> the individual pieces do not?).


Yeah, I suppose it could be conceivable, say with __attribute__ section
and/or linker scripts.

> But I first want to fix the regression introduced by the change
> to dw2_find_pc_sect_psymtab: There is more code in a symtab than is
> documented by function and lexical block pc ranges (e.g. C++ method thunks).
> 
> Regression tested on amd64-linux, and by verifying the assert no longer
> triggers in the testcase I was using.
> 
> Ok to commit?


FWIW, looks fine to me, but I don't really know all the ins and outs
of the symbol stuff.

> 
> Note that this obviates the need for the patch in:
> http://sourceware.org/ml/gdb-patches/2012-05/msg00958.html
> 
> Also note that this accompanies this patch:
> http://sourceware.org/ml/gdb-patches/2012-06/msg00105.html
> 
> 2012-06-04  Doug Evans  <dje@google.com>
> 
> 	* buildsym.c (end_symtab): Add the range of the static block to
> 	the pending addrmap.
> 
> Index: buildsym.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/buildsym.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 buildsym.c
> --- buildsym.c	29 May 2012 20:23:17 -0000	1.97
> +++ buildsym.c	5 Jun 2012 00:26:01 -0000
> @@ -1024,8 +1027,15 @@ end_symtab (CORE_ADDR end_addr, struct o
>      {
>        /* Define the STATIC_BLOCK & GLOBAL_BLOCK, and build the
>           blockvector.  */
> -      finish_block (0, &file_symbols, 0, last_source_start_addr,
> -		    end_addr, objfile);
> +      struct block *static_block;
> +
> +      static_block = finish_block (0, &file_symbols, 0,
> +				   last_source_start_addr, end_addr,
> +				   objfile);
> +      /* Mark the range of the static block so that if we end up using
> +	 blockvector.map then find_block_in_blockvector behaves identically
> +	 regardless of whether the addrmap is present.  */
> +      record_block_range (static_block, last_source_start_addr, end_addr - 1);
>        finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
>  			     end_addr, objfile, 1);
>        blockvector = make_blockvector (objfile);


-- 
Pedro Alves

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

[Jan Kratochvil]

On Tue, 05 Jun 2012 03:14:46 +0200, Doug Evans wrote:
> 2012-06-04  Doug Evans  <dje@google.com>
> 
> 	* buildsym.c (end_symtab): Add the range of the static block to
> 	the pending addrmap.
> 
> Index: buildsym.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/buildsym.c,v
> retrieving revision 1.97
> diff -u -p -r1.97 buildsym.c
> --- buildsym.c	29 May 2012 20:23:17 -0000	1.97
> +++ buildsym.c	5 Jun 2012 00:26:01 -0000
> @@ -1024,8 +1027,15 @@ end_symtab (CORE_ADDR end_addr, struct o
>      {
>        /* Define the STATIC_BLOCK & GLOBAL_BLOCK, and build the
>           blockvector.  */
> -      finish_block (0, &file_symbols, 0, last_source_start_addr,
> -		    end_addr, objfile);
> +      struct block *static_block;
> +
> +      static_block = finish_block (0, &file_symbols, 0,
> +				   last_source_start_addr, end_addr,
> +				   objfile);
> +      /* Mark the range of the static block so that if we end up using
> +	 blockvector.map then find_block_in_blockvector behaves identically
> +	 regardless of whether the addrmap is present.  */
> +      record_block_range (static_block, last_source_start_addr, end_addr - 1);
>        finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
>  			     end_addr, objfile, 1);

I am fine with the patch.  FYI I would extend the comment a bit:

      /* Mark the range of the static block so that if we end up using
	 blockvector.map then find_block_in_blockvector behaves identically
	 regardless of whether the addrmap is present.  That means it returns
	 this symtab even for addresses covered by compilation unit ranges but
	 not belonging to any symbol in this symtab.

	 STATIC_BLOCK is more inner than GLOBAL_BLOCK, this is the only reason
	 why choose it for LAST_SOURCE_START_ADDR..END_ADDR.  */


>        blockvector = make_blockvector (objfile);


Thanks,
Jan

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

[Jan Kratochvil]

On Tue, 05 Jun 2012 03:14:46 +0200, Doug Evans wrote:
> I was seeing the assert in dw2_find_pc_sect_psymtab trigger

Loaded symbols for /lib64/ld-linux-x86-64.so.2
dwarf2read.c:3087: internal-error: dw2_find_pc_sect_symtab: Assertion `result != NULL' failed.

FSF GDB is unusable on Fedora Rawhide, this patch is pending for some time,
could it be checked in for gdb-7.5?


Thanks,
Jan

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

[Jan Kratochvil]

On Tue, 05 Jun 2012 03:14:46 +0200, Doug Evans wrote:
> --- buildsym.c	29 May 2012 20:23:17 -0000	1.97
> +++ buildsym.c	5 Jun 2012 00:26:01 -0000
> @@ -1024,8 +1027,15 @@ end_symtab (CORE_ADDR end_addr, struct o
>      {
>        /* Define the STATIC_BLOCK & GLOBAL_BLOCK, and build the
>           blockvector.  */
> -      finish_block (0, &file_symbols, 0, last_source_start_addr,
> -		    end_addr, objfile);
> +      struct block *static_block;
> +
> +      static_block = finish_block (0, &file_symbols, 0,
> +				   last_source_start_addr, end_addr,
> +				   objfile);
> +      /* Mark the range of the static block so that if we end up using
> +	 blockvector.map then find_block_in_blockvector behaves identically
> +	 regardless of whether the addrmap is present.  */
> +      record_block_range (static_block, last_source_start_addr, end_addr - 1);

On IRC Doug made a note:
	Arguably the second is the better fix but it's still a hack as
	addrmaps are intended to handle discontiguous symtabs and this defeats
	that.

Where "the first fix" was:
	[RFA] Fix gdb segv in dw2_find_pc_sect_symtab
	http://sourceware.org/ml/gdb-patches/2012-05/msg00958.html

I think the right way is to call dwarf2_record_block_ranges for
DW_AT_compilation_unit but I haven't tried to write such patch yet, is there
a problem?


>        finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
>  			     end_addr, objfile, 1);
>        blockvector = make_blockvector (objfile);


Thanks,
Jan

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

[Doug Evans]

On Fri, Jun 22, 2012 at 12:35 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Tue, 05 Jun 2012 03:14:46 +0200, Doug Evans wrote:
>> --- buildsym.c        29 May 2012 20:23:17 -0000      1.97
>> +++ buildsym.c        5 Jun 2012 00:26:01 -0000
>> @@ -1024,8 +1027,15 @@ end_symtab (CORE_ADDR end_addr, struct o
>>      {
>>        /* Define the STATIC_BLOCK & GLOBAL_BLOCK, and build the
>>           blockvector.  */
>> -      finish_block (0, &file_symbols, 0, last_source_start_addr,
>> -                 end_addr, objfile);
>> +      struct block *static_block;
>> +
>> +      static_block = finish_block (0, &file_symbols, 0,
>> +                                last_source_start_addr, end_addr,
>> +                                objfile);
>> +      /* Mark the range of the static block so that if we end up using
>> +      blockvector.map then find_block_in_blockvector behaves identically
>> +      regardless of whether the addrmap is present.  */
>> +      record_block_range (static_block, last_source_start_addr, end_addr - 1);
>
> On IRC Doug made a note:
>        Arguably the second is the better fix but it's still a hack as
>        addrmaps are intended to handle discontiguous symtabs and this defeats
>        that.
>
> Where "the first fix" was:
>        [RFA] Fix gdb segv in dw2_find_pc_sect_symtab
>        http://sourceware.org/ml/gdb-patches/2012-05/msg00958.html
>
> I think the right way is to call dwarf2_record_block_ranges for
> DW_AT_compilation_unit but I haven't tried to write such patch yet, is there
> a problem?
>
>
>>        finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
>>                            end_addr, objfile, 1);
>>        blockvector = make_blockvector (objfile);

With some work that could be made to work.
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.

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

[Jan Kratochvil]

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)
     {

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

[Doug Evans]

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.

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

[Jan Kratochvil]

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

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

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

On Tue, Jul 3, 2012 at 5:54 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
>> - If it does actually fix the bug in question it seems more accidental
>> than on purpose.
>
> I do not see why you think so.

I think I was confused about something.

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

Note that I was specifically referring to GLOBAL and STATIC blocks.
There the concept of having a start and end has lost its meaning.
They have a lower bound and an upper bound which may be useful in
sorting, but using their start/end values as start/end values is
problematic.

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

I was playing with a case of "pc 0x42 in read in psymtab, but not in
symtab" case today.
gdb has other problems that get in the way, but having more
consistency between partial and full syms has to be a good thing.
[btw, seems kinda odd to have two addrmaps: one for psymtabs/index and
one for fullsyms.  I wonder if there's an opportunity here.]

Here's an updated version of your patch to match the current tree.

2012-07-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
            Doug Evans  <dje@google.com>

        * buildsym.c (end_symtab_1): Split it to ...
        (end_symtab_get_static_block): ... this ...
        (end_symtab_from_static_block): ... and this function.
        (end_symtab, end_expandable_symtab): Call them.
        * 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.

[-- Attachment #2: gdb-120711-static-block-ranges-1.patch.txt --]
[-- Type: text/plain, Size: 9214 bytes --]

2012-07-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Doug Evans  <dje@google.com>

	* buildsym.c (end_symtab_1): Split it to ...
	(end_symtab_get_static_block): ... this ...
	(end_symtab_from_static_block): ... and this function.
	(end_symtab, end_expandable_symtab): Call them.
	* 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.

Index: buildsym.c
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.c,v
retrieving revision 1.100
diff -u -p -r1.100 buildsym.c
--- buildsym.c	10 Jul 2012 20:20:15 -0000	1.100
+++ buildsym.c	12 Jul 2012 04:56:06 -0000
@@ -957,42 +957,28 @@ reset_symtab_globals (void)
     }
 }
 
-/* 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.
-
-   If EXPANDABLE is non-zero the dictionaries for the global and static
-   blocks are made expandable.
-
-   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).  */
-
-static struct symtab *
-end_symtab_1 (CORE_ADDR end_addr, struct objfile *objfile, int section,
-	      int expandable)
+/* 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).
+
+   END_ADDR is the same as for end_symtab: the address of the end of the
+   file's text.
+
+   If EXPANDABLE is non-zero the STATIC_BLOCK dictionary is made
+   expandable.  */
+
+struct block *
+end_symtab_get_static_block (CORE_ADDR end_addr, struct objfile *objfile,
+			     int expandable)
 {
-  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);
@@ -1058,18 +1044,51 @@ end_symtab_1 (CORE_ADDR end_addr, struct
       && have_line_numbers == 0
       && pending_macros == NULL)
     {
-      /* Ignore symtabs that have no functions with real debugging
-         info.  */
+      /* Ignore symtabs that have no functions with real debugging info.  */
+      return NULL;
+    }
+  else
+    {
+      /* Define the STATIC_BLOCK.  */
+      return finish_block_internal (NULL, &file_symbols, NULL,
+				    last_source_start_addr, end_addr, objfile,
+				    0, expandable);
+    }
+}
+
+/* Implementation of the second part of end_symtab.  Pass STATIC_BLOCK
+   as value returned by end_symtab_get_static_block.
+
+   SECTION is the same as for end_symtab: the section number
+   (in objfile->section_offsets) of the blockvector and linetable.
+
+   If EXPANDABLE is non-zero the GLOBAL_BLOCK dictionary is made
+   expandable.  */
+
+struct symtab *
+end_symtab_from_static_block (struct block *static_block,
+			      struct objfile *objfile, int section,
+			      int expandable)
+{
+  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_internal (0, &file_symbols, 0, last_source_start_addr,
-			     end_addr, objfile, 0, expandable);
-      finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
-			     end_addr, objfile, 1, expandable);
+      finish_block_internal (NULL, &global_symbols, NULL,
+			     last_source_start_addr, end_addr, objfile,
+			     1, expandable);
       blockvector = make_blockvector (objfile);
     }
 
@@ -1251,21 +1270,46 @@ end_symtab_1 (CORE_ADDR end_addr, struct
   return symtab;
 }
 
-/* See end_symtab_1 for details.  */
+/* 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)
 {
-  return end_symtab_1 (end_addr, objfile, section, 0);
+  struct block *static_block;
+
+  static_block = end_symtab_get_static_block (end_addr, objfile, 0);
+  return end_symtab_from_static_block (static_block, objfile, section, 0);
 }
 
-/* See end_symtab_1 for details.  */
+/* Same as end_symtab except create a symtab that can be later added to.  */
 
 struct symtab *
 end_expandable_symtab (CORE_ADDR end_addr, struct objfile *objfile,
 		       int section)
 {
-  return end_symtab_1 (end_addr, objfile, section, 1);
+  struct block *static_block;
+
+  static_block = end_symtab_get_static_block (end_addr, objfile, 1);
+  return end_symtab_from_static_block (static_block, objfile, section, 1);
 }
 
 /* Subroutine of augment_type_symtab to simplify it.
Index: buildsym.h
===================================================================
RCS file: /cvs/src/src/gdb/buildsym.h,v
retrieving revision 1.33
diff -u -p -r1.33 buildsym.h
--- buildsym.h	10 Jul 2012 20:20:15 -0000	1.33
+++ buildsym.h	12 Jul 2012 04:56:06 -0000
@@ -258,6 +258,15 @@ extern void push_subfile (void);
 
 extern char *pop_subfile (void);
 
+extern struct block *end_symtab_get_static_block (CORE_ADDR end_addr,
+						  struct objfile *objfile,
+						  int expandable);
+
+extern struct symtab *end_symtab_from_static_block (struct block *static_block,
+						    struct objfile *objfile,
+						    int section,
+						    int expandable);
+
 extern struct symtab *end_symtab (CORE_ADDR end_addr,
 				  struct objfile *objfile, int section);
 
Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.684
diff -u -p -r1.684 dwarf2read.c
--- dwarf2read.c	10 Jul 2012 20:28:32 -0000	1.684
+++ dwarf2read.c	12 Jul 2012 04:56:07 -0000
@@ -6593,6 +6593,7 @@ process_full_comp_unit (struct dwarf2_pe
   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));
 
@@ -6623,7 +6624,17 @@ process_full_comp_unit (struct dwarf2_pe
      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, 0);
+
+  /* If the comp unit has DW_AT_ranges, it may have discontiguous ranges.
+     Also, DW_AT_ranges may record ranges not belonging to any child DIEs
+     (such as virtual method tables).  Record the ranges in STATIC_BLOCK's
+     addrmap to help ensure it has an accurate map of pc values belonging to
+     this comp unit.  */
+  dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
+
+  symtab = end_symtab_from_static_block (static_block, objfile,
+					 SECT_OFF_TEXT (objfile), 0);
 
   if (symtab != NULL)
     {

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

[Jan Kratochvil]

On Thu, 12 Jul 2012 07:19:30 +0200, Doug Evans wrote:
> I was playing with a case of "pc 0x42 in read in psymtab, but not in
> symtab" case today.
> gdb has other problems that get in the way, but having more
> consistency between partial and full syms has to be a good thing.

Therefore considering it as an agreement/approval of the patch.

Therefore I will check it in tomorrow with no reply.


> [btw, seems kinda odd to have two addrmaps: one for psymtabs/index and
> one for fullsyms.  I wonder if there's an opportunity here.]
> 
> Here's an updated version of your patch to match the current tree.
> 
> 2012-07-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
>             Doug Evans  <dje@google.com>
> 
>         * buildsym.c (end_symtab_1): Split it to ...
>         (end_symtab_get_static_block): ... this ...
>         (end_symtab_from_static_block): ... and this function.
>         (end_symtab, end_expandable_symtab): Call them.
>         * 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.


Thanks,
Jan

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

[Jan Kratochvil]

On Thu, 12 Jul 2012 22:30:23 +0200, Jan Kratochvil wrote:
> Therefore I will check it in tomorrow with no reply.

Checked in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2012-07/msg00096.html

--- src/gdb/ChangeLog	2012/07/13 20:15:50	1.14462
+++ src/gdb/ChangeLog	2012/07/13 20:26:09	1.14463
@@ -1,4 +1,16 @@
 2012-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Doug Evans  <dje@google.com>
+
+	* buildsym.c (end_symtab_1): Split it to ...
+	(end_symtab_get_static_block): ... this ...
+	(end_symtab_from_static_block): ... and this function.
+	(end_symtab, end_expandable_symtab): Call them.
+	* 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.
+
+2012-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* dwarf2loc.c (disassemble_dwarf_expression): Handle
 	DW_OP_GNU_parameter_ref.
--- src/gdb/testsuite/ChangeLog	2012/07/13 08:14:36	1.3291
+++ src/gdb/testsuite/ChangeLog	2012/07/13 20:26:10	1.3292
@@ -1,4 +1,10 @@
 2012-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Doug Evans  <dje@google.com>
+
+	* gdb.dwarf2/dw2-minsym-in-cu.S: New file.
+	* gdb.dwarf2/dw2-minsym-in-cu.exp: New file.
+
+2012-07-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	Fix gdbserver run regression.
 	* gdb.trace/disconnected-tracing.exp (executabel): Fix typo.
--- src/gdb/buildsym.c	2012/07/10 20:20:15	1.100
+++ src/gdb/buildsym.c	2012/07/13 20:26:10	1.101
@@ -957,42 +957,28 @@
     }
 }
 
-/* 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.
-
-   If EXPANDABLE is non-zero the dictionaries for the global and static
-   blocks are made expandable.
-
-   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).  */
-
-static struct symtab *
-end_symtab_1 (CORE_ADDR end_addr, struct objfile *objfile, int section,
-	      int expandable)
+/* 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).
+
+   END_ADDR is the same as for end_symtab: the address of the end of the
+   file's text.
+
+   If EXPANDABLE is non-zero the STATIC_BLOCK dictionary is made
+   expandable.  */
+
+struct block *
+end_symtab_get_static_block (CORE_ADDR end_addr, struct objfile *objfile,
+			     int expandable)
 {
-  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);
@@ -1058,18 +1044,51 @@
       && have_line_numbers == 0
       && pending_macros == NULL)
     {
-      /* Ignore symtabs that have no functions with real debugging
-         info.  */
+      /* Ignore symtabs that have no functions with real debugging info.  */
+      return NULL;
+    }
+  else
+    {
+      /* Define the STATIC_BLOCK.  */
+      return finish_block_internal (NULL, &file_symbols, NULL,
+				    last_source_start_addr, end_addr, objfile,
+				    0, expandable);
+    }
+}
+
+/* Implementation of the second part of end_symtab.  Pass STATIC_BLOCK
+   as value returned by end_symtab_get_static_block.
+
+   SECTION is the same as for end_symtab: the section number
+   (in objfile->section_offsets) of the blockvector and linetable.
+
+   If EXPANDABLE is non-zero the GLOBAL_BLOCK dictionary is made
+   expandable.  */
+
+struct symtab *
+end_symtab_from_static_block (struct block *static_block,
+			      struct objfile *objfile, int section,
+			      int expandable)
+{
+  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_internal (0, &file_symbols, 0, last_source_start_addr,
-			     end_addr, objfile, 0, expandable);
-      finish_block_internal (0, &global_symbols, 0, last_source_start_addr,
-			     end_addr, objfile, 1, expandable);
+      finish_block_internal (NULL, &global_symbols, NULL,
+			     last_source_start_addr, end_addr, objfile,
+			     1, expandable);
       blockvector = make_blockvector (objfile);
     }
 
@@ -1251,21 +1270,46 @@
   return symtab;
 }
 
-/* See end_symtab_1 for details.  */
+/* 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)
 {
-  return end_symtab_1 (end_addr, objfile, section, 0);
+  struct block *static_block;
+
+  static_block = end_symtab_get_static_block (end_addr, objfile, 0);
+  return end_symtab_from_static_block (static_block, objfile, section, 0);
 }
 
-/* See end_symtab_1 for details.  */
+/* Same as end_symtab except create a symtab that can be later added to.  */
 
 struct symtab *
 end_expandable_symtab (CORE_ADDR end_addr, struct objfile *objfile,
 		       int section)
 {
-  return end_symtab_1 (end_addr, objfile, section, 1);
+  struct block *static_block;
+
+  static_block = end_symtab_get_static_block (end_addr, objfile, 1);
+  return end_symtab_from_static_block (static_block, objfile, section, 1);
 }
 
 /* Subroutine of augment_type_symtab to simplify it.
--- src/gdb/buildsym.h	2012/07/10 20:20:15	1.33
+++ src/gdb/buildsym.h	2012/07/13 20:26:10	1.34
@@ -258,6 +258,15 @@
 
 extern char *pop_subfile (void);
 
+extern struct block *end_symtab_get_static_block (CORE_ADDR end_addr,
+						  struct objfile *objfile,
+						  int expandable);
+
+extern struct symtab *end_symtab_from_static_block (struct block *static_block,
+						    struct objfile *objfile,
+						    int section,
+						    int expandable);
+
 extern struct symtab *end_symtab (CORE_ADDR end_addr,
 				  struct objfile *objfile, int section);
 
--- src/gdb/dwarf2read.c	2012/07/13 20:15:03	1.685
+++ src/gdb/dwarf2read.c	2012/07/13 20:26:10	1.686
@@ -6593,6 +6593,7 @@
   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));
 
@@ -6623,7 +6624,17 @@
      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, 0);
+
+  /* If the comp unit has DW_AT_ranges, it may have discontiguous ranges.
+     Also, DW_AT_ranges may record ranges not belonging to any child DIEs
+     (such as virtual method tables).  Record the ranges in STATIC_BLOCK's
+     addrmap to help ensure it has an accurate map of pc values belonging to
+     this comp unit.  */
+  dwarf2_record_block_ranges (cu->dies, static_block, baseaddr, cu);
+
+  symtab = end_symtab_from_static_block (static_block, objfile,
+					 SECT_OFF_TEXT (objfile), 0);
 
   if (symtab != NULL)
     {
--- src/gdb/testsuite/gdb.dwarf2/dw2-minsym-in-cu.S
+++ src/gdb/testsuite/gdb.dwarf2/dw2-minsym-in-cu.S	2012-07-13 20:26:31.658295000 +0000
@@ -0,0 +1,108 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004, 2007-2012 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.text
+.Lbegin_text1:
+
+	.globl main
+	.type main, %function
+main:
+.Lbegin_main:
+	.int 0
+.Lend_main:
+	.size main, .-main
+
+	.globl func2
+	.type func2, %function
+func2:
+	.int 0
+	.size func2, .-func2
+
+.Lend_text1:
+
+/* Debug information */
+
+	.section .debug_info
+.Lcu1_begin:
+	/* CU header */
+	.4byte	.Lcu1_end - .Lcu1_start		/* Length of Compilation Unit */
+.Lcu1_start:
+	.2byte	2				/* DWARF Version */
+	.4byte	.Labbrev1_begin			/* Offset into abbrev section */
+	.byte	4				/* Pointer size */
+
+	/* CU die */
+	.uleb128 1				/* Abbrev: DW_TAG_compile_unit */
+	.4byte	.Lend_text1			/* DW_AT_high_pc */
+	.4byte	.Lbegin_text1			/* DW_AT_low_pc */
+	.ascii	"file1.txt\0"			/* DW_AT_name */
+	.ascii	"GNU C 3.3.3\0"			/* DW_AT_producer */
+	.byte	1				/* DW_AT_language (C) */
+
+	/* main */
+	.uleb128	2			/* Abbrev: DW_TAG_subprogram */
+	.byte		1			/* DW_AT_external */
+	.ascii		"main\0"		/* DW_AT_name */
+	.4byte	.Ldebug_ranges			/* DW_AT_ranges */
+
+	.byte		0			/* End of children of CU */
+.Lcu1_end:
+
+/* DW_AT_ranges.  */
+	.section	.debug_ranges
+.Ldebug_ranges:
+	.4byte	.Lbegin_main
+	.4byte	.Lend_main - 1
+	/* Make it slightly more interesting to set pending_addrmap_interesting.  */
+	.4byte	.Lend_main - 1
+	.4byte	.Lend_main
+	.4byte	0
+	.4byte	0
+
+/* Abbrev table */
+	.section .debug_abbrev
+.Labbrev1_begin:
+	.uleb128	1			/* Abbrev code */
+	.uleb128	0x11			/* DW_TAG_compile_unit */
+	.byte		1			/* has_children */
+	.uleb128	0x12			/* DW_AT_high_pc */
+	.uleb128	0x1			/* DW_FORM_addr */
+	.uleb128	0x11			/* DW_AT_low_pc */
+	.uleb128	0x1			/* DW_FORM_addr */
+	.uleb128	0x3			/* DW_AT_name */
+	.uleb128	0x8			/* DW_FORM_string */
+	.uleb128	0x25			/* DW_AT_producer */
+	.uleb128	0x8			/* DW_FORM_string */
+	.uleb128	0x13			/* DW_AT_language */
+	.uleb128	0xb			/* DW_FORM_data1 */
+	.byte		0x0			/* Terminator */
+	.byte		0x0			/* Terminator */
+
+	.uleb128	2			/* Abbrev code */
+	.uleb128	0x2e			/* DW_TAG_subprogram */
+	.byte		0			/* has_children */
+	.uleb128	0x3f			/* DW_AT_external */
+	.uleb128	0xc			/* DW_FORM_flag */
+	.uleb128	0x3			/* DW_AT_name */
+	.uleb128	0x8			/* DW_FORM_string */
+	.uleb128	0x55			/* DW_AT_ranges */
+	.uleb128	0x6			/* DW_FORM_data4 */
+	.byte		0x0			/* Terminator */
+	.byte		0x0			/* Terminator */
+
+	.byte		0x0			/* Terminator */
+	.byte		0x0			/* Terminator */
--- src/gdb/testsuite/gdb.dwarf2/dw2-minsym-in-cu.exp
+++ src/gdb/testsuite/gdb.dwarf2/dw2-minsym-in-cu.exp	2012-07-13 20:26:32.216111000 +0000
@@ -0,0 +1,34 @@
+# Copyright 2004-2005, 2007-2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0  
+}
+
+# This testfile has reproducibility only with cc-with-index.sh.
+
+set testfile "dw2-minsym-in-cu"
+set srcfile ${testfile}.S
+set executable ${testfile}
+
+if [prepare_for_testing ${testfile}.exp ${executable} ${srcfile}] {
+    return -1
+}
+
+# Ask for address which is still located in this CU but not described by
+# any DIE.
+gdb_test "info fun func2" {All functions matching regular expression "func2":}