| Submitter | asharif tools |
|---|---|
| Date | Jan. 19, 2011, 7:58 p.m. |
| Message ID | <AANLkTiksvDRVYeCkPaTpzbgaCt_nQEgFRai-GN4fUAmL@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/79584/ |
| State | New |
| Headers | show |
Comments
On Wed, 19 Jan 2011, asharif tools wrote: > I filed a bug report here: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47363 > > which describes the issue. There is an incorrect error message printed > when the profile is corrupted in the case where *count > *all. This > error message may be confusing to the user. I have a patch here that > prints a more appropriate error message in the case where *count > > *all. > > I can amend the error message if you want me to. I don't think this adds much clarity for the user unless we document what "BB count", "overall count" and "profiler count" are. I think the important message is "value profile inconsistent with basic-block profile" I don't see what decisions on further actions a user could make with more information. Richard. > gcc/ChangeLog > 2011-01-19 Ahmad Sharif <asharif@google.com> > > PR gcov-profile/47363 > * gcc/value-prof.c (check_counter): Added an error message for the case > where *count > *all. > > Index: gcc/value-prof.c > =================================================================== > --- gcc/value-prof.c (revision 169016) > +++ gcc/value-prof.c (working copy) > @@ -473,9 +473,15 @@ > } > else > { > - error_at (locus, "corrupted value profile: %s " > - "profiler overall count (%d) does not match BB count (%d)", > - name, (int)*all, (int)bb_count); > + if (*all != bb_count) > + error_at (locus, "corrupted value profile: %s " > + "profiler overall count (%d) does not match BB count (%d)", > + name, (int)*all, (int)bb_count); > + if (*count > *all) > + error_at (locus, "corrupted value profile: %s " > + "profiler count (%d) greater than " > + "profiler overall count (%d)", > + name, (int)*count, (int)*all); > return true; > } > } > >
On Thu, Jan 20, 2011 at 1:57 AM, Richard Guenther <rguenther@suse.de> wrote: > On Wed, 19 Jan 2011, asharif tools wrote: > >> I filed a bug report here: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47363 >> >> which describes the issue. There is an incorrect error message printed >> when the profile is corrupted in the case where *count > *all. This >> error message may be confusing to the user. I have a patch here that >> prints a more appropriate error message in the case where *count > >> *all. >> >> I can amend the error message if you want me to. > > I don't think this adds much clarity for the user unless we > document what "BB count", "overall count" and "profiler count" > are. I think the important message is Perhaps it doesn't add much clarity, but with the current code, you get a message like this: profiler overall count (55) does not match BB count (55). The numbers do match but the message says they don't. > > "value profile inconsistent with basic-block profile" > > I don't see what decisions on further actions a user could make > with more information. You may be right about this. Do you suggest I change the message to something else? Or should I just drop this change? > > Richard. > >> gcc/ChangeLog >> 2011-01-19 Ahmad Sharif <asharif@google.com> >> >> PR gcov-profile/47363 >> * gcc/value-prof.c (check_counter): Added an error message for the case >> where *count > *all. >> >> Index: gcc/value-prof.c >> =================================================================== >> --- gcc/value-prof.c (revision 169016) >> +++ gcc/value-prof.c (working copy) >> @@ -473,9 +473,15 @@ >> } >> else >> { >> - error_at (locus, "corrupted value profile: %s " >> - "profiler overall count (%d) does not match BB count (%d)", >> - name, (int)*all, (int)bb_count); >> + if (*all != bb_count) >> + error_at (locus, "corrupted value profile: %s " >> + "profiler overall count (%d) does not match BB count (%d)", >> + name, (int)*all, (int)bb_count); >> + if (*count > *all) >> + error_at (locus, "corrupted value profile: %s " >> + "profiler count (%d) greater than " >> + "profiler overall count (%d)", >> + name, (int)*count, (int)*all); >> return true; >> } >> } >> >> > > -- > Richard Guenther <rguenther@suse.de> > Novell / SUSE Labs > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex >
On Thu, 20 Jan 2011, asharif tools wrote: > On Thu, Jan 20, 2011 at 1:57 AM, Richard Guenther <rguenther@suse.de> wrote: > > On Wed, 19 Jan 2011, asharif tools wrote: > > > >> I filed a bug report here: > >> > >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47363 > >> > >> which describes the issue. There is an incorrect error message printed > >> when the profile is corrupted in the case where *count > *all. This > >> error message may be confusing to the user. I have a patch here that > >> prints a more appropriate error message in the case where *count > > >> *all. > >> > >> I can amend the error message if you want me to. > > > > I don't think this adds much clarity for the user unless we > > document what "BB count", "overall count" and "profiler count" > > are. I think the important message is > > Perhaps it doesn't add much clarity, but with the current code, you > get a message like this: > > profiler overall count (55) does not match BB count (55). > > The numbers do match but the message says they don't. > > > > > "value profile inconsistent with basic-block profile" > > > > I don't see what decisions on further actions a user could make > > with more information. > > You may be right about this. > > Do you suggest I change the message to something else? Or should I > just drop this change? It would be nice to improve the message to something that is more helpful for the user. Richard. > > > > Richard. > > > >> gcc/ChangeLog > >> 2011-01-19 Ahmad Sharif <asharif@google.com> > >> > >> PR gcov-profile/47363 > >> * gcc/value-prof.c (check_counter): Added an error message for the case > >> where *count > *all. > >> > >> Index: gcc/value-prof.c > >> =================================================================== > >> --- gcc/value-prof.c (revision 169016) > >> +++ gcc/value-prof.c (working copy) > >> @@ -473,9 +473,15 @@ > >> } > >> else > >> { > >> - error_at (locus, "corrupted value profile: %s " > >> - "profiler overall count (%d) does not match BB count (%d)", > >> - name, (int)*all, (int)bb_count); > >> + if (*all != bb_count) > >> + error_at (locus, "corrupted value profile: %s " > >> + "profiler overall count (%d) does not match BB count (%d)", > >> + name, (int)*all, (int)bb_count); > >> + if (*count > *all) > >> + error_at (locus, "corrupted value profile: %s " > >> + "profiler count (%d) greater than " > >> + "profiler overall count (%d)", > >> + name, (int)*count, (int)*all); > >> return true; > >> } > >> } > >> > >> > > > > -- > > Richard Guenther <rguenther@suse.de> > > Novell / SUSE Labs > > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex > > > >
Patch
Index: gcc/value-prof.c =================================================================== --- gcc/value-prof.c (revision 169016) +++ gcc/value-prof.c (working copy) @@ -473,9 +473,15 @@ } else { - error_at (locus, "corrupted value profile: %s " - "profiler overall count (%d) does not match BB count (%d)", - name, (int)*all, (int)bb_count); + if (*all != bb_count) + error_at (locus, "corrupted value profile: %s " + "profiler overall count (%d) does not match BB count (%d)", + name, (int)*all, (int)bb_count); + if (*count > *all) + error_at (locus, "corrupted value profile: %s " + "profiler count (%d) greater than " + "profiler overall count (%d)", + name, (int)*count, (int)*all); return true; } }