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

[Jan Hubicka <hubicka@kam.mff.cuni.cz>]

> 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
>