[PATCH] gcov-profile: Allow negavive counts of indirect calls [PR105282]

[PATCH] gcov-profile: Allow negavive counts of indirect calls [PR105282]

[Sergei Trofimovich]

From: Sergei Trofimovich <siarheit@google.com>

TOPN metrics are histograms that contain overall count and per-bucket
count. Overall count can be nevative when two profiles merge and some
of per-bucket metrics are dropped.

Noticed as an ICE on python PGO build where gcc crashes as:

    during IPA pass: modref
    a.c:36:1: ICE: in stream_out_histogram_value, at value-prof.cc:340
       36 | }
          | ^
    stream_out_histogram_value(output_block*, histogram_value_t*)
            gcc/value-prof.cc:340

gcc/ChangeLog:

	PR gcov-profile/105282
	* value-prof.cc (stream_out_histogram_value): Allow negavive counts
	on HIST_TYPE_INDIR_CALL.
---
 gcc/value-prof.cc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gcc/value-prof.cc b/gcc/value-prof.cc
index 9785c7a03ea..4927d119aa0 100644
--- a/gcc/value-prof.cc
+++ b/gcc/value-prof.cc
@@ -319,40 +319,44 @@ stream_out_histogram_value (struct output_block *ob, histogram_value hist)
   streamer_write_bitpack (&bp);
   switch (hist->type)
     {
     case HIST_TYPE_INTERVAL:
       streamer_write_hwi (ob, hist->hdata.intvl.int_start);
       streamer_write_uhwi (ob, hist->hdata.intvl.steps);
       break;
     default:
       break;
     }
   for (i = 0; i < hist->n_counters; i++)
     {
       /* When user uses an unsigned type with a big value, constant converted
 	 to gcov_type (a signed type) can be negative.  */
       gcov_type value = hist->hvalue.counters[i];
       if (hist->type == HIST_TYPE_TOPN_VALUES
 	  || hist->type == HIST_TYPE_IOR)
 	/* Note that the IOR counter tracks pointer values and these can have
 	   sign bit set.  */
 	;
+      else if (hist->type == HIST_TYPE_INDIR_CALL && i == 0)
+	/* 'all' counter overflow is stored as a negative value. Individual
+	   counters and values are expected to be non-negative.  */
+	;
       else
 	gcc_assert (value >= 0);
 
       streamer_write_gcov_count (ob, value);
     }
   if (hist->hvalue.next)
     stream_out_histogram_value (ob, hist->hvalue.next);
 }
 
 /* Dump information about HIST to DUMP_FILE.  */
 
 void
 stream_in_histogram_value (class lto_input_block *ib, gimple *stmt)
 {
   enum hist_type type;
   unsigned int ncounters = 0;
   struct bitpack_d bp;
   unsigned int i;
   histogram_value new_val;
   bool next;
-- 
2.35.1

Re: [PATCH] gcov-profile: Allow negavive counts of indirect calls [PR105282]

[Martin Liška]

Hi.

Thanks you for the patch, please apply it.

Cheers,
Martin

Re: [PATCH] gcov-profile: Allow negavive counts of indirect calls [PR105282]

[Jan Hubicka]

> From: Sergei Trofimovich <siarheit@google.com>
> 
> TOPN metrics are histograms that contain overall count and per-bucket
> count. Overall count can be nevative when two profiles merge and some
> of per-bucket metrics are dropped.
> 
> Noticed as an ICE on python PGO build where gcc crashes as:
> 
>     during IPA pass: modref
>     a.c:36:1: ICE: in stream_out_histogram_value, at value-prof.cc:340
>        36 | }
>           | ^
>     stream_out_histogram_value(output_block*, histogram_value_t*)
>             gcc/value-prof.cc:340
> 
> gcc/ChangeLog:
> 
> 	PR gcov-profile/105282
> 	* value-prof.cc (stream_out_histogram_value): Allow negavive counts
> 	on HIST_TYPE_INDIR_CALL.
> ---
>  gcc/value-prof.cc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gcc/value-prof.cc b/gcc/value-prof.cc
> index 9785c7a03ea..4927d119aa0 100644
> --- a/gcc/value-prof.cc
> +++ b/gcc/value-prof.cc
> @@ -319,40 +319,44 @@ stream_out_histogram_value (struct output_block *ob, histogram_value hist)
>    streamer_write_bitpack (&bp);
>    switch (hist->type)
>      {
>      case HIST_TYPE_INTERVAL:
>        streamer_write_hwi (ob, hist->hdata.intvl.int_start);
>        streamer_write_uhwi (ob, hist->hdata.intvl.steps);
>        break;
>      default:
>        break;
>      }
>    for (i = 0; i < hist->n_counters; i++)
>      {
>        /* When user uses an unsigned type with a big value, constant converted
>  	 to gcov_type (a signed type) can be negative.  */
>        gcov_type value = hist->hvalue.counters[i];
>        if (hist->type == HIST_TYPE_TOPN_VALUES
>  	  || hist->type == HIST_TYPE_IOR)
>  	/* Note that the IOR counter tracks pointer values and these can have
>  	   sign bit set.  */
>  	;
> +      else if (hist->type == HIST_TYPE_INDIR_CALL && i == 0)
> +	/* 'all' counter overflow is stored as a negative value. Individual
> +	   counters and values are expected to be non-negative.  */
> +	;

I tink we can just drop the sanity check completely.  In general the
profile data may be corrupted and each use of it should be guarded to
not explode on such situation.
I added the check here long time ago while implementing the early
version of profile streaming patch. At that time some bugs was causing
counts to be negative due to weird overflows in the logic normalizing
profiles from different object files to same number of executions.

Honza
>        else
>  	gcc_assert (value >= 0);
>  
>        streamer_write_gcov_count (ob, value);
>      }
>    if (hist->hvalue.next)
>      stream_out_histogram_value (ob, hist->hvalue.next);
>  }
>  
>  /* Dump information about HIST to DUMP_FILE.  */
>  
>  void
>  stream_in_histogram_value (class lto_input_block *ib, gimple *stmt)
>  {
>    enum hist_type type;
>    unsigned int ncounters = 0;
>    struct bitpack_d bp;
>    unsigned int i;
>    histogram_value new_val;
>    bool next;
> -- 
> 2.35.1
> 

Re: [PATCH] gcov-profile: Allow negavive counts of indirect calls [PR105282]

[Martin Liška]

On 4/20/22 10:55, Jan Hubicka via Gcc-patches wrote:
> I tink we can just drop the sanity check completely.  In general the
> profile data may be corrupted and each use of it should be guarded to
> not explode on such situation.

Makes sense to me. I'm going to do it once stage1 opens.

Cheers,
Martin