| Message ID | CAAe5K+W7mdn1NtHEDFcAbpJ_R1krsNoOVNBAh2HuGdc97A4Wrw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
> As suggested by Honza, avoid bloating libgcov from gcc_assert by using > a new macro gcov_nonruntime_assert in gcov-io.c that is only mapped to > gcc_assert when not in libgcov. > > Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? > > Thanks, > Teresa > > 2014-01-09 Teresa Johnson <tejohnson@google.com> > > * gcov-io.c (gcov_position): Use gcov_nonruntime_assert. > (gcov_is_error): Ditto. > (gcov_rewrite): Ditto. > (gcov_open): Ditto. > (gcov_write_words): Ditto. > (gcov_write_length): Ditto. > (gcov_read_words): Ditto. > (gcov_read_summary): Ditto. > (gcov_sync): Ditto. > (gcov_seek): Ditto. > (gcov_histo_index): Ditto. > (static void gcov_histogram_merge): Ditto. > (compute_working_sets): Ditto. > * gcov-io.h (gcov_nonruntime_assert): Define. > > @@ -481,14 +481,14 @@ gcov_read_words (unsigned words) > const gcov_unsigned_t *result; > unsigned excess = gcov_var.length - gcov_var.offset; > > - gcc_assert (gcov_var.mode > 0); > + gcov_nonruntime_assert (gcov_var.mode > 0); > if (excess < words) > { > gcov_var.start += gcov_var.offset; > #if IN_LIBGCOV > if (excess) > { > - gcc_assert (excess == 1); > + gcov_nonruntime_assert (excess == 1); It probably makes no sense to put nonruntime access into IN_LIBGCOV defines. > memcpy (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, 4); > } > #else > @@ -497,7 +497,7 @@ gcov_read_words (unsigned words) > gcov_var.offset = 0; > gcov_var.length = excess; > #if IN_LIBGCOV > - gcc_assert (!gcov_var.length || gcov_var.length == 1); > + gcov_nonruntime_assert (!gcov_var.length || gcov_var.length == 1); > excess = GCOV_BLOCK_SIZE; > #else > if (gcov_var.length + words > gcov_var.alloc) > @@ -614,7 +614,7 @@ gcov_read_summary (struct gcov_summary *summary) > while (!cur_bitvector) > { > h_ix = bv_ix * 32; > - gcc_assert (bv_ix < GCOV_HISTOGRAM_BITVECTOR_SIZE); > + gcov_nonruntime_assert (bv_ix < GCOV_HISTOGRAM_BITVECTOR_SIZE); > cur_bitvector = histo_bitvector[bv_ix++]; > } > while (!(cur_bitvector & 0x1)) > @@ -622,7 +622,7 @@ gcov_read_summary (struct gcov_summary *summary) > h_ix++; > cur_bitvector >>= 1; > } > - gcc_assert (h_ix < GCOV_HISTOGRAM_SIZE); > + gcov_nonruntime_assert (h_ix < GCOV_HISTOGRAM_SIZE); How many of those asserts can be triggered by a corrupted gcda file? I would like to make libgcov more safe WRT file corruptions, too, so in that case we should produce an error message. The rest of changes seems OK. Honza
On Thu, Jan 9, 2014 at 6:56 AM, Jan Hubicka <hubicka@ucw.cz> wrote: >> As suggested by Honza, avoid bloating libgcov from gcc_assert by using >> a new macro gcov_nonruntime_assert in gcov-io.c that is only mapped to >> gcc_assert when not in libgcov. >> >> Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? >> >> Thanks, >> Teresa >> >> 2014-01-09 Teresa Johnson <tejohnson@google.com> >> >> * gcov-io.c (gcov_position): Use gcov_nonruntime_assert. >> (gcov_is_error): Ditto. >> (gcov_rewrite): Ditto. >> (gcov_open): Ditto. >> (gcov_write_words): Ditto. >> (gcov_write_length): Ditto. >> (gcov_read_words): Ditto. >> (gcov_read_summary): Ditto. >> (gcov_sync): Ditto. >> (gcov_seek): Ditto. >> (gcov_histo_index): Ditto. >> (static void gcov_histogram_merge): Ditto. >> (compute_working_sets): Ditto. >> * gcov-io.h (gcov_nonruntime_assert): Define. >> > >> @@ -481,14 +481,14 @@ gcov_read_words (unsigned words) >> const gcov_unsigned_t *result; >> unsigned excess = gcov_var.length - gcov_var.offset; >> >> - gcc_assert (gcov_var.mode > 0); >> + gcov_nonruntime_assert (gcov_var.mode > 0); >> if (excess < words) >> { >> gcov_var.start += gcov_var.offset; >> #if IN_LIBGCOV >> if (excess) >> { >> - gcc_assert (excess == 1); >> + gcov_nonruntime_assert (excess == 1); > > It probably makes no sense to put nonruntime access into IN_LIBGCOV defines. You are right - there were several that were in IN_LIBGCOV defines that I can just remove. > >> memcpy (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, 4); >> } >> #else >> @@ -497,7 +497,7 @@ gcov_read_words (unsigned words) >> gcov_var.offset = 0; >> gcov_var.length = excess; >> #if IN_LIBGCOV >> - gcc_assert (!gcov_var.length || gcov_var.length == 1); >> + gcov_nonruntime_assert (!gcov_var.length || gcov_var.length == 1); >> excess = GCOV_BLOCK_SIZE; >> #else >> if (gcov_var.length + words > gcov_var.alloc) >> @@ -614,7 +614,7 @@ gcov_read_summary (struct gcov_summary *summary) >> while (!cur_bitvector) >> { >> h_ix = bv_ix * 32; >> - gcc_assert (bv_ix < GCOV_HISTOGRAM_BITVECTOR_SIZE); >> + gcov_nonruntime_assert (bv_ix < GCOV_HISTOGRAM_BITVECTOR_SIZE); >> cur_bitvector = histo_bitvector[bv_ix++]; >> } >> while (!(cur_bitvector & 0x1)) >> @@ -622,7 +622,7 @@ gcov_read_summary (struct gcov_summary *summary) >> h_ix++; >> cur_bitvector >>= 1; >> } >> - gcc_assert (h_ix < GCOV_HISTOGRAM_SIZE); >> + gcov_nonruntime_assert (h_ix < GCOV_HISTOGRAM_SIZE); > > How many of those asserts can be triggered by a corrupted gcda file? > I would like to make libgcov more safe WRT file corruptions, too, so in that > case we should produce an error message. In that case should we call gcov_error when IN_LIBGCOV? One possibility would be to simply make gcov_nonruntime_assert be defined as if (!EXPR) gcov_error in the IN_LIBGCOV case. But I think what you wanted here was to reduce libgcov bloat by removing calls altogether, which this wouldn't solve. But if we want to call gcov_error in some cases, I think I need to add another macro that will either do gcc_assert when !IN_LIBGCOV and "if (!EXPR) gcov_error" when IN_LIBGCOV. Is that what you had in mind? Thanks, Teresa > > The rest of changes seems OK. > > Honza
> > In that case should we call gcov_error when IN_LIBGCOV? One > possibility would be to simply make gcov_nonruntime_assert be defined > as if (!EXPR) gcov_error in the IN_LIBGCOV case. But I think what you > wanted here was to reduce libgcov bloat by removing calls altogether, > which this wouldn't solve. But if we want to call gcov_error in some > cases, I think I need to add another macro that will either do > gcc_assert when !IN_LIBGCOV and "if (!EXPR) gcov_error" when > IN_LIBGCOV. Is that what you had in mind? I think for errors that can be triggered by data corruption, we ought to produce resonable error messages in both IN_LIBGCOV or for offline tools. Just unwound sequence if(...) gcov_error seems fine to me in this case, but we may also have assert like wrapper. I see we do not provide gcov_error outside libgcov, we probably ought to map it to fatal_error in GCC binary. thanks, Honza
Index: gcov-io.c =================================================================== --- gcov-io.c (revision 206435) +++ gcov-io.c (working copy) @@ -67,7 +67,7 @@ GCOV_LINKAGE struct gcov_var static inline gcov_position_t gcov_position (void) { - gcc_assert (gcov_var.mode > 0); + gcov_nonruntime_assert (gcov_var.mode > 0); return gcov_var.start + gcov_var.offset; } @@ -83,7 +83,7 @@ gcov_is_error (void) GCOV_LINKAGE inline void gcov_rewrite (void) { - gcc_assert (gcov_var.mode > 0); + gcov_nonruntime_assert (gcov_var.mode > 0); gcov_var.mode = -1; gcov_var.start = 0; gcov_var.offset = 0; @@ -133,7 +133,7 @@ gcov_open (const char *name, int mode) s_flock.l_pid = getpid (); #endif - gcc_assert (!gcov_var.file); + gcov_nonruntime_assert (!gcov_var.file); gcov_var.start = 0; gcov_var.offset = gcov_var.length = 0; gcov_var.overread = -1u; @@ -291,14 +291,14 @@ gcov_write_words (unsigned words) { gcov_unsigned_t *result; - gcc_assert (gcov_var.mode < 0); + gcov_nonruntime_assert (gcov_var.mode < 0); #if IN_LIBGCOV if (gcov_var.offset >= GCOV_BLOCK_SIZE) { gcov_write_block (GCOV_BLOCK_SIZE); if (gcov_var.offset) { - gcc_assert (gcov_var.offset == 1); + gcov_nonruntime_assert (gcov_var.offset == 1); memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4); } } @@ -393,9 +393,9 @@ gcov_write_length (gcov_position_t position) gcov_unsigned_t length; gcov_unsigned_t *buffer; - gcc_assert (gcov_var.mode < 0); - gcc_assert (position + 2 <= gcov_var.start + gcov_var.offset); - gcc_assert (position >= gcov_var.start); + gcov_nonruntime_assert (gcov_var.mode < 0); + gcov_nonruntime_assert (position + 2 <= gcov_var.start + gcov_var.offset); + gcov_nonruntime_assert (position >= gcov_var.start); offset = position - gcov_var.start; length = gcov_var.offset - offset - 2; buffer = (gcov_unsigned_t *) &gcov_var.buffer[offset]; @@ -481,14 +481,14 @@ gcov_read_words (unsigned words) const gcov_unsigned_t *result; unsigned excess = gcov_var.length - gcov_var.offset; - gcc_assert (gcov_var.mode > 0); + gcov_nonruntime_assert (gcov_var.mode > 0); if (excess < words) { gcov_var.start += gcov_var.offset; #if IN_LIBGCOV if (excess) { - gcc_assert (excess == 1); + gcov_nonruntime_assert (excess == 1); memcpy (gcov_var.buffer, gcov_var.buffer + gcov_var.offset, 4); } #else @@ -497,7 +497,7 @@ gcov_read_words (unsigned words) gcov_var.offset = 0; gcov_var.length = excess; #if IN_LIBGCOV - gcc_assert (!gcov_var.length || gcov_var.length == 1); + gcov_nonruntime_assert (!gcov_var.length || gcov_var.length == 1); excess = GCOV_BLOCK_SIZE; #else if (gcov_var.length + words > gcov_var.alloc) @@ -614,7 +614,7 @@ gcov_read_summary (struct gcov_summary *summary) while (!cur_bitvector) { h_ix = bv_ix * 32; - gcc_assert (bv_ix < GCOV_HISTOGRAM_BITVECTOR_SIZE); + gcov_nonruntime_assert (bv_ix < GCOV_HISTOGRAM_BITVECTOR_SIZE); cur_bitvector = histo_bitvector[bv_ix++]; } while (!(cur_bitvector & 0x1)) @@ -622,7 +622,7 @@ gcov_read_summary (struct gcov_summary *summary) h_ix++; cur_bitvector >>= 1; } - gcc_assert (h_ix < GCOV_HISTOGRAM_SIZE); + gcov_nonruntime_assert (h_ix < GCOV_HISTOGRAM_SIZE); csum->histogram[h_ix].num_counters = gcov_read_unsigned (); csum->histogram[h_ix].min_value = gcov_read_counter (); @@ -642,7 +642,7 @@ gcov_read_summary (struct gcov_summary *summary) GCOV_LINKAGE void gcov_sync (gcov_position_t base, gcov_unsigned_t length) { - gcc_assert (gcov_var.mode > 0); + gcov_nonruntime_assert (gcov_var.mode > 0); base += length; if (base - gcov_var.start <= gcov_var.length) gcov_var.offset = base - gcov_var.start; @@ -661,7 +661,7 @@ gcov_sync (gcov_position_t base, gcov_unsigned_t l GCOV_LINKAGE void gcov_seek (gcov_position_t base) { - gcc_assert (gcov_var.mode < 0); + gcov_nonruntime_assert (gcov_var.mode < 0); if (gcov_var.offset) gcov_write_block (gcov_var.offset); fseek (gcov_var.file, base << 2, SEEK_SET); @@ -736,7 +736,7 @@ gcov_histo_index (gcov_type value) if (r < 2) return (unsigned)value; - gcc_assert (r < 64); + gcov_nonruntime_assert (r < 64); /* Find the two next most significant bits to determine which of the four linear sub-buckets to select. */ @@ -853,7 +853,7 @@ static void gcov_histogram_merge (gcov_bucket_type /* The merged counters get placed in the new merged histogram at the entry for the merged min_value. */ tmp_i = gcov_histo_index (merge_min); - gcc_assert (tmp_i < GCOV_HISTOGRAM_SIZE); + gcov_nonruntime_assert (tmp_i < GCOV_HISTOGRAM_SIZE); tmp_histo[tmp_i].num_counters += merge_num; tmp_histo[tmp_i].cum_value += merge_cum; if (!tmp_histo[tmp_i].min_value || @@ -867,7 +867,7 @@ static void gcov_histogram_merge (gcov_bucket_type } } - gcc_assert (tgt_i < 0); + gcov_nonruntime_assert (tgt_i < 0); /* In the case where there were more counters in the source histogram, accumulate the remaining unmerged cumulative counter values. Add @@ -884,8 +884,8 @@ static void gcov_histogram_merge (gcov_bucket_type } /* At this point, tmp_i should be the smallest non-zero entry in the tmp_histo. */ - gcc_assert (tmp_i >= 0 && tmp_i < GCOV_HISTOGRAM_SIZE - && tmp_histo[tmp_i].num_counters > 0); + gcov_nonruntime_assert (tmp_i >= 0 && tmp_i < GCOV_HISTOGRAM_SIZE + && tmp_histo[tmp_i].num_counters > 0); tmp_histo[tmp_i].cum_value += src_cum; /* Finally, copy the merged histogram into tgt_histo. */ @@ -997,6 +997,6 @@ compute_working_sets (const struct gcov_ctr_summar using a temporary above. */ cum += histo_bucket->cum_value; } - gcc_assert (ws_ix == NUM_GCOV_WORKING_SETS); + gcov_nonruntime_assert (ws_ix == NUM_GCOV_WORKING_SETS); } #endif /* IN_GCOV <= 0 && !IN_LIBGCOV */ Index: gcov-io.h =================================================================== --- gcov-io.h (revision 206435) +++ gcov-io.h (working copy) @@ -195,6 +195,12 @@ typedef unsigned HOST_WIDEST_INT gcov_type_unsigne #define GCOV_LINKAGE extern #endif +#if IN_LIBGCOV +#define gcov_nonruntime_assert(EXPR) ((void)(0 && (EXPR))) +#else +#define gcov_nonruntime_assert(EXPR) gcc_assert (EXPR) +#endif + /* File suffixes. */ #define GCOV_DATA_SUFFIX ".gcda" #define GCOV_NOTE_SUFFIX ".gcno"
As suggested by Honza, avoid bloating libgcov from gcc_assert by using a new macro gcov_nonruntime_assert in gcov-io.c that is only mapped to gcc_assert when not in libgcov. Bootstrapped and tested on x86_64-unknown-linux-gnu. Ok for trunk? Thanks, Teresa 2014-01-09 Teresa Johnson <tejohnson@google.com> * gcov-io.c (gcov_position): Use gcov_nonruntime_assert. (gcov_is_error): Ditto. (gcov_rewrite): Ditto. (gcov_open): Ditto. (gcov_write_words): Ditto. (gcov_write_length): Ditto. (gcov_read_words): Ditto. (gcov_read_summary): Ditto. (gcov_sync): Ditto. (gcov_seek): Ditto. (gcov_histo_index): Ditto. (static void gcov_histogram_merge): Ditto. (compute_working_sets): Ditto. * gcov-io.h (gcov_nonruntime_assert): Define.