| Summary: | gprof doesn't work when there are histogram data before the first symbol | ||
|---|---|---|---|
| Product: | binutils | Reporter: | Wei Guozhi <carrot> |
| Component: | gprof | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | nickc |
| Priority: | P2 | ||
| Version: | 2.24 | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
| Attachments: | Prevent wraparound of j | ||
Created attachment 6026 [details]
Prevent wraparound of j
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 (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 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 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. (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 Hi Carrot,
> It looks you have checked in your first patch.
Doh! Sorry - this should be fixed now.
Cheers
Nick
(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 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 |
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.