Patchwork Patch to add an error message when the profile is corrupted

login
register
mail settings
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

asharif tools - Jan. 19, 2011, 7:58 p.m.
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.

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.
Richard Guenther - Jan. 20, 2011, 9:57 a.m.
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;
>  	}
>      }
> 
>
asharif tools - Jan. 20, 2011, 8:12 p.m.
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
>
Richard Guenther - Jan. 21, 2011, 9:42 a.m.
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;
 	}
     }