| Message ID | 20220415074855.1031369-1-slyich@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gcov-profile: Allow negavive counts of indirect calls [PR105282] | expand |
Hi. Thanks you for the patch, please apply it. Cheers, Martin
> 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 >
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
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;
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(+)