Bug 13325 - gprof doesn't work when there are histogram data before the first symbol
Summary: gprof doesn't work when there are histogram data before the first symbol
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gprof (show other bugs)
Version: 2.24
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-20 02:15 UTC by Wei Guozhi
Modified: 2011-10-26 09:53 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Prevent wraparound of j (357 bytes, patch)
2011-10-24 14:14 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wei Guozhi 2011-10-20 02:15:38 UTC
In function hist.c:hist_assign_samples_1, when credit histogram data to symbols it uses the loop

for (i = 0, j = 1; i < r->num_bins; ++i)
  {
       ...
       for (j = j - 1; j < symtab.len; ++j)
         {
           sym_low_pc = symtab.base[j].hist.scaled_addr;
           sym_high_pc = symtab.base[j + 1].hist.scaled_addr;
 
           /* If high end of bin is below entry address,
              go for next bin.  */
           if (bin_high_pc < sym_low_pc)
             break;
               ...
          }
  }

Usually the last histogram is belong to symtab.base[j-1], and the current histogram may also belong to symtab.base[j-1], so the loop starts from (j-1).

Suppose we have a histogram data that isn't belong to any known symbol, and its address is before the first symbol, when it reaches to the inner loop, j=1, after entering the inner loop, j = 0, then exit the inner loop. When it reaches the inner loop again, j = 0 - 1 = 4294967295, which is much larger than the symbol table length, so the following histogram data will never hit any symbols.

So the inner loop should never modify j if the histogram doesn't hit any symbol.
Comment 1 Nick Clifton 2011-10-24 14:14:36 UTC
Created attachment 6026 [details]
Prevent wraparound of j
Comment 2 Nick Clifton 2011-10-24 14:16:18 UTC
Hi Wei,

  I believe that the uploaded patch should take care of this problem.  Please could you let me know what you think.

  The patch just prevents j from being decremented below 0, which I think should be sufficient.

Cheers
  Nick
Comment 3 Wei Guozhi 2011-10-24 21:59:42 UTC
(In reply to comment #2)
> Hi Wei,
> 
>   I believe that the uploaded patch should take care of this problem.  Please
> could you let me know what you think.
> 
>   The patch just prevents j from being decremented below 0, which I think
> should be sufficient.
> 
> Cheers
>   Nick

Hi Nick

This patch works perfectly.

For other histogram data that isn't belong to any known symbol and is not before the first symbol, the value of j is still decremented. This is not harmful to the final result, it just waste time to check more symbols. How about the following patch? It can avoid rechecking symbols with lower address.

368c368
<   unsigned int i, j;
---
>   unsigned int i, j, k;
374c374
<   for (i = 0, j = 1; i < r->num_bins; ++i)
---
>   for (i = 0, k = 1; i < r->num_bins; ++i)
393c393
<       for (j = j - 1; j < symtab.len; ++j)
---
>       for (j = k - 1; j < symtab.len; k = ++j)

thanks
Carrot
Comment 4 cvs-commit@gcc.gnu.org 2011-10-25 08:38:58 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	nickc@sourceware.org	2011-10-25 08:38:50

Modified files:
	gprof          : ChangeLog hist.c 

Log message:
	PR gprof/13325
	* hist.c (hist_assign_samples_1): Make sure that inner loop
	iterator remains valid.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gprof/ChangeLog.diff?cvsroot=src&r1=1.307&r2=1.308
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gprof/hist.c.diff?cvsroot=src&r1=1.24&r2=1.25
Comment 5 Nick Clifton 2011-10-25 08:40:38 UTC
Hi Carrot,

  You are right - your patch is better.  So I have checked it in along with this changelog entry.

Cheers
  Nick

gprof/ChangeLog
2011-10-25   Wei Guozhi  <carrot@google.com>

	PR gprof/13325
	* hist.c (hist_assign_samples_1): Make sure that inner loop
	iterator remains valid.
Comment 6 Wei Guozhi 2011-10-25 16:34:21 UTC
(In reply to comment #5)
> Hi Carrot,
> 
>   You are right - your patch is better.  So I have checked it in along with
> this changelog entry.
> 
> Cheers
>   Nick
> 
> gprof/ChangeLog
> 2011-10-25   Wei Guozhi  <carrot@google.com>
> 
>     PR gprof/13325
>     * hist.c (hist_assign_samples_1): Make sure that inner loop
>     iterator remains valid.

Hi Nick

It looks you have checked in your first patch.

thanks
Carrot
Comment 7 Nick Clifton 2011-10-25 16:47:55 UTC
Hi Carrot,

> It looks you have checked in your first patch.

Doh!  Sorry - this should be fixed now.

Cheers
   Nick
Comment 8 Wei Guozhi 2011-10-25 22:02:57 UTC
(In reply to comment #7)
> Hi Carrot,
> 
> > It looks you have checked in your first patch.
> 
> Doh!  Sorry - this should be fixed now.
> 

I think the comment 

/* PR gprof/13325: Make sure that J does not go below I.  */

should be changed to

/* PR gprof/13325: Make sure that K does not get decremented and J will never be less than 0.  */

Variable J and I don't have any direct relation.

thanks
Carrot
Comment 9 Nick Clifton 2011-10-26 09:53:05 UTC
Hi Carrot,

> I think the comment
>
> /* PR gprof/13325: Make sure that J does not go below I.  */
>
> should be changed to
>
> /* PR gprof/13325: Make sure that K does not get decremented and J will never
> be less than 0.  */

OK - done.

Cheers
   Nick