PATCH: minor cleanup to dwarf2read.c

PATCH: minor cleanup to dwarf2read.c

[Jim Blandy]

2001-07-03  Jim Blandy  <jimb@redhat.com>

	* dwarf2read.c (dwarf2_build_psymtabs_hard): Remove extraneous
	code in loop condition.  This seemed to be trying to round
	info_ptr up to the next four-byte boundary, but that's not what it
	actually did.  If we discover the problem the old code was really
	trying to address, we can fix it properly.

Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.26
diff -c -r1.26 dwarf2read.c
*** gdb/dwarf2read.c	2001/07/02 17:43:07	1.26
--- gdb/dwarf2read.c	2001/07/03 22:27:17
***************
*** 980,987 ****
    obstack_init (&dwarf2_tmp_obstack);
    back_to = make_cleanup (dwarf2_free_tmp_obstack, NULL);
  
!   while ((unsigned int) (info_ptr - dwarf_info_buffer)
! 	 + ((info_ptr - dwarf_info_buffer) % 4) < dwarf_info_size)
      {
        struct comp_unit_head cu_header;
        beg_of_comp_unit = info_ptr;
--- 980,986 ----
    obstack_init (&dwarf2_tmp_obstack);
    back_to = make_cleanup (dwarf2_free_tmp_obstack, NULL);
  
!   while (info_ptr < dwarf_info_buffer + dwarf_info_size)
      {
        struct comp_unit_head cu_header;
        beg_of_comp_unit = info_ptr;

Re: PATCH: minor cleanup to dwarf2read.c

[Eli Zaretskii]

On Tue, 3 Jul 2001, Jim Blandy wrote:

> 2001-07-03  Jim Blandy  <jimb@redhat.com>
> 
> 	* dwarf2read.c (dwarf2_build_psymtabs_hard): Remove extraneous
> 	code in loop condition.  This seemed to be trying to round
> 	info_ptr up to the next four-byte boundary, but that's not what it
> 	actually did.  If we discover the problem the old code was really
> 	trying to address, we can fix it properly.

IMHO, ChangeLog is never a proper place to put such comments.  Should
the problem surface in the future, how do we expect someone to find
this piece of info?

I suggest to put this text as a comment in the source, together with a
copy of the old code, in case someone will actually need to fix this.

Btw, doesn't "cvs diff" and the ChangeLog entries tell enough about
the reason for the original code?  If not, perhaps the person who did
that change ("cvs annotate" should reveal that) could shed some light
on this.  Since the change you did is clearly incompatible with the
old code, I'm uneasy about making this change just because we don't
know why the old code was there.  Someone might have labored very hard
on it.

> *** gdb/dwarf2read.c	2001/07/02 17:43:07	1.26
> --- gdb/dwarf2read.c	2001/07/03 22:27:17
> ***************
> *** 980,987 ****
>     obstack_init (&dwarf2_tmp_obstack);
>     back_to = make_cleanup (dwarf2_free_tmp_obstack, NULL);
>   
> !   while ((unsigned int) (info_ptr - dwarf_info_buffer)
> ! 	 + ((info_ptr - dwarf_info_buffer) % 4) < dwarf_info_size)
>       {
>         struct comp_unit_head cu_header;
>         beg_of_comp_unit = info_ptr;
> --- 980,986 ----
>     obstack_init (&dwarf2_tmp_obstack);
>     back_to = make_cleanup (dwarf2_free_tmp_obstack, NULL);
>   
> !   while (info_ptr < dwarf_info_buffer + dwarf_info_size)
>       {
>         struct comp_unit_head cu_header;
>         beg_of_comp_unit = info_ptr;
> 

Re: PATCH: minor cleanup to dwarf2read.c

[Jim Blandy]

Eli Zaretskii <eliz@is.elta.co.il> writes:
> On Tue, 3 Jul 2001, Jim Blandy wrote:
> 
> > 2001-07-03  Jim Blandy  <jimb@redhat.com>
> > 
> > 	* dwarf2read.c (dwarf2_build_psymtabs_hard): Remove extraneous
> > 	code in loop condition.  This seemed to be trying to round
> > 	info_ptr up to the next four-byte boundary, but that's not what it
> > 	actually did.  If we discover the problem the old code was really
> > 	trying to address, we can fix it properly.
> 
> IMHO, ChangeLog is never a proper place to put such comments.  Should
> the problem surface in the future, how do we expect someone to find
> this piece of info?
> 
> I suggest to put this text as a comment in the source, together with a
> copy of the old code, in case someone will actually need to fix
> this.

I did hesitate to put that in the ChangeLog.  I didn't because I felt
the code was pretty obvious.

There's a buffer of byte-oriented data, dwarf_info_buffer, whose
length in bytes is dwarf_info_size.  We're walking through it with a
char pointer named info_ptr.  Each iteration through the loop, we
advance info_ptr over some variable number of bytes.  There's no way
clever tests for termination belong in the while condition, since the
things we're reading have non-trivial structure; overrun tests need to
be in the individual functions that consume data and advance info_ptr.

In other words, it was a perfectly straightforward situation marred by
obscure code.  I don't think it really needs a comment at all.  If you
have:


        foo_buf = malloc (sizeof (*foo_buf) * foo_length);

        for (i = 0; i < foo_length; i++)
          ...

you don't put any scary comments around the for statement about the
termination conditions, do you?

Re: PATCH: minor cleanup to dwarf2read.c

[Eli Zaretskii]

On 4 Jul 2001, Jim Blandy wrote:

> If you have:
> 
>         foo_buf = malloc (sizeof (*foo_buf) * foo_length);
> 
>         for (i = 0; i < foo_length; i++)
>           ...
> 
> you don't put any scary comments around the for statement about the
> termination conditions, do you?

I meant something like this:

     /* The while loop below was originally this:

        while ((unsigned int) (info_ptr - dwarf_info_buffer)
              ((info_ptr - dwarf_info_buffer) % 4) < dwarf_info_size)

        This seems to be trying to round info_ptr up to the next 
        four-byte boundary, but that's not what it actually did.  If we 
        discover the problem the old code was really trying to address, 
        we can fix it properly.  */

Re: PATCH: minor cleanup to dwarf2read.c

[Jim Blandy]

Eli Zaretskii <eliz@is.elta.co.il> writes:

> 
> 
> On 4 Jul 2001, Jim Blandy wrote:
> 
> > If you have:
> > 
> >         foo_buf = malloc (sizeof (*foo_buf) * foo_length);
> > 
> >         for (i = 0; i < foo_length; i++)
> >           ...
> > 
> > you don't put any scary comments around the for statement about the
> > termination conditions, do you?
> 
> I meant something like this:
> 
>      /* The while loop below was originally this:
> 
>         while ((unsigned int) (info_ptr - dwarf_info_buffer)
>               ((info_ptr - dwarf_info_buffer) % 4) < dwarf_info_size)
> 
>         This seems to be trying to round info_ptr up to the next 
>         four-byte boundary, but that's not what it actually did.  If we 
>         discover the problem the old code was really trying to address, 
>         we can fix it properly.  */
> 

Right.  While I understand (and completely agree with) the principle
that explanatory comments belong in the code and not in the ChangeLog
entry, in this case, I think no comment is necessary.  In fact,
something like the above would (I believe) inhibit understanding of
the code, because it suggests that a perfectly straightforward loop is
actually doing something odd and subtle.

Re: PATCH: minor cleanup to dwarf2read.c

[Andrew Cagney]

> I meant something like this:
>> 
>> /* The while loop below was originally this:
>> 
>> while ((unsigned int) (info_ptr - dwarf_info_buffer)
>> ((info_ptr - dwarf_info_buffer) % 4) < dwarf_info_size)
>> 
>> This seems to be trying to round info_ptr up to the next 
>> four-byte boundary, but that's not what it actually did.  If we 
>> discover the problem the old code was really trying to address, 
>> we can fix it properly.  */
>> 
> 
> 
> Right.  While I understand (and completely agree with) the principle
> that explanatory comments belong in the code and not in the ChangeLog
> entry, in this case, I think no comment is necessary.  In fact,
> something like the above would (I believe) inhibit understanding of
> the code, because it suggests that a perfectly straightforward loop is
> actually doing something odd and subtle.


Here I think Eli's comments are correct and important.
There should be a comment explaining what the code was and why it was 
reverted.  Only that way can you ensure that a later (or even the 
original) party doesn't come through and revert the reverted.

enjoy,
	Andrew

PATCH: minor cleanup to dwarf2read.c

[Jim Blandy]

(I'm reading dwarf2read over to try to get a handle on what we need
for C++, and so I can do a decent job evaluating Daniel's patch.  As
I'm doing this, I'm fixing minor things I see (and ignoring larger
problems).  If this ends up producing gratuitous conflicts with
Daniel's changes, I'll take care of cleaning it up.)

2001-07-04  Jim Blandy  <jimb@redhat.com>

	* dwarf2read.c (struct partial_die_info): New member: has_pc_info.
	(read_partial_die): Delete fourth argument; we return this info in
	the struct partial_die_info object itself now.
	(dwarf2_build_psymtabs_hard, scan_partial_symbols): Use the
	has_pc_info field of the partial die struct, rather than passing a
	variable by reference to read_partial_die.

Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.26
diff -c -r1.26 dwarf2read.c
*** gdb/dwarf2read.c	2001/07/02 17:43:07	1.26
--- gdb/dwarf2read.c	2001/07/04 15:58:14
***************
*** 194,199 ****
--- 194,200 ----
      unsigned int offset;
      unsigned int abbrev;
      char *name;
+     int has_pc_info;
      CORE_ADDR lowpc;
      CORE_ADDR highpc;
      struct dwarf_block *locdesc;
***************
*** 586,592 ****
  static struct abbrev_info *dwarf2_lookup_abbrev (unsigned int);
  
  static char *read_partial_die (struct partial_die_info *,
! 			       bfd *, char *, int *,
  			       const struct comp_unit_head *);
  
  static char *read_full_die (struct die_info **, bfd *, char *,
--- 587,593 ----
  static struct abbrev_info *dwarf2_lookup_abbrev (unsigned int);
  
  static char *read_partial_die (struct partial_die_info *,
! 			       bfd *, char *,
  			       const struct comp_unit_head *);
  
  static char *read_full_die (struct die_info **, bfd *, char *,
***************
*** 971,977 ****
    struct partial_die_info comp_unit_die;
    struct partial_symtab *pst;
    struct cleanup *back_to;
-   int comp_unit_has_pc_info;
    CORE_ADDR lowpc, highpc;
  
    info_ptr = dwarf_info_buffer;
--- 972,977 ----
***************
*** 1013,1019 ****
  
        /* Read the compilation unit die */
        info_ptr = read_partial_die (&comp_unit_die, abfd, info_ptr,
! 				   &comp_unit_has_pc_info, &cu_header);
  
        /* Set the language we're debugging */
        set_cu_language (comp_unit_die.language);
--- 1013,1019 ----
  
        /* Read the compilation unit die */
        info_ptr = read_partial_die (&comp_unit_die, abfd, info_ptr,
! 				   &cu_header);
  
        /* Set the language we're debugging */
        set_cu_language (comp_unit_die.language);
***************
*** 1048,1054 ****
  
  	  /* If the compilation unit didn't have an explicit address range,
  	     then use the information extracted from its child dies.  */
! 	  if (!comp_unit_has_pc_info)
  	    {
  	      comp_unit_die.lowpc = lowpc;
  	      comp_unit_die.highpc = highpc;
--- 1048,1054 ----
  
  	  /* If the compilation unit didn't have an explicit address range,
  	     then use the information extracted from its child dies.  */
! 	  if (! comp_unit_die.has_pc_info)
  	    {
  	      comp_unit_die.lowpc = lowpc;
  	      comp_unit_die.highpc = highpc;
***************
*** 1091,1112 ****
       back to that level. */
  
    int nesting_level = 1;
-   int has_pc_info;
  
    *lowpc = ((CORE_ADDR) -1);
    *highpc = ((CORE_ADDR) 0);
  
    while (nesting_level)
      {
!       info_ptr = read_partial_die (&pdi, abfd, info_ptr,
! 				   &has_pc_info, cu_header);
  
        if (pdi.name)
  	{
  	  switch (pdi.tag)
  	    {
  	    case DW_TAG_subprogram:
! 	      if (has_pc_info)
  		{
  		  if (pdi.lowpc < *lowpc)
  		    {
--- 1091,1110 ----
       back to that level. */
  
    int nesting_level = 1;
  
    *lowpc = ((CORE_ADDR) -1);
    *highpc = ((CORE_ADDR) 0);
  
    while (nesting_level)
      {
!       info_ptr = read_partial_die (&pdi, abfd, info_ptr, cu_header);
  
        if (pdi.name)
  	{
  	  switch (pdi.tag)
  	    {
  	    case DW_TAG_subprogram:
! 	      if (pdi.has_pc_info)
  		{
  		  if (pdi.lowpc < *lowpc)
  		    {
***************
*** 3124,3131 ****
  
  static char *
  read_partial_die (struct partial_die_info *part_die, bfd *abfd,
! 		  char *info_ptr, int *has_pc_info,
! 		  const struct comp_unit_head *cu_header)
  {
    unsigned int abbrev_number, bytes_read, i;
    struct abbrev_info *abbrev;
--- 3122,3128 ----
  
  static char *
  read_partial_die (struct partial_die_info *part_die, bfd *abfd,
! 		  char *info_ptr, const struct comp_unit_head *cu_header)
  {
    unsigned int abbrev_number, bytes_read, i;
    struct abbrev_info *abbrev;
***************
*** 3136,3142 ****
    int has_high_pc_attr = 0;
  
    *part_die = zeroed_partial_die;
-   *has_pc_info = 0;
    abbrev_number = read_unsigned_leb128 (abfd, info_ptr, &bytes_read);
    info_ptr += bytes_read;
    if (!abbrev_number)
--- 3133,3138 ----
***************
*** 3222,3228 ****
        int dummy;
  
        spec_ptr = dwarf_info_buffer + dwarf2_get_ref_die_offset (&spec_attr);
!       read_partial_die (&spec_die, abfd, spec_ptr, &dummy, cu_header);
        if (spec_die.name)
  	{
  	  part_die->name = spec_die.name;
--- 3218,3224 ----
        int dummy;
  
        spec_ptr = dwarf_info_buffer + dwarf2_get_ref_die_offset (&spec_attr);
!       read_partial_die (&spec_die, abfd, spec_ptr, cu_header);
        if (spec_die.name)
  	{
  	  part_die->name = spec_die.name;
***************
*** 3245,3251 ****
        && part_die->lowpc < part_die->highpc
        && (part_die->lowpc != 0
  	  || (bfd_get_file_flags (abfd) & HAS_RELOC)))
!     *has_pc_info = 1;
    return info_ptr;
  }
  
--- 3241,3247 ----
        && part_die->lowpc < part_die->highpc
        && (part_die->lowpc != 0
  	  || (bfd_get_file_flags (abfd) & HAS_RELOC)))
!     part_die->has_pc_info = 1;
    return info_ptr;
  }