Message ID | 7adcbe4f-5aa7-30bc-82e6-a877e03eaafd@suse.cz |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5E7813858034 for <patchwork@sourceware.org>; Fri, 1 Oct 2021 09:55:34 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 150E23857C6D for <gcc-patches@gcc.gnu.org>; Fri, 1 Oct 2021 09:55:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 150E23857C6D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 082D22267A for <gcc-patches@gcc.gnu.org>; Fri, 1 Oct 2021 09:55:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1633082108; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Q/l9/H/05CQKxByrv1WrF65CgKXyDMvPC3sCMMMVumk=; b=ZUbMcPUDCiFInF4MYHI4Q36xKIR558yCKJYcCY3mixaZTuxEV7iX4rC2Ob29QNKaAZgdKZ B7qg22yRoPY3Okf5cb6zjuzVJ2sT6UoIpjXKfTNuiV+HrLStTTAr049UQ9ZZxWadBSM+z0 7HeVxDldAbNBL4prdUK8Wor4ci44EVc= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1633082108; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=Q/l9/H/05CQKxByrv1WrF65CgKXyDMvPC3sCMMMVumk=; b=biNnNMYBqvDHpXrzRgZGDTicmHYTsS8rDj1cINNUhkqULPGoS3PWFchUQMlANBoUjFlDV6 IK520czdbKiJC/Bg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id EA74713C33 for <gcc-patches@gcc.gnu.org>; Fri, 1 Oct 2021 09:55:07 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id ZIkJOPvaVmETBAAAMHmgww (envelope-from <mliska@suse.cz>) for <gcc-patches@gcc.gnu.org>; Fri, 01 Oct 2021 09:55:07 +0000 Message-ID: <7adcbe4f-5aa7-30bc-82e6-a877e03eaafd@suse.cz> Date: Fri, 1 Oct 2021 11:55:07 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.1 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH] gcov: make profile merging smarter To: gcc-patches@gcc.gnu.org Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
gcov: make profile merging smarter
|
|
Commit Message
Martin Liška
Oct. 1, 2021, 9:55 a.m. UTC
Support merging of profiles that are built from a different .o files but belong to the same source file. Moreover, a checksum is verified during profile merging and so we can safely combine such profile. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. I'm going to install the patch if there are no comments about it. Thanks, Martin PR gcov-profile/90364 gcc/ChangeLog: * coverage.c (build_info): Emit checksum to the global variable. (build_info_type): Add new field for checksum. (coverage_obj_finish): Pass object_checksum. (coverage_init): Use 0 as checksum for .gcno files. * gcov-dump.c (dump_gcov_file): Dump also new checksum field. * gcov.c (read_graph_file): Read also checksum. libgcc/ChangeLog: * libgcov-driver.c (merge_one_data): Skip timestamp and verify checksums. (write_one_data): Write also checksum. * libgcov-util.c (read_gcda_file): Read also checksum field. * libgcov.h (struct gcov_info): Add new field. --- gcc/coverage.c | 50 ++++++++++++++++++++++++++++------------- gcc/gcov-dump.c | 9 ++++---- gcc/gcov.c | 5 +++++ libgcc/libgcov-driver.c | 8 +++++-- libgcc/libgcov-util.c | 3 +++ libgcc/libgcov.h | 1 + 6 files changed, 54 insertions(+), 22 deletions(-)
Comments
On Fri, Oct 1, 2021 at 11:55 AM Martin Liška <mliska@suse.cz> wrote: > > Support merging of profiles that are built from a different .o files > but belong to the same source file. Moreover, a checksum is verified > during profile merging and so we can safely combine such profile. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > I'm going to install the patch if there are no comments about it. Is the ->stamp field now unused? I wonder whether crc32 is good enough or whether we need to enable -fprofile-correction by default for example? But -fprofile-correction is about -fprofile-use, not profile merging, right? What does the latter do upon mismatch? Alternatively would it be possible to keep multiple sets of data in the file, one for each 'stamp'? How does the profile-use step figure a mismatch here, or does it not care whether it mixes input with 'different stamp'? The current behavior is also somewhat odd: > ./a.out libgcov profiling error:/tmp/t.gcda:overwriting an existing profile data with a different timestamp but it should probably say 'warning' since it indeed simply overwrites data instead of merging it. I wonder if we can simply drop the stamp check alltogether and make the checks that match up the data against the number of counters behave as to warning and overwriting the data instead of failing fatally? The actual merging of data would need to be delayed, but at least until the first actual merge was done we could simply proceed? Richard. > Thanks, > Martin > > PR gcov-profile/90364 > > gcc/ChangeLog: > > * coverage.c (build_info): Emit checksum to the global variable. > (build_info_type): Add new field for checksum. > (coverage_obj_finish): Pass object_checksum. > (coverage_init): Use 0 as checksum for .gcno files. > * gcov-dump.c (dump_gcov_file): Dump also new checksum field. > * gcov.c (read_graph_file): Read also checksum. > > libgcc/ChangeLog: > > * libgcov-driver.c (merge_one_data): Skip timestamp and verify > checksums. > (write_one_data): Write also checksum. > * libgcov-util.c (read_gcda_file): Read also checksum field. > * libgcov.h (struct gcov_info): Add new field. > --- > gcc/coverage.c | 50 ++++++++++++++++++++++++++++------------- > gcc/gcov-dump.c | 9 ++++---- > gcc/gcov.c | 5 +++++ > libgcc/libgcov-driver.c | 8 +++++-- > libgcc/libgcov-util.c | 3 +++ > libgcc/libgcov.h | 1 + > 6 files changed, 54 insertions(+), 22 deletions(-) > > diff --git a/gcc/coverage.c b/gcc/coverage.c > index 10d7f8366cb..4467f1eaa5c 100644 > --- a/gcc/coverage.c > +++ b/gcc/coverage.c > @@ -129,16 +129,7 @@ static const char *const ctr_names[GCOV_COUNTERS] = { > #undef DEF_GCOV_COUNTER > > /* Forward declarations. */ > -static void read_counts_file (void); > static tree build_var (tree, tree, int); > -static void build_fn_info_type (tree, unsigned, tree); > -static void build_info_type (tree, tree); > -static tree build_fn_info (const struct coverage_data *, tree, tree); > -static tree build_info (tree, tree); > -static bool coverage_obj_init (void); > -static vec<constructor_elt, va_gc> *coverage_obj_fn > -(vec<constructor_elt, va_gc> *, tree, struct coverage_data const *); > -static void coverage_obj_finish (vec<constructor_elt, va_gc> *); > > /* Return the type node for gcov_type. */ > > @@ -218,6 +209,9 @@ read_counts_file (void) > tag = gcov_read_unsigned (); > bbg_file_stamp = crc32_unsigned (bbg_file_stamp, tag); > > + /* Read checksum. */ > + gcov_read_unsigned (); > + > counts_hash = new hash_table<counts_entry> (10); > while ((tag = gcov_read_unsigned ())) > { > @@ -935,6 +929,12 @@ build_info_type (tree type, tree fn_info_ptr_type) > DECL_CHAIN (field) = fields; > fields = field; > > + /* Checksum. */ > + field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, > + get_gcov_unsigned_t ()); > + DECL_CHAIN (field) = fields; > + fields = field; > + > /* Filename */ > field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, > build_pointer_type (build_qualified_type > @@ -977,7 +977,7 @@ build_info_type (tree type, tree fn_info_ptr_type) > function info objects. */ > > static tree > -build_info (tree info_type, tree fn_ary) > +build_info (tree info_type, tree fn_ary, unsigned object_checksum) > { > tree info_fields = TYPE_FIELDS (info_type); > tree merge_fn_type, n_funcs; > @@ -996,13 +996,19 @@ build_info (tree info_type, tree fn_ary) > /* next -- NULL */ > CONSTRUCTOR_APPEND_ELT (v1, info_fields, null_pointer_node); > info_fields = DECL_CHAIN (info_fields); > - > + > /* stamp */ > CONSTRUCTOR_APPEND_ELT (v1, info_fields, > build_int_cstu (TREE_TYPE (info_fields), > bbg_file_stamp)); > info_fields = DECL_CHAIN (info_fields); > > + /* Checksum. */ > + CONSTRUCTOR_APPEND_ELT (v1, info_fields, > + build_int_cstu (TREE_TYPE (info_fields), > + object_checksum)); > + info_fields = DECL_CHAIN (info_fields); > + > /* Filename */ > da_file_name_len = strlen (da_file_name); > filename_string = build_string (da_file_name_len + 1, da_file_name); > @@ -1214,7 +1220,8 @@ coverage_obj_fn (vec<constructor_elt, va_gc> *ctor, tree fn, > function objects from CTOR. Generate the gcov_info initializer. */ > > static void > -coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) > +coverage_obj_finish (vec<constructor_elt, va_gc> *ctor, > + unsigned object_checksum) > { > unsigned n_functions = vec_safe_length (ctor); > tree fn_info_ary_type = build_array_type > @@ -1231,7 +1238,7 @@ coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) > varpool_node::finalize_decl (fn_info_ary); > > DECL_INITIAL (gcov_info_var) > - = build_info (TREE_TYPE (gcov_info_var), fn_info_ary); > + = build_info (TREE_TYPE (gcov_info_var), fn_info_ary, object_checksum); > varpool_node::finalize_decl (gcov_info_var); > } > > @@ -1300,7 +1307,6 @@ coverage_init (const char *filename) > strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX); > > bbg_file_stamp = local_tick; > - > if (flag_auto_profile) > read_autofdo_file (); > else if (flag_branch_probabilities) > @@ -1328,6 +1334,8 @@ coverage_init (const char *filename) > gcov_write_unsigned (GCOV_NOTE_MAGIC); > gcov_write_unsigned (GCOV_VERSION); > gcov_write_unsigned (bbg_file_stamp); > + /* Use an arbitrary checksum */ > + gcov_write_unsigned (0); > gcov_write_string (getpwd ()); > > /* Do not support has_unexecuted_blocks for Ada. */ > @@ -1353,14 +1361,24 @@ coverage_finish (void) > cannot uniquely stamp it. If we can stamp it, libgcov will DTRT. */ > unlink (da_file_name); > > + /* Global GCDA checksum that aggregates all functions. */ > + unsigned object_checksum = 0; > + > if (coverage_obj_init ()) > { > vec<constructor_elt, va_gc> *fn_ctor = NULL; > struct coverage_data *fn; > > for (fn = functions_head; fn; fn = fn->next) > - fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); > - coverage_obj_finish (fn_ctor); > + { > + fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); > + > + object_checksum = crc32_unsigned (object_checksum, fn->ident); > + object_checksum = crc32_unsigned (object_checksum, > + fn->lineno_checksum); > + object_checksum = crc32_unsigned (object_checksum, fn->cfg_checksum); > + } > + coverage_obj_finish (fn_ctor, object_checksum); > } > > XDELETEVEC (da_file_name); > diff --git a/gcc/gcov-dump.c b/gcc/gcov-dump.c > index f1489fe0214..bfaf735d2ff 100644 > --- a/gcc/gcov-dump.c > +++ b/gcc/gcov-dump.c > @@ -206,11 +206,12 @@ dump_gcov_file (const char *filename) > } > > /* stamp */ > - { > - unsigned stamp = gcov_read_unsigned (); > + unsigned stamp = gcov_read_unsigned (); > + printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); > > - printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); > - } > + /* Checksum */ > + unsigned checksum = gcov_read_unsigned (); > + printf ("%s:checksum %lu\n", filename, (unsigned long)checksum); > > if (!is_data_type) > { > diff --git a/gcc/gcov.c b/gcc/gcov.c > index cf0a49d8c30..829e955a63b 100644 > --- a/gcc/gcov.c > +++ b/gcc/gcov.c > @@ -1814,6 +1814,8 @@ read_graph_file (void) > bbg_file_name, v, e); > } > bbg_stamp = gcov_read_unsigned (); > + /* Read checksum. */ > + gcov_read_unsigned (); > bbg_cwd = xstrdup (gcov_read_string ()); > bbg_supports_has_unexecuted_blocks = gcov_read_unsigned (); > > @@ -2031,6 +2033,9 @@ read_count_file (void) > goto cleanup; > } > > + /* Read checksum. */ > + gcov_read_unsigned (); > + > while ((tag = gcov_read_unsigned ())) > { > unsigned length = gcov_read_unsigned (); > diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c > index 087f71e0107..7aa97bbb06a 100644 > --- a/libgcc/libgcov-driver.c > +++ b/libgcc/libgcov-driver.c > @@ -260,12 +260,15 @@ merge_one_data (const char *filename, > if (!gcov_version (gi_ptr, length, filename)) > return -1; > > + /* Skip timestamp. */ > + gcov_read_unsigned (); > + > length = gcov_read_unsigned (); > - if (length != gi_ptr->stamp) > + if (length != gi_ptr->checksum) > { > /* Read from a different compilation. Overwrite the file. */ > gcov_error (GCOV_PROF_PREFIX "overwriting an existing profile data " > - "with a different timestamp\n", filename); > + "with a different checksum\n", filename); > return 0; > } > > @@ -495,6 +498,7 @@ write_one_data (const struct gcov_info *gi_ptr, > dump_unsigned (GCOV_DATA_MAGIC, dump_fn, arg); > dump_unsigned (GCOV_VERSION, dump_fn, arg); > dump_unsigned (gi_ptr->stamp, dump_fn, arg); > + dump_unsigned (gi_ptr->checksum, dump_fn, arg); > > #ifdef NEED_L_GCOV > /* Generate whole program statistics. */ > diff --git a/libgcc/libgcov-util.c b/libgcc/libgcov-util.c > index bc7416d99bf..766ca3559c4 100644 > --- a/libgcc/libgcov-util.c > +++ b/libgcc/libgcov-util.c > @@ -310,6 +310,9 @@ read_gcda_file (const char *filename) > /* Read stamp. */ > obj_info->stamp = gcov_read_unsigned (); > > + /* Read checksum. */ > + obj_info->checksum = gcov_read_unsigned (); > + > while (1) > { > gcov_position_t base; > diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h > index f6354a7a070..2a365c95759 100644 > --- a/libgcc/libgcov.h > +++ b/libgcc/libgcov.h > @@ -230,6 +230,7 @@ struct gcov_info > struct gcov_info *next; /* link to next, used by libgcov */ > > gcov_unsigned_t stamp; /* uniquifying time stamp */ > + gcov_unsigned_t checksum; /* unique object checksum */ > const char *filename; /* output file name */ > > gcov_merge_fn merge[GCOV_COUNTERS]; /* merge functions (null for > -- > 2.33.0 >
On 10/1/21 12:17, Richard Biener wrote: > On Fri, Oct 1, 2021 at 11:55 AM Martin Liška <mliska@suse.cz> wrote: >> >> Support merging of profiles that are built from a different .o files >> but belong to the same source file. Moreover, a checksum is verified >> during profile merging and so we can safely combine such profile. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> I'm going to install the patch if there are no comments about it. > > Is the ->stamp field now unused? Yes, it's still used for pairing of .gcda and .gcno files when --coverage is used. > > I wonder whether crc32 is good enough or whether we need to enable Dunno. We can alternatively use a stronger hashing function, maybe inchash? > -fprofile-correction by default for example? Probably not. > But -fprofile-correction > is about -fprofile-use, not profile merging, right? What does the latter > do upon mismatch? It prints the 'libgcov profiling error'. > > Alternatively would it be possible to keep multiple sets of data in the file, > one for each 'stamp'? Yes, we can theoretically do it, but I'm not planning working on that now. > How does the profile-use step figure a mismatch > here, or does it not care whether it mixes input with 'different stamp'? Based on function id, it does verification for cfg_checksum and lineno_checksum. > > The current behavior is also somewhat odd: > >> ./a.out > libgcov profiling error:/tmp/t.gcda:overwriting an existing profile > data with a different timestamp > > but it should probably say 'warning' since it indeed simply overwrites data > instead of merging it. Yes, I can prepare an incremental patch for it. > > I wonder if we can simply drop the stamp check alltogether and make > the checks that match up the data against the number of counters behave > as to warning and overwriting the data instead of failing fatally? The > actual merging of data would need to be delayed, but at least until > the first actual merge was done we could simply proceed? Do you mean doing only merging of functions that have correct checksum, and bailing out for the functions that don't? Thanks for the ideas. Martin > > Richard. > >> Thanks, >> Martin >> >> PR gcov-profile/90364 >> >> gcc/ChangeLog: >> >> * coverage.c (build_info): Emit checksum to the global variable. >> (build_info_type): Add new field for checksum. >> (coverage_obj_finish): Pass object_checksum. >> (coverage_init): Use 0 as checksum for .gcno files. >> * gcov-dump.c (dump_gcov_file): Dump also new checksum field. >> * gcov.c (read_graph_file): Read also checksum. >> >> libgcc/ChangeLog: >> >> * libgcov-driver.c (merge_one_data): Skip timestamp and verify >> checksums. >> (write_one_data): Write also checksum. >> * libgcov-util.c (read_gcda_file): Read also checksum field. >> * libgcov.h (struct gcov_info): Add new field. >> --- >> gcc/coverage.c | 50 ++++++++++++++++++++++++++++------------- >> gcc/gcov-dump.c | 9 ++++---- >> gcc/gcov.c | 5 +++++ >> libgcc/libgcov-driver.c | 8 +++++-- >> libgcc/libgcov-util.c | 3 +++ >> libgcc/libgcov.h | 1 + >> 6 files changed, 54 insertions(+), 22 deletions(-) >> >> diff --git a/gcc/coverage.c b/gcc/coverage.c >> index 10d7f8366cb..4467f1eaa5c 100644 >> --- a/gcc/coverage.c >> +++ b/gcc/coverage.c >> @@ -129,16 +129,7 @@ static const char *const ctr_names[GCOV_COUNTERS] = { >> #undef DEF_GCOV_COUNTER >> >> /* Forward declarations. */ >> -static void read_counts_file (void); >> static tree build_var (tree, tree, int); >> -static void build_fn_info_type (tree, unsigned, tree); >> -static void build_info_type (tree, tree); >> -static tree build_fn_info (const struct coverage_data *, tree, tree); >> -static tree build_info (tree, tree); >> -static bool coverage_obj_init (void); >> -static vec<constructor_elt, va_gc> *coverage_obj_fn >> -(vec<constructor_elt, va_gc> *, tree, struct coverage_data const *); >> -static void coverage_obj_finish (vec<constructor_elt, va_gc> *); >> >> /* Return the type node for gcov_type. */ >> >> @@ -218,6 +209,9 @@ read_counts_file (void) >> tag = gcov_read_unsigned (); >> bbg_file_stamp = crc32_unsigned (bbg_file_stamp, tag); >> >> + /* Read checksum. */ >> + gcov_read_unsigned (); >> + >> counts_hash = new hash_table<counts_entry> (10); >> while ((tag = gcov_read_unsigned ())) >> { >> @@ -935,6 +929,12 @@ build_info_type (tree type, tree fn_info_ptr_type) >> DECL_CHAIN (field) = fields; >> fields = field; >> >> + /* Checksum. */ >> + field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, >> + get_gcov_unsigned_t ()); >> + DECL_CHAIN (field) = fields; >> + fields = field; >> + >> /* Filename */ >> field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, >> build_pointer_type (build_qualified_type >> @@ -977,7 +977,7 @@ build_info_type (tree type, tree fn_info_ptr_type) >> function info objects. */ >> >> static tree >> -build_info (tree info_type, tree fn_ary) >> +build_info (tree info_type, tree fn_ary, unsigned object_checksum) >> { >> tree info_fields = TYPE_FIELDS (info_type); >> tree merge_fn_type, n_funcs; >> @@ -996,13 +996,19 @@ build_info (tree info_type, tree fn_ary) >> /* next -- NULL */ >> CONSTRUCTOR_APPEND_ELT (v1, info_fields, null_pointer_node); >> info_fields = DECL_CHAIN (info_fields); >> - >> + >> /* stamp */ >> CONSTRUCTOR_APPEND_ELT (v1, info_fields, >> build_int_cstu (TREE_TYPE (info_fields), >> bbg_file_stamp)); >> info_fields = DECL_CHAIN (info_fields); >> >> + /* Checksum. */ >> + CONSTRUCTOR_APPEND_ELT (v1, info_fields, >> + build_int_cstu (TREE_TYPE (info_fields), >> + object_checksum)); >> + info_fields = DECL_CHAIN (info_fields); >> + >> /* Filename */ >> da_file_name_len = strlen (da_file_name); >> filename_string = build_string (da_file_name_len + 1, da_file_name); >> @@ -1214,7 +1220,8 @@ coverage_obj_fn (vec<constructor_elt, va_gc> *ctor, tree fn, >> function objects from CTOR. Generate the gcov_info initializer. */ >> >> static void >> -coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) >> +coverage_obj_finish (vec<constructor_elt, va_gc> *ctor, >> + unsigned object_checksum) >> { >> unsigned n_functions = vec_safe_length (ctor); >> tree fn_info_ary_type = build_array_type >> @@ -1231,7 +1238,7 @@ coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) >> varpool_node::finalize_decl (fn_info_ary); >> >> DECL_INITIAL (gcov_info_var) >> - = build_info (TREE_TYPE (gcov_info_var), fn_info_ary); >> + = build_info (TREE_TYPE (gcov_info_var), fn_info_ary, object_checksum); >> varpool_node::finalize_decl (gcov_info_var); >> } >> >> @@ -1300,7 +1307,6 @@ coverage_init (const char *filename) >> strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX); >> >> bbg_file_stamp = local_tick; >> - >> if (flag_auto_profile) >> read_autofdo_file (); >> else if (flag_branch_probabilities) >> @@ -1328,6 +1334,8 @@ coverage_init (const char *filename) >> gcov_write_unsigned (GCOV_NOTE_MAGIC); >> gcov_write_unsigned (GCOV_VERSION); >> gcov_write_unsigned (bbg_file_stamp); >> + /* Use an arbitrary checksum */ >> + gcov_write_unsigned (0); >> gcov_write_string (getpwd ()); >> >> /* Do not support has_unexecuted_blocks for Ada. */ >> @@ -1353,14 +1361,24 @@ coverage_finish (void) >> cannot uniquely stamp it. If we can stamp it, libgcov will DTRT. */ >> unlink (da_file_name); >> >> + /* Global GCDA checksum that aggregates all functions. */ >> + unsigned object_checksum = 0; >> + >> if (coverage_obj_init ()) >> { >> vec<constructor_elt, va_gc> *fn_ctor = NULL; >> struct coverage_data *fn; >> >> for (fn = functions_head; fn; fn = fn->next) >> - fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); >> - coverage_obj_finish (fn_ctor); >> + { >> + fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); >> + >> + object_checksum = crc32_unsigned (object_checksum, fn->ident); >> + object_checksum = crc32_unsigned (object_checksum, >> + fn->lineno_checksum); >> + object_checksum = crc32_unsigned (object_checksum, fn->cfg_checksum); >> + } >> + coverage_obj_finish (fn_ctor, object_checksum); >> } >> >> XDELETEVEC (da_file_name); >> diff --git a/gcc/gcov-dump.c b/gcc/gcov-dump.c >> index f1489fe0214..bfaf735d2ff 100644 >> --- a/gcc/gcov-dump.c >> +++ b/gcc/gcov-dump.c >> @@ -206,11 +206,12 @@ dump_gcov_file (const char *filename) >> } >> >> /* stamp */ >> - { >> - unsigned stamp = gcov_read_unsigned (); >> + unsigned stamp = gcov_read_unsigned (); >> + printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); >> >> - printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); >> - } >> + /* Checksum */ >> + unsigned checksum = gcov_read_unsigned (); >> + printf ("%s:checksum %lu\n", filename, (unsigned long)checksum); >> >> if (!is_data_type) >> { >> diff --git a/gcc/gcov.c b/gcc/gcov.c >> index cf0a49d8c30..829e955a63b 100644 >> --- a/gcc/gcov.c >> +++ b/gcc/gcov.c >> @@ -1814,6 +1814,8 @@ read_graph_file (void) >> bbg_file_name, v, e); >> } >> bbg_stamp = gcov_read_unsigned (); >> + /* Read checksum. */ >> + gcov_read_unsigned (); >> bbg_cwd = xstrdup (gcov_read_string ()); >> bbg_supports_has_unexecuted_blocks = gcov_read_unsigned (); >> >> @@ -2031,6 +2033,9 @@ read_count_file (void) >> goto cleanup; >> } >> >> + /* Read checksum. */ >> + gcov_read_unsigned (); >> + >> while ((tag = gcov_read_unsigned ())) >> { >> unsigned length = gcov_read_unsigned (); >> diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c >> index 087f71e0107..7aa97bbb06a 100644 >> --- a/libgcc/libgcov-driver.c >> +++ b/libgcc/libgcov-driver.c >> @@ -260,12 +260,15 @@ merge_one_data (const char *filename, >> if (!gcov_version (gi_ptr, length, filename)) >> return -1; >> >> + /* Skip timestamp. */ >> + gcov_read_unsigned (); >> + >> length = gcov_read_unsigned (); >> - if (length != gi_ptr->stamp) >> + if (length != gi_ptr->checksum) >> { >> /* Read from a different compilation. Overwrite the file. */ >> gcov_error (GCOV_PROF_PREFIX "overwriting an existing profile data " >> - "with a different timestamp\n", filename); >> + "with a different checksum\n", filename); >> return 0; >> } >> >> @@ -495,6 +498,7 @@ write_one_data (const struct gcov_info *gi_ptr, >> dump_unsigned (GCOV_DATA_MAGIC, dump_fn, arg); >> dump_unsigned (GCOV_VERSION, dump_fn, arg); >> dump_unsigned (gi_ptr->stamp, dump_fn, arg); >> + dump_unsigned (gi_ptr->checksum, dump_fn, arg); >> >> #ifdef NEED_L_GCOV >> /* Generate whole program statistics. */ >> diff --git a/libgcc/libgcov-util.c b/libgcc/libgcov-util.c >> index bc7416d99bf..766ca3559c4 100644 >> --- a/libgcc/libgcov-util.c >> +++ b/libgcc/libgcov-util.c >> @@ -310,6 +310,9 @@ read_gcda_file (const char *filename) >> /* Read stamp. */ >> obj_info->stamp = gcov_read_unsigned (); >> >> + /* Read checksum. */ >> + obj_info->checksum = gcov_read_unsigned (); >> + >> while (1) >> { >> gcov_position_t base; >> diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h >> index f6354a7a070..2a365c95759 100644 >> --- a/libgcc/libgcov.h >> +++ b/libgcc/libgcov.h >> @@ -230,6 +230,7 @@ struct gcov_info >> struct gcov_info *next; /* link to next, used by libgcov */ >> >> gcov_unsigned_t stamp; /* uniquifying time stamp */ >> + gcov_unsigned_t checksum; /* unique object checksum */ >> const char *filename; /* output file name */ >> >> gcov_merge_fn merge[GCOV_COUNTERS]; /* merge functions (null for >> -- >> 2.33.0 >>
On Fri, Oct 1, 2021 at 12:53 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/1/21 12:17, Richard Biener wrote: > > On Fri, Oct 1, 2021 at 11:55 AM Martin Liška <mliska@suse.cz> wrote: > >> > >> Support merging of profiles that are built from a different .o files > >> but belong to the same source file. Moreover, a checksum is verified > >> during profile merging and so we can safely combine such profile. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> I'm going to install the patch if there are no comments about it. > > > > Is the ->stamp field now unused? > > Yes, it's still used for pairing of .gcda and .gcno files when --coverage is used. > > > > > I wonder whether crc32 is good enough or whether we need to enable > > Dunno. We can alternatively use a stronger hashing function, maybe inchash? > > > -fprofile-correction by default for example? > > Probably not. > > > But -fprofile-correction > > is about -fprofile-use, not profile merging, right? What does the latter > > do upon mismatch? > > It prints the 'libgcov profiling error'. > > > > > Alternatively would it be possible to keep multiple sets of data in the file, > > one for each 'stamp'? > > Yes, we can theoretically do it, but I'm not planning working on that now. > > > How does the profile-use step figure a mismatch > > here, or does it not care whether it mixes input with 'different stamp'? > > Based on function id, it does verification for cfg_checksum and lineno_checksum. > > > > > The current behavior is also somewhat odd: > > > >> ./a.out > > libgcov profiling error:/tmp/t.gcda:overwriting an existing profile > > data with a different timestamp > > > > but it should probably say 'warning' since it indeed simply overwrites data > > instead of merging it. > > Yes, I can prepare an incremental patch for it. > > > > > I wonder if we can simply drop the stamp check alltogether and make > > the checks that match up the data against the number of counters behave > > as to warning and overwriting the data instead of failing fatally? The > > actual merging of data would need to be delayed, but at least until > > the first actual merge was done we could simply proceed? > > Do you mean doing only merging of functions that have correct checksum, and bailing > out for the functions that don't? I meant in merge_one_data do not check ->stamp or ->checksum but instead rely on the counter merging code to detect mismatches (there's read_mismatch and read_error). There's multiple things we can do when we run into those: - when we did not actually merged any counter yet we could issue the warning as before and drop the old data on the floor - when we _did_ merge some counters already we could hard-error (I suppose we can't roll-back merging that took place already) - we could do the merging two-stage, first see whether the data matches and only if it did perform the merging Note that all of the changes (including yours) have user-visible effects and the behavior is somewhat unobvious. Not merging when the object was re-built is indeed the most obvious behavior so I'm not sure it's a good idea. A new env variable to say whether to simply keep the _old_ data when merging in new data isn't possible would be another "fix" I guess? Richard. > > Thanks for the ideas. > > Martin > > > > > Richard. > > > >> Thanks, > >> Martin > >> > >> PR gcov-profile/90364 > >> > >> gcc/ChangeLog: > >> > >> * coverage.c (build_info): Emit checksum to the global variable. > >> (build_info_type): Add new field for checksum. > >> (coverage_obj_finish): Pass object_checksum. > >> (coverage_init): Use 0 as checksum for .gcno files. > >> * gcov-dump.c (dump_gcov_file): Dump also new checksum field. > >> * gcov.c (read_graph_file): Read also checksum. > >> > >> libgcc/ChangeLog: > >> > >> * libgcov-driver.c (merge_one_data): Skip timestamp and verify > >> checksums. > >> (write_one_data): Write also checksum. > >> * libgcov-util.c (read_gcda_file): Read also checksum field. > >> * libgcov.h (struct gcov_info): Add new field. > >> --- > >> gcc/coverage.c | 50 ++++++++++++++++++++++++++++------------- > >> gcc/gcov-dump.c | 9 ++++---- > >> gcc/gcov.c | 5 +++++ > >> libgcc/libgcov-driver.c | 8 +++++-- > >> libgcc/libgcov-util.c | 3 +++ > >> libgcc/libgcov.h | 1 + > >> 6 files changed, 54 insertions(+), 22 deletions(-) > >> > >> diff --git a/gcc/coverage.c b/gcc/coverage.c > >> index 10d7f8366cb..4467f1eaa5c 100644 > >> --- a/gcc/coverage.c > >> +++ b/gcc/coverage.c > >> @@ -129,16 +129,7 @@ static const char *const ctr_names[GCOV_COUNTERS] = { > >> #undef DEF_GCOV_COUNTER > >> > >> /* Forward declarations. */ > >> -static void read_counts_file (void); > >> static tree build_var (tree, tree, int); > >> -static void build_fn_info_type (tree, unsigned, tree); > >> -static void build_info_type (tree, tree); > >> -static tree build_fn_info (const struct coverage_data *, tree, tree); > >> -static tree build_info (tree, tree); > >> -static bool coverage_obj_init (void); > >> -static vec<constructor_elt, va_gc> *coverage_obj_fn > >> -(vec<constructor_elt, va_gc> *, tree, struct coverage_data const *); > >> -static void coverage_obj_finish (vec<constructor_elt, va_gc> *); > >> > >> /* Return the type node for gcov_type. */ > >> > >> @@ -218,6 +209,9 @@ read_counts_file (void) > >> tag = gcov_read_unsigned (); > >> bbg_file_stamp = crc32_unsigned (bbg_file_stamp, tag); > >> > >> + /* Read checksum. */ > >> + gcov_read_unsigned (); > >> + > >> counts_hash = new hash_table<counts_entry> (10); > >> while ((tag = gcov_read_unsigned ())) > >> { > >> @@ -935,6 +929,12 @@ build_info_type (tree type, tree fn_info_ptr_type) > >> DECL_CHAIN (field) = fields; > >> fields = field; > >> > >> + /* Checksum. */ > >> + field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, > >> + get_gcov_unsigned_t ()); > >> + DECL_CHAIN (field) = fields; > >> + fields = field; > >> + > >> /* Filename */ > >> field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, > >> build_pointer_type (build_qualified_type > >> @@ -977,7 +977,7 @@ build_info_type (tree type, tree fn_info_ptr_type) > >> function info objects. */ > >> > >> static tree > >> -build_info (tree info_type, tree fn_ary) > >> +build_info (tree info_type, tree fn_ary, unsigned object_checksum) > >> { > >> tree info_fields = TYPE_FIELDS (info_type); > >> tree merge_fn_type, n_funcs; > >> @@ -996,13 +996,19 @@ build_info (tree info_type, tree fn_ary) > >> /* next -- NULL */ > >> CONSTRUCTOR_APPEND_ELT (v1, info_fields, null_pointer_node); > >> info_fields = DECL_CHAIN (info_fields); > >> - > >> + > >> /* stamp */ > >> CONSTRUCTOR_APPEND_ELT (v1, info_fields, > >> build_int_cstu (TREE_TYPE (info_fields), > >> bbg_file_stamp)); > >> info_fields = DECL_CHAIN (info_fields); > >> > >> + /* Checksum. */ > >> + CONSTRUCTOR_APPEND_ELT (v1, info_fields, > >> + build_int_cstu (TREE_TYPE (info_fields), > >> + object_checksum)); > >> + info_fields = DECL_CHAIN (info_fields); > >> + > >> /* Filename */ > >> da_file_name_len = strlen (da_file_name); > >> filename_string = build_string (da_file_name_len + 1, da_file_name); > >> @@ -1214,7 +1220,8 @@ coverage_obj_fn (vec<constructor_elt, va_gc> *ctor, tree fn, > >> function objects from CTOR. Generate the gcov_info initializer. */ > >> > >> static void > >> -coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) > >> +coverage_obj_finish (vec<constructor_elt, va_gc> *ctor, > >> + unsigned object_checksum) > >> { > >> unsigned n_functions = vec_safe_length (ctor); > >> tree fn_info_ary_type = build_array_type > >> @@ -1231,7 +1238,7 @@ coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) > >> varpool_node::finalize_decl (fn_info_ary); > >> > >> DECL_INITIAL (gcov_info_var) > >> - = build_info (TREE_TYPE (gcov_info_var), fn_info_ary); > >> + = build_info (TREE_TYPE (gcov_info_var), fn_info_ary, object_checksum); > >> varpool_node::finalize_decl (gcov_info_var); > >> } > >> > >> @@ -1300,7 +1307,6 @@ coverage_init (const char *filename) > >> strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX); > >> > >> bbg_file_stamp = local_tick; > >> - > >> if (flag_auto_profile) > >> read_autofdo_file (); > >> else if (flag_branch_probabilities) > >> @@ -1328,6 +1334,8 @@ coverage_init (const char *filename) > >> gcov_write_unsigned (GCOV_NOTE_MAGIC); > >> gcov_write_unsigned (GCOV_VERSION); > >> gcov_write_unsigned (bbg_file_stamp); > >> + /* Use an arbitrary checksum */ > >> + gcov_write_unsigned (0); > >> gcov_write_string (getpwd ()); > >> > >> /* Do not support has_unexecuted_blocks for Ada. */ > >> @@ -1353,14 +1361,24 @@ coverage_finish (void) > >> cannot uniquely stamp it. If we can stamp it, libgcov will DTRT. */ > >> unlink (da_file_name); > >> > >> + /* Global GCDA checksum that aggregates all functions. */ > >> + unsigned object_checksum = 0; > >> + > >> if (coverage_obj_init ()) > >> { > >> vec<constructor_elt, va_gc> *fn_ctor = NULL; > >> struct coverage_data *fn; > >> > >> for (fn = functions_head; fn; fn = fn->next) > >> - fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); > >> - coverage_obj_finish (fn_ctor); > >> + { > >> + fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); > >> + > >> + object_checksum = crc32_unsigned (object_checksum, fn->ident); > >> + object_checksum = crc32_unsigned (object_checksum, > >> + fn->lineno_checksum); > >> + object_checksum = crc32_unsigned (object_checksum, fn->cfg_checksum); > >> + } > >> + coverage_obj_finish (fn_ctor, object_checksum); > >> } > >> > >> XDELETEVEC (da_file_name); > >> diff --git a/gcc/gcov-dump.c b/gcc/gcov-dump.c > >> index f1489fe0214..bfaf735d2ff 100644 > >> --- a/gcc/gcov-dump.c > >> +++ b/gcc/gcov-dump.c > >> @@ -206,11 +206,12 @@ dump_gcov_file (const char *filename) > >> } > >> > >> /* stamp */ > >> - { > >> - unsigned stamp = gcov_read_unsigned (); > >> + unsigned stamp = gcov_read_unsigned (); > >> + printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); > >> > >> - printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); > >> - } > >> + /* Checksum */ > >> + unsigned checksum = gcov_read_unsigned (); > >> + printf ("%s:checksum %lu\n", filename, (unsigned long)checksum); > >> > >> if (!is_data_type) > >> { > >> diff --git a/gcc/gcov.c b/gcc/gcov.c > >> index cf0a49d8c30..829e955a63b 100644 > >> --- a/gcc/gcov.c > >> +++ b/gcc/gcov.c > >> @@ -1814,6 +1814,8 @@ read_graph_file (void) > >> bbg_file_name, v, e); > >> } > >> bbg_stamp = gcov_read_unsigned (); > >> + /* Read checksum. */ > >> + gcov_read_unsigned (); > >> bbg_cwd = xstrdup (gcov_read_string ()); > >> bbg_supports_has_unexecuted_blocks = gcov_read_unsigned (); > >> > >> @@ -2031,6 +2033,9 @@ read_count_file (void) > >> goto cleanup; > >> } > >> > >> + /* Read checksum. */ > >> + gcov_read_unsigned (); > >> + > >> while ((tag = gcov_read_unsigned ())) > >> { > >> unsigned length = gcov_read_unsigned (); > >> diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c > >> index 087f71e0107..7aa97bbb06a 100644 > >> --- a/libgcc/libgcov-driver.c > >> +++ b/libgcc/libgcov-driver.c > >> @@ -260,12 +260,15 @@ merge_one_data (const char *filename, > >> if (!gcov_version (gi_ptr, length, filename)) > >> return -1; > >> > >> + /* Skip timestamp. */ > >> + gcov_read_unsigned (); > >> + > >> length = gcov_read_unsigned (); > >> - if (length != gi_ptr->stamp) > >> + if (length != gi_ptr->checksum) > >> { > >> /* Read from a different compilation. Overwrite the file. */ > >> gcov_error (GCOV_PROF_PREFIX "overwriting an existing profile data " > >> - "with a different timestamp\n", filename); > >> + "with a different checksum\n", filename); > >> return 0; > >> } > >> > >> @@ -495,6 +498,7 @@ write_one_data (const struct gcov_info *gi_ptr, > >> dump_unsigned (GCOV_DATA_MAGIC, dump_fn, arg); > >> dump_unsigned (GCOV_VERSION, dump_fn, arg); > >> dump_unsigned (gi_ptr->stamp, dump_fn, arg); > >> + dump_unsigned (gi_ptr->checksum, dump_fn, arg); > >> > >> #ifdef NEED_L_GCOV > >> /* Generate whole program statistics. */ > >> diff --git a/libgcc/libgcov-util.c b/libgcc/libgcov-util.c > >> index bc7416d99bf..766ca3559c4 100644 > >> --- a/libgcc/libgcov-util.c > >> +++ b/libgcc/libgcov-util.c > >> @@ -310,6 +310,9 @@ read_gcda_file (const char *filename) > >> /* Read stamp. */ > >> obj_info->stamp = gcov_read_unsigned (); > >> > >> + /* Read checksum. */ > >> + obj_info->checksum = gcov_read_unsigned (); > >> + > >> while (1) > >> { > >> gcov_position_t base; > >> diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h > >> index f6354a7a070..2a365c95759 100644 > >> --- a/libgcc/libgcov.h > >> +++ b/libgcc/libgcov.h > >> @@ -230,6 +230,7 @@ struct gcov_info > >> struct gcov_info *next; /* link to next, used by libgcov */ > >> > >> gcov_unsigned_t stamp; /* uniquifying time stamp */ > >> + gcov_unsigned_t checksum; /* unique object checksum */ > >> const char *filename; /* output file name */ > >> > >> gcov_merge_fn merge[GCOV_COUNTERS]; /* merge functions (null for > >> -- > >> 2.33.0 > >> >
On 10/4/21 13:16, Richard Biener wrote: > I meant in merge_one_data do not check ->stamp or ->checksum but instead rely > on the counter merging code to detect mismatches (there's read_mismatch and > read_error). There's multiple things we can do when we run into those: > > - when we did not actually merged any counter yet we could issue the > warning as before and drop the old data on the floor > - when we_did_ merge some counters already we could hard-error > (I suppose we can't roll-back merging that took place already) > - we could do the merging two-stage, first see whether the data matches > and only if it did perform the merging I've got your point, you are basically suggesting a fine grained merging (function based). Huh, I don't like it much as it's typically a mistake in the build setup that 2 objects (with a different checksum) want to emit profile to the same .gcda file. My patch handles the obvious situation where an object file is built exactly the same way (so no e.g. -O0 and -O2). > > Note that all of the changes (including yours) have user-visible effects and > the behavior is somewhat unobvious. Not merging when the object was > re-built is indeed the most obvious behavior so I'm not sure it's a good > idea. A new env variable to say whether to simply keep the_old_ data > when merging in new data isn't possible would be another "fix" I guess? Even for a situation when checksum matches, but the timestamp is different? Sure, we can provide env. variables that can tweak the behavior. Cheers, Martin
On Mon, Oct 4, 2021 at 1:32 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/4/21 13:16, Richard Biener wrote: > > I meant in merge_one_data do not check ->stamp or ->checksum but instead rely > > on the counter merging code to detect mismatches (there's read_mismatch and > > read_error). There's multiple things we can do when we run into those: > > > > - when we did not actually merged any counter yet we could issue the > > warning as before and drop the old data on the floor > > - when we_did_ merge some counters already we could hard-error > > (I suppose we can't roll-back merging that took place already) > > - we could do the merging two-stage, first see whether the data matches > > and only if it did perform the merging > > I've got your point, you are basically suggesting a fine grained merging > (function based). Huh, I don't like it much as it's typically a mistake > in the build setup that 2 objects (with a different checksum) want to emit > profile to the same .gcda file. I agree, it's usually a mistake. > My patch handles the obvious situation where an object file is built exactly > the same way (so no e.g. -O0 and -O2). Yeah, but then the two profiles may not be related at all ... > > > > Note that all of the changes (including yours) have user-visible effects and > > the behavior is somewhat unobvious. Not merging when the object was > > re-built is indeed the most obvious behavior so I'm not sure it's a good > > idea. A new env variable to say whether to simply keep the_old_ data > > when merging in new data isn't possible would be another "fix" I guess? > > Even for a situation when checksum matches, but the timestamp is different? > Sure, we can provide env. variables that can tweak the behavior. I suppose another distinguishing factor might be the name of the executable. But yeah, in the end it's a fishy area ... So I guess your originally posted patch might be the best way to go - can you try to amend the documentation as for the behavior with respect to re-compiling and profile merging? I suppose that if you re-compile just a single .o you currently merge into all the other .o file counters but _not_ into the newly compiled old counters. That would make coverage off as well for incremental re-compiling? I only can find @item Run the program on a representative workload to generate the arc profile information. This may be repeated any number of times. You can run concurrent instances of your program, and provided that the file system supports locking, the data files will be correctly updated. Unless a strict ISO C dialect option is in effect, @code{fork} calls are detected and correctly handled without double counting. but that's under -coverage, not sure if there's a better place to amend. Note I see there's -fprofile-dir which eventually can be used to "fix" the SPEC issue as well? Richard. > Cheers, > Martin >
On 10/5/21 12:04, Richard Biener wrote: > On Mon, Oct 4, 2021 at 1:32 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 10/4/21 13:16, Richard Biener wrote: >>> I meant in merge_one_data do not check ->stamp or ->checksum but instead rely >>> on the counter merging code to detect mismatches (there's read_mismatch and >>> read_error). There's multiple things we can do when we run into those: >>> >>> - when we did not actually merged any counter yet we could issue the >>> warning as before and drop the old data on the floor >>> - when we_did_ merge some counters already we could hard-error >>> (I suppose we can't roll-back merging that took place already) >>> - we could do the merging two-stage, first see whether the data matches >>> and only if it did perform the merging >> >> I've got your point, you are basically suggesting a fine grained merging >> (function based). Huh, I don't like it much as it's typically a mistake >> in the build setup that 2 objects (with a different checksum) want to emit >> profile to the same .gcda file. > > I agree, it's usually a mistake. > >> My patch handles the obvious situation where an object file is built exactly >> the same way (so no e.g. -O0 and -O2). > > Yeah, but then the two profiles may not be related at all ... Well, it's quite common case that one object file is then linked into multiple binaries (e.g. util.o in a project). We collect also sum_max: Sum of individual run max values. which helps handling such a situation. > >>> >>> Note that all of the changes (including yours) have user-visible effects and >>> the behavior is somewhat unobvious. Not merging when the object was >>> re-built is indeed the most obvious behavior so I'm not sure it's a good >>> idea. A new env variable to say whether to simply keep the_old_ data >>> when merging in new data isn't possible would be another "fix" I guess? >> >> Even for a situation when checksum matches, but the timestamp is different? >> Sure, we can provide env. variables that can tweak the behavior. > > I suppose another distinguishing factor might be the name of the executable. Well, at compile time, we don't know name of a final executable. > > But yeah, in the end it's a fishy area ... > > So I guess your originally posted patch might be the best way to go - can you > try to amend the documentation as for the behavior with respect to > re-compiling and profile merging? I suppose that if you re-compile just > a single .o you currently merge into all the other .o file counters but _not_ > into the newly compiled old counters. Yes, I can update the documentation. > That would make coverage off > as well for incremental re-compiling? Yes. > > I only can find > > @item > Run the program on a representative workload to generate the arc profile > information. This may be repeated any number of times. You can run > concurrent instances of your program, and provided that the file system > supports locking, the data files will be correctly updated. Unless > a strict ISO C dialect option is in effect, @code{fork} calls are > detected and correctly handled without double counting. > > but that's under -coverage, not sure if there's a better place to amend. > > Note I see there's -fprofile-dir which eventually can be used to "fix" > the SPEC issue as well? We would have to provide a different option value of -fprofile-dir for both binaries. That's something we can't easily do in a SPEC config file. Let me update the documentation bits. Martin > > Richard. > >> Cheers, >> Martin >>
On 10/11/21 15:49, Martin Liška wrote:
> Let me update the documentation bits.
There's the updated patch.
May I install the patch now?
Thanks,
Martin
On 10/11/21 16:05, Martin Liška wrote:
> May I install the patch now?
Pushed to master, I guess we can tweak documentation in the future
if needed.
Martin
diff --git a/gcc/coverage.c b/gcc/coverage.c index 10d7f8366cb..4467f1eaa5c 100644 --- a/gcc/coverage.c +++ b/gcc/coverage.c @@ -129,16 +129,7 @@ static const char *const ctr_names[GCOV_COUNTERS] = { #undef DEF_GCOV_COUNTER /* Forward declarations. */ -static void read_counts_file (void); static tree build_var (tree, tree, int); -static void build_fn_info_type (tree, unsigned, tree); -static void build_info_type (tree, tree); -static tree build_fn_info (const struct coverage_data *, tree, tree); -static tree build_info (tree, tree); -static bool coverage_obj_init (void); -static vec<constructor_elt, va_gc> *coverage_obj_fn -(vec<constructor_elt, va_gc> *, tree, struct coverage_data const *); -static void coverage_obj_finish (vec<constructor_elt, va_gc> *); /* Return the type node for gcov_type. */ @@ -218,6 +209,9 @@ read_counts_file (void) tag = gcov_read_unsigned (); bbg_file_stamp = crc32_unsigned (bbg_file_stamp, tag); + /* Read checksum. */ + gcov_read_unsigned (); + counts_hash = new hash_table<counts_entry> (10); while ((tag = gcov_read_unsigned ())) { @@ -935,6 +929,12 @@ build_info_type (tree type, tree fn_info_ptr_type) DECL_CHAIN (field) = fields; fields = field; + /* Checksum. */ + field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, + get_gcov_unsigned_t ()); + DECL_CHAIN (field) = fields; + fields = field; + /* Filename */ field = build_decl (BUILTINS_LOCATION, FIELD_DECL, NULL_TREE, build_pointer_type (build_qualified_type @@ -977,7 +977,7 @@ build_info_type (tree type, tree fn_info_ptr_type) function info objects. */ static tree -build_info (tree info_type, tree fn_ary) +build_info (tree info_type, tree fn_ary, unsigned object_checksum) { tree info_fields = TYPE_FIELDS (info_type); tree merge_fn_type, n_funcs; @@ -996,13 +996,19 @@ build_info (tree info_type, tree fn_ary) /* next -- NULL */ CONSTRUCTOR_APPEND_ELT (v1, info_fields, null_pointer_node); info_fields = DECL_CHAIN (info_fields); - + /* stamp */ CONSTRUCTOR_APPEND_ELT (v1, info_fields, build_int_cstu (TREE_TYPE (info_fields), bbg_file_stamp)); info_fields = DECL_CHAIN (info_fields); + /* Checksum. */ + CONSTRUCTOR_APPEND_ELT (v1, info_fields, + build_int_cstu (TREE_TYPE (info_fields), + object_checksum)); + info_fields = DECL_CHAIN (info_fields); + /* Filename */ da_file_name_len = strlen (da_file_name); filename_string = build_string (da_file_name_len + 1, da_file_name); @@ -1214,7 +1220,8 @@ coverage_obj_fn (vec<constructor_elt, va_gc> *ctor, tree fn, function objects from CTOR. Generate the gcov_info initializer. */ static void -coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) +coverage_obj_finish (vec<constructor_elt, va_gc> *ctor, + unsigned object_checksum) { unsigned n_functions = vec_safe_length (ctor); tree fn_info_ary_type = build_array_type @@ -1231,7 +1238,7 @@ coverage_obj_finish (vec<constructor_elt, va_gc> *ctor) varpool_node::finalize_decl (fn_info_ary); DECL_INITIAL (gcov_info_var) - = build_info (TREE_TYPE (gcov_info_var), fn_info_ary); + = build_info (TREE_TYPE (gcov_info_var), fn_info_ary, object_checksum); varpool_node::finalize_decl (gcov_info_var); } @@ -1300,7 +1307,6 @@ coverage_init (const char *filename) strcpy (da_file_name + prefix_len + len, GCOV_DATA_SUFFIX); bbg_file_stamp = local_tick; - if (flag_auto_profile) read_autofdo_file (); else if (flag_branch_probabilities) @@ -1328,6 +1334,8 @@ coverage_init (const char *filename) gcov_write_unsigned (GCOV_NOTE_MAGIC); gcov_write_unsigned (GCOV_VERSION); gcov_write_unsigned (bbg_file_stamp); + /* Use an arbitrary checksum */ + gcov_write_unsigned (0); gcov_write_string (getpwd ()); /* Do not support has_unexecuted_blocks for Ada. */ @@ -1353,14 +1361,24 @@ coverage_finish (void) cannot uniquely stamp it. If we can stamp it, libgcov will DTRT. */ unlink (da_file_name); + /* Global GCDA checksum that aggregates all functions. */ + unsigned object_checksum = 0; + if (coverage_obj_init ()) { vec<constructor_elt, va_gc> *fn_ctor = NULL; struct coverage_data *fn; for (fn = functions_head; fn; fn = fn->next) - fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); - coverage_obj_finish (fn_ctor); + { + fn_ctor = coverage_obj_fn (fn_ctor, fn->fn_decl, fn); + + object_checksum = crc32_unsigned (object_checksum, fn->ident); + object_checksum = crc32_unsigned (object_checksum, + fn->lineno_checksum); + object_checksum = crc32_unsigned (object_checksum, fn->cfg_checksum); + } + coverage_obj_finish (fn_ctor, object_checksum); } XDELETEVEC (da_file_name); diff --git a/gcc/gcov-dump.c b/gcc/gcov-dump.c index f1489fe0214..bfaf735d2ff 100644 --- a/gcc/gcov-dump.c +++ b/gcc/gcov-dump.c @@ -206,11 +206,12 @@ dump_gcov_file (const char *filename) } /* stamp */ - { - unsigned stamp = gcov_read_unsigned (); + unsigned stamp = gcov_read_unsigned (); + printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); - printf ("%s:stamp %lu\n", filename, (unsigned long)stamp); - } + /* Checksum */ + unsigned checksum = gcov_read_unsigned (); + printf ("%s:checksum %lu\n", filename, (unsigned long)checksum); if (!is_data_type) { diff --git a/gcc/gcov.c b/gcc/gcov.c index cf0a49d8c30..829e955a63b 100644 --- a/gcc/gcov.c +++ b/gcc/gcov.c @@ -1814,6 +1814,8 @@ read_graph_file (void) bbg_file_name, v, e); } bbg_stamp = gcov_read_unsigned (); + /* Read checksum. */ + gcov_read_unsigned (); bbg_cwd = xstrdup (gcov_read_string ()); bbg_supports_has_unexecuted_blocks = gcov_read_unsigned (); @@ -2031,6 +2033,9 @@ read_count_file (void) goto cleanup; } + /* Read checksum. */ + gcov_read_unsigned (); + while ((tag = gcov_read_unsigned ())) { unsigned length = gcov_read_unsigned (); diff --git a/libgcc/libgcov-driver.c b/libgcc/libgcov-driver.c index 087f71e0107..7aa97bbb06a 100644 --- a/libgcc/libgcov-driver.c +++ b/libgcc/libgcov-driver.c @@ -260,12 +260,15 @@ merge_one_data (const char *filename, if (!gcov_version (gi_ptr, length, filename)) return -1; + /* Skip timestamp. */ + gcov_read_unsigned (); + length = gcov_read_unsigned (); - if (length != gi_ptr->stamp) + if (length != gi_ptr->checksum) { /* Read from a different compilation. Overwrite the file. */ gcov_error (GCOV_PROF_PREFIX "overwriting an existing profile data " - "with a different timestamp\n", filename); + "with a different checksum\n", filename); return 0; } @@ -495,6 +498,7 @@ write_one_data (const struct gcov_info *gi_ptr, dump_unsigned (GCOV_DATA_MAGIC, dump_fn, arg); dump_unsigned (GCOV_VERSION, dump_fn, arg); dump_unsigned (gi_ptr->stamp, dump_fn, arg); + dump_unsigned (gi_ptr->checksum, dump_fn, arg); #ifdef NEED_L_GCOV /* Generate whole program statistics. */ diff --git a/libgcc/libgcov-util.c b/libgcc/libgcov-util.c index bc7416d99bf..766ca3559c4 100644 --- a/libgcc/libgcov-util.c +++ b/libgcc/libgcov-util.c @@ -310,6 +310,9 @@ read_gcda_file (const char *filename) /* Read stamp. */ obj_info->stamp = gcov_read_unsigned (); + /* Read checksum. */ + obj_info->checksum = gcov_read_unsigned (); + while (1) { gcov_position_t base; diff --git a/libgcc/libgcov.h b/libgcc/libgcov.h index f6354a7a070..2a365c95759 100644 --- a/libgcc/libgcov.h +++ b/libgcc/libgcov.h @@ -230,6 +230,7 @@ struct gcov_info struct gcov_info *next; /* link to next, used by libgcov */ gcov_unsigned_t stamp; /* uniquifying time stamp */ + gcov_unsigned_t checksum; /* unique object checksum */ const char *filename; /* output file name */ gcov_merge_fn merge[GCOV_COUNTERS]; /* merge functions (null for