Message ID | d8ad32a1-e6e7-2801-c497-1142e681d34a@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 2024C385801E for <patchwork@sourceware.org>; Mon, 11 Oct 2021 11:01:56 +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 716E63858D35 for <gcc-patches@gcc.gnu.org>; Mon, 11 Oct 2021 11:01:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 716E63858D35 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 A958521CCA for <gcc-patches@gcc.gnu.org>; Mon, 11 Oct 2021 11:01:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1633950098; 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=9JZr4oitN39vRWpwPfmXI3hxuruUTQHROC33q4qDy28=; b=u5aqQ+3kQsYIZ3e5vUJNY7oWtw8VwBblH5GIz8FgXMAKdBKk40Qqh3bQjVpEnnwIIzPk/a 8kw+GwdKpob0letQA973KC1rGAzzmItazUkMgW0KvY97xQI9Irc3/DO+G0qsyu42r26csh mKyYBKXP9Ghc95G70Pi32QuhxTzpFd8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1633950098; 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=9JZr4oitN39vRWpwPfmXI3hxuruUTQHROC33q4qDy28=; b=YvzoYdoJ+YAgKKu+nw6aBiusmtY1MudxL5KOSlZoNGbdnG+vEsiywpvMrpwCD0bk25/Wtw SEx9ipVFBK+8qqBQ== 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 91F4513C63 for <gcc-patches@gcc.gnu.org>; Mon, 11 Oct 2021 11:01:38 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id El2IIpIZZGFpSgAAMHmgww (envelope-from <mliska@suse.cz>) for <gcc-patches@gcc.gnu.org>; Mon, 11 Oct 2021 11:01:38 +0000 Message-ID: <d8ad32a1-e6e7-2801-c497-1142e681d34a@suse.cz> Date: Mon, 11 Oct 2021 13:01:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.1.2 From: =?utf-8?q?Martin_Li=C5=A1ka?= <mliska@suse.cz> Subject: [PATCH] options: Fix variable tracking option processing. 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=-11.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 |
options: Fix variable tracking option processing.
|
|
Commit Message
Martin Liška
Oct. 11, 2021, 11:01 a.m. UTC
After the recent change in Optimize attribute handling, we need finish_option function properly auto-detecting variable tracking options. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin PR debug/102585 gcc/ChangeLog: * common.opt: Do not init flag_var_tracking* options. * opts.c (finish_options): Handle flag_var_tracking* options. * toplev.c (process_options): Move to opts.c. gcc/testsuite/ChangeLog: * gcc.dg/pr102585.c: New test. --- gcc/common.opt | 14 +++++--------- gcc/opts.c | 28 ++++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/pr102585.c | 6 ++++++ gcc/toplev.c | 33 +++------------------------------ 4 files changed, 42 insertions(+), 39 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr102585.c
Comments
On Mon, Oct 11, 2021 at 1:02 PM Martin Liška <mliska@suse.cz> wrote: > > After the recent change in Optimize attribute handling, we need > finish_option function properly auto-detecting variable tracking options. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > PR debug/102585 > > gcc/ChangeLog: > > * common.opt: Do not init flag_var_tracking* options. > * opts.c (finish_options): Handle flag_var_tracking* options. > * toplev.c (process_options): Move to opts.c. > > gcc/testsuite/ChangeLog: > > * gcc.dg/pr102585.c: New test. > --- > gcc/common.opt | 14 +++++--------- > gcc/opts.c | 28 ++++++++++++++++++++++++++++ > gcc/testsuite/gcc.dg/pr102585.c | 6 ++++++ > gcc/toplev.c | 33 +++------------------------------ > 4 files changed, 42 insertions(+), 39 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr102585.c > > diff --git a/gcc/common.opt b/gcc/common.opt > index 52693e226d2..ec020f4e642 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -3003,19 +3003,16 @@ Common Undocumented Var(flag_use_linker_plugin) > > ; Positive if we should track variables, negative if we should run > ; the var-tracking pass only to discard debug annotations, zero if > -; we're not to run it. When flag_var_tracking == 2 (AUTODETECT_VALUE) it > -; will be set according to optimize, debug_info_level and debug_hooks > -; in process_options (). > +; we're not to run it. > fvar-tracking > -Common Var(flag_var_tracking) Init(2) PerFunction > +Common Var(flag_var_tracking) PerFunction > Perform variable tracking. > > ; Positive if we should track variables at assignments, negative if > ; we should run the var-tracking pass only to discard debug > -; annotations. When flag_var_tracking_assignments == > -; AUTODETECT_VALUE it will be set according to flag_var_tracking. > +; annotations. > fvar-tracking-assignments > -Common Var(flag_var_tracking_assignments) Init(2) PerFunction > +Common Var(flag_var_tracking_assignments) PerFunction > Perform variable tracking by annotating assignments. > > ; Nonzero if we should toggle flag_var_tracking_assignments after > @@ -3026,8 +3023,7 @@ Toggle -fvar-tracking-assignments. > > ; Positive if we should track uninitialized variables, negative if > ; we should run the var-tracking pass only to discard debug > -; annotations. When flag_var_tracking_uninit == AUTODETECT_VALUE it > -; will be set according to flag_var_tracking. > +; annotations. > fvar-tracking-uninit > Common Var(flag_var_tracking_uninit) PerFunction > Perform variable tracking and also tag variables that are uninitialized. > diff --git a/gcc/opts.c b/gcc/opts.c > index 2116c2991dd..eeb6b1dcc7c 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -1353,6 +1353,34 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, > SET_OPTION_IF_UNSET (opts, opts_set, flag_vect_cost_model, > VECT_COST_MODEL_CHEAP); > > + /* If the user specifically requested variable tracking with tagging > + uninitialized variables, we need to turn on variable tracking. > + (We already determined above that variable tracking is feasible.) */ > + if (opts->x_flag_var_tracking_uninit == 1) > + opts->x_flag_var_tracking = 1; > + > + if (!opts_set->x_flag_var_tracking) > + opts->x_flag_var_tracking = optimize >= 1; That's still not equivalent to the old code for -fvar-tracking-uninit which sets opts->x_flag_var_tracking to 1 and the old code checked that for AUTOINIT_VALUE but you override it here for -O0. > + if (!opts_set->x_flag_var_tracking_uninit) > + opts->x_flag_var_tracking_uninit = opts->x_flag_var_tracking; > + > + if (!opts_set->x_flag_var_tracking_assignments) > + opts->x_flag_var_tracking_assignments > + = (opts->x_flag_var_tracking > + && !(opts->x_flag_selective_scheduling > + || opts->x_flag_selective_scheduling2)); > + > + if (opts->x_flag_var_tracking_assignments_toggle) > + opts->x_flag_var_tracking_assignments = !opts->x_flag_var_tracking_assignments; > + > + if (opts->x_flag_var_tracking_assignments && !opts->x_flag_var_tracking) > + opts->x_flag_var_tracking = opts->x_flag_var_tracking_assignments = -1; > + > + if (opts->x_flag_var_tracking_assignments > + && (opts->x_flag_selective_scheduling || opts->x_flag_selective_scheduling2)) > + warning_at (loc, 0, > + "var-tracking-assignments changes selective scheduling"); > } > > #define LEFT_COLUMN 27 > diff --git a/gcc/testsuite/gcc.dg/pr102585.c b/gcc/testsuite/gcc.dg/pr102585.c > new file mode 100644 > index 00000000000..efd066b4a4e > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr102585.c > @@ -0,0 +1,6 @@ > +/* PR debug/102585 */ > +/* { dg-do compile } */ > +/* { dg-options "-fvar-tracking-assignments -fno-var-tracking" } */ > + > +#pragma GCC optimize 0 > +void d_demangle_callback_Og() { int c = 0; } > diff --git a/gcc/toplev.c b/gcc/toplev.c > index 81748b1152a..2f13d740b98 100644 > --- a/gcc/toplev.c > +++ b/gcc/toplev.c > @@ -1490,8 +1490,8 @@ process_options (bool no_backend) > || !dwarf_debuginfo_p () > || debug_hooks->var_location == do_nothing_debug_hooks.var_location) > { > - if (flag_var_tracking == 1 > - || flag_var_tracking_uninit == 1) > + if ((OPTION_SET_P (flag_var_tracking) && flag_var_tracking == 1) > + || (OPTION_SET_P (flag_var_tracking_uninit) && flag_var_tracking_uninit == 1)) > { > if (debug_info_level < DINFO_LEVEL_NORMAL) > warning_at (UNKNOWN_LOCATION, 0, > @@ -1504,6 +1504,7 @@ process_options (bool no_backend) > } > flag_var_tracking = 0; > flag_var_tracking_uninit = 0; > + flag_var_tracking_assignments = 0; > } > > /* The debug hooks are used to implement -fdump-go-spec because it > @@ -1512,34 +1513,6 @@ process_options (bool no_backend) > if (flag_dump_go_spec != NULL) > debug_hooks = dump_go_spec_init (flag_dump_go_spec, debug_hooks); > > - /* If the user specifically requested variable tracking with tagging > - uninitialized variables, we need to turn on variable tracking. > - (We already determined above that variable tracking is feasible.) */ > - if (flag_var_tracking_uninit == 1) > - flag_var_tracking = 1; > - > - if (flag_var_tracking == AUTODETECT_VALUE) > - flag_var_tracking = optimize >= 1; > - > - if (flag_var_tracking_uninit == AUTODETECT_VALUE) > - flag_var_tracking_uninit = flag_var_tracking; > - > - if (flag_var_tracking_assignments == AUTODETECT_VALUE) > - flag_var_tracking_assignments > - = (flag_var_tracking > - && !(flag_selective_scheduling || flag_selective_scheduling2)); > - > - if (flag_var_tracking_assignments_toggle) > - flag_var_tracking_assignments = !flag_var_tracking_assignments; > - > - if (flag_var_tracking_assignments && !flag_var_tracking) > - flag_var_tracking = flag_var_tracking_assignments = -1; > - > - if (flag_var_tracking_assignments > - && (flag_selective_scheduling || flag_selective_scheduling2)) > - warning_at (UNKNOWN_LOCATION, 0, > - "var-tracking-assignments changes selective scheduling"); > - > if (debug_nonbind_markers_p == AUTODETECT_VALUE) > debug_nonbind_markers_p > = (optimize > -- > 2.33.0 >
On 10/11/21 15:05, Richard Biener wrote: >> + if (!opts_set->x_flag_var_tracking) >> + opts->x_flag_var_tracking = optimize >= 1; > That's still not equivalent to the old code for -fvar-tracking-uninit which > sets opts->x_flag_var_tracking to 1 and the old code checked that > for AUTOINIT_VALUE but you override it here for -O0. > Do you mean the newly added code: + if (!opts_set->x_flag_var_tracking) + opts->x_flag_var_tracking = optimize >= 1; that should be equivalent to: - if (flag_var_tracking == AUTODETECT_VALUE) - flag_var_tracking = optimize >= 1; ? Or do I miss something? Thanks, Martin
On Mon, Oct 11, 2021 at 3:21 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/11/21 15:05, Richard Biener wrote: > >> + if (!opts_set->x_flag_var_tracking) > >> + opts->x_flag_var_tracking = optimize >= 1; > > That's still not equivalent to the old code for -fvar-tracking-uninit which > > sets opts->x_flag_var_tracking to 1 and the old code checked that > > for AUTOINIT_VALUE but you override it here for -O0. > > > > Do you mean the newly added code: > > + if (!opts_set->x_flag_var_tracking) > > + opts->x_flag_var_tracking = optimize >= 1; > > > that should be equivalent to: > > - if (flag_var_tracking == AUTODETECT_VALUE) > > - flag_var_tracking = optimize >= 1; > > > ? Or do I miss something? Yes. I think to be equivalent it would need to be if (!opts_set->x_flag_var_tracking_uninit && !opts_set->x_flag_var_tracking) opts->x_flag_var_tracking = optimize >= 1; see how in the old code the order of the tests makes a difference because we test flag_* we also set. Please double-check the change with regard to that. Btw, I'd be more comfortable when the move of the code would be independent of the adjustment to not rely on AUTODETECT_VALUE. Can we do the latter change first (IIRC the former one failed already)? Richard. > > Thanks, > Martin
On 10/11/21 15:45, Richard Biener wrote: > Btw, I'd be more comfortable when the move of the code would be > independent of the adjustment to not rely on AUTODETECT_VALUE. > Can we do the latter change first (IIRC the former one failed already)? All right, so I'm doing the first step by eliminating AUTODETECT_VALUE. Note we can't easily use EnabledBy, the option logic is more complicated (like optimize >= 1). Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
On Tue, Oct 12, 2021 at 5:21 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/11/21 15:45, Richard Biener wrote: > > Btw, I'd be more comfortable when the move of the code would be > > independent of the adjustment to not rely on AUTODETECT_VALUE. > > Can we do the latter change first (IIRC the former one failed already)? > > All right, so I'm doing the first step by eliminating AUTODETECT_VALUE. > Note we can't easily use EnabledBy, the option logic is more complicated (like optimize >= 1). > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? Let's split this ;) The debug_inline_points part is OK. How can debug_variable_location_views be ever -1? But the debug_variable_location_views part looks OK as well. More or less all parts that have the variable assigned in a single place in gcc/ are OK (dwarf2out_as_locview_support). But the main flag_var_tracking* cases need more thorough view, maybe we can convert them to single-set code first? Richard. > Thanks, > Martin
On 10/13/21 10:47, Richard Biener wrote: > Let's split this;) The debug_inline_points part is OK. Fine. > > How can debug_variable_location_views be ever -1? But the > debug_variable_location_views part looks OK as well. It comes from here: gvariable-location-views=incompat5 Common Driver RejectNegative Var(debug_variable_location_views, -1) Init(2) but it's fine as using -gvariable-location-views=incompat5 leads to OPTION_SET_P(debug_variable_location_views) == true. > > More or less all parts that have the variable assigned in a single > place in gcc/ are OK (dwarf2out_as_locview_support). But the > main flag_var_tracking* cases need more thorough view, > maybe we can convert them to single-set code first? I don't think so, your have code like if (flag_var_tracking_assignments_toggle) flag_var_tracking_assignments = !flag_var_tracking_assignments; which makes it more complicated. Or do I miss something? Cheers, Martin
On Wed, Oct 13, 2021 at 1:59 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/13/21 10:47, Richard Biener wrote: > > Let's split this;) The debug_inline_points part is OK. > > Fine. > > > > > How can debug_variable_location_views be ever -1? But the > > debug_variable_location_views part looks OK as well. > > It comes from here: > gvariable-location-views=incompat5 > Common Driver RejectNegative Var(debug_variable_location_views, -1) Init(2) > > but it's fine as using -gvariable-location-views=incompat5 leads to > OPTION_SET_P(debug_variable_location_views) == true. > > > > > More or less all parts that have the variable assigned in a single > > place in gcc/ are OK (dwarf2out_as_locview_support). But the > > main flag_var_tracking* cases need more thorough view, > > maybe we can convert them to single-set code first? > > I don't think so, your have code like > > if (flag_var_tracking_assignments_toggle) > flag_var_tracking_assignments = !flag_var_tracking_assignments; > > which makes it more complicated. Or do I miss something? It does, yes. But that's a ^ with flag_var_tracking_assignments_toggle ;) It's also one of the more weird flags, so it could be applied after the otherwise single set of flag_var_tracking_assignments ... Richard. > > Cheers, > Martin
On 10/13/21 14:50, Richard Biener wrote: > It does, yes. But that's a ^ with flag_var_tracking_assignments_toggle;) > > It's also one of the more weird flags, so it could be applied after the > otherwise single set of flag_var_tracking_assignments ... Well, it's far from being simple. Can we please make a step and install the patch I sent? I mean the latest that does the removal of AUTODETECT_VALUE. Martin
On Wed, Oct 13, 2021 at 3:12 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/13/21 14:50, Richard Biener wrote: > > It does, yes. But that's a ^ with flag_var_tracking_assignments_toggle;) > > > > It's also one of the more weird flags, so it could be applied after the > > otherwise single set of flag_var_tracking_assignments ... > > Well, it's far from being simple. > Can we please make a step and install the patch I sent? I mean the latest > that does the removal of AUTODETECT_VALUE. But parts of the patch are not obvious and you've not explained why you remove all Init(AUTODETECT_VALUE) but for flag_var_tracking you change it to Init(1). I count 4 assignments to flag_var_tracking in toplev.c and one in nvptx.c and c6x.c each. if (flag_var_tracking_uninit == AUTODETECT_VALUE) flag_var_tracking_uninit = flag_var_tracking; can probably be handled by EnabledBy, but then we also have if (flag_var_tracking_uninit == 1) flag_var_tracking = 1; which suggests the same the other way around. I guess positional handling might differ with say -fvar-tracking -fno-var-tracking-uninit vs. -fno-var-tracking-uninit -fvar-tracking when using EnabledBy vs. the "explicit" code. + else if (!OPTION_SET_P (flag_var_tracking) && flag_var_tracking) flag_var_tracking = optimize >= 1; I think _this_ should, instead of the Init(1), become an entry in default_options_table with OPT_LEVELS_1_PLUS. As said, besides flag_var_* the posted patch looks OK and is good to commit. Richard. > > Martin
On 10/13/21 15:29, Richard Biener wrote: > On Wed, Oct 13, 2021 at 3:12 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 10/13/21 14:50, Richard Biener wrote: >>> It does, yes. But that's a ^ with flag_var_tracking_assignments_toggle;) >>> >>> It's also one of the more weird flags, so it could be applied after the >>> otherwise single set of flag_var_tracking_assignments ... >> >> Well, it's far from being simple. >> Can we please make a step and install the patch I sent? I mean the latest >> that does the removal of AUTODETECT_VALUE. > > But parts of the patch are not obvious and you've not explained why you > remove all Init(AUTODETECT_VALUE) but for flag_var_tracking you > change it to Init(1). I count 4 assignments to flag_var_tracking in toplev.c > and one in nvptx.c and c6x.c each. All right. So the assignments in these target set flag_var_tracking = 0, which is fine. > > if (flag_var_tracking_uninit == AUTODETECT_VALUE) > flag_var_tracking_uninit = flag_var_tracking; > > can probably be handled by EnabledBy, but then we also have > > if (flag_var_tracking_uninit == 1) > flag_var_tracking = 1; Yep, I made: fvar-tracking Common Var(flag_var_tracking) PerFunction EnabledBy(fvar-tracking-uninit) and made fvar-tracking enabled with OPT_LEVELS_1_PLUS. > > which suggests the same the other way around. I guess The other way around is problematic as leads to a cycle. I tried adding a cycle detection in common_handle_option_auto (using a bitmap). But it breaks case OPT_Wunused, which set sets various sub-values :/ > positional handling might differ with say > -fvar-tracking -fno-var-tracking-uninit vs. -fno-var-tracking-uninit > -fvar-tracking I verified this is fine in debugger. > when using EnabledBy vs. the "explicit" code. > > + else if (!OPTION_SET_P (flag_var_tracking) && flag_var_tracking) > flag_var_tracking = optimize >= 1; > > I think _this_ should, instead of the Init(1), become an entry in > default_options_table with OPT_LEVELS_1_PLUS. Done. > > As said, besides flag_var_* the posted patch looks OK and is good to > commit. I'm sending an updated version that survives regression tests. Thoughts? Martin > > Richard. > >> >> Martin
On Thu, Oct 14, 2021 at 1:10 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/13/21 15:29, Richard Biener wrote: > > On Wed, Oct 13, 2021 at 3:12 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 10/13/21 14:50, Richard Biener wrote: > >>> It does, yes. But that's a ^ with flag_var_tracking_assignments_toggle;) > >>> > >>> It's also one of the more weird flags, so it could be applied after the > >>> otherwise single set of flag_var_tracking_assignments ... > >> > >> Well, it's far from being simple. > >> Can we please make a step and install the patch I sent? I mean the latest > >> that does the removal of AUTODETECT_VALUE. > > > > But parts of the patch are not obvious and you've not explained why you > > remove all Init(AUTODETECT_VALUE) but for flag_var_tracking you > > change it to Init(1). I count 4 assignments to flag_var_tracking in toplev.c > > and one in nvptx.c and c6x.c each. > > All right. So the assignments in these target set flag_var_tracking = 0, which > is fine. > > > > > if (flag_var_tracking_uninit == AUTODETECT_VALUE) > > flag_var_tracking_uninit = flag_var_tracking; > > > > can probably be handled by EnabledBy, but then we also have > > > > if (flag_var_tracking_uninit == 1) > > flag_var_tracking = 1; > > Yep, I made: > > fvar-tracking > Common Var(flag_var_tracking) PerFunction EnabledBy(fvar-tracking-uninit) > > and made fvar-tracking enabled with OPT_LEVELS_1_PLUS. > > > > > which suggests the same the other way around. I guess > > The other way around is problematic as leads to a cycle. I tried adding > a cycle detection in common_handle_option_auto (using a bitmap). But it breaks > case OPT_Wunused, which set sets various sub-values :/ > > > > positional handling might differ with say > > -fvar-tracking -fno-var-tracking-uninit vs. -fno-var-tracking-uninit > > -fvar-tracking > > I verified this is fine in debugger. > > > when using EnabledBy vs. the "explicit" code. > > > > + else if (!OPTION_SET_P (flag_var_tracking) && flag_var_tracking) > > flag_var_tracking = optimize >= 1; > > > > I think _this_ should, instead of the Init(1), become an entry in > > default_options_table with OPT_LEVELS_1_PLUS. > > Done. > > > > > As said, besides flag_var_* the posted patch looks OK and is good to > > commit. > > I'm sending an updated version that survives regression tests. > > Thoughts? OK. Thanks, Richard. > Martin > > > > > Richard. > > > >> > >> Martin
All right, and there's second part that moves the code from toplev.c to opts.c (finish_options) as I've done in the original version. The patch also handles PR102766 where nvptx.c target sets: debug_nonbind_markers_p = 0; So the easiest approach is marking the flag as set in global_options_set, I haven't found a better approach :/ Reason is that nvptx_option_override is called before finish_options. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
On Fri, Oct 15, 2021 at 5:22 PM Martin Liška <mliska@suse.cz> wrote: > > All right, and there's second part that moves the code > from toplev.c to opts.c (finish_options) as I've done in the original version. > > The patch also handles PR102766 where nvptx.c target sets: > debug_nonbind_markers_p = 0; > > So the easiest approach is marking the flag as set in global_options_set, > I haven't found a better approach :/ Reason is that nvptx_option_override > is called before finish_options. So currently nvptx_option_override is called before we do this code blob (it's called at the beginning of process_options). Why's the solution not to move this setting to finish_options as well? (and disabling it along var-tracking when we end with no -g in process_options) IMHO the target should have the last say, so the hook should be invoked after we are finished overriding stuff and finish_options should be _only_ doing diagnostics and disabling stuff we cannot handle. Richard. > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin
On 10/19/21 11:12, Richard Biener wrote: > On Fri, Oct 15, 2021 at 5:22 PM Martin Liška <mliska@suse.cz> wrote: >> >> All right, and there's second part that moves the code >> from toplev.c to opts.c (finish_options) as I've done in the original version. >> >> The patch also handles PR102766 where nvptx.c target sets: >> debug_nonbind_markers_p = 0; >> >> So the easiest approach is marking the flag as set in global_options_set, >> I haven't found a better approach :/ Reason is that nvptx_option_override >> is called before finish_options. > > So currently nvptx_option_override is called before we do this code > blob (it's called at the beginning of process_options). Yes, happens early in process_options. > > Why's the solution not to move this setting to finish_options as well? I would like to, but the option detection depends on target hooks and some debuginfo functionality, leading to: g++ -no-pie -g -DIN_GCC -fPIC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-error=format-diag -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -static-libstdc++ -static-libgcc -o Tlto-wrapper \ lto-wrapper.o collect-utils.o ggc-none.o libcommon-target.a libcommon.a ../libcpp/libcpp.a ../libbacktrace/.libs/libbacktrace.a ../libiberty/pic/libiberty.a ../libdecnumber/libdecnumber.a /home/marxin/Programming/gcc/gcc/opts.c:1382: error: undefined reference to 'dwarf2out_default_as_loc_support()' /home/marxin/Programming/gcc/gcc/opts.c:1384: error: undefined reference to 'dwarf2out_default_as_locview_support()' /home/marxin/Programming/gcc/gcc/opts.c:1409: error: undefined reference to 'targetm' Or can we do better? > (and disabling it along var-tracking when we end with no -g in process_options) > > IMHO the target should have the last say, so the hook should be invoked > after we are finished overriding stuff and finish_options should be > _only_ doing diagnostics and disabling stuff we cannot handle. Well, that sounds good, but we are quite far from that :/ Martin > > Richard. > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? >> Thanks, >> Martin
On Tue, Oct 19, 2021 at 11:34 AM Martin Liška <mliska@suse.cz> wrote: > > On 10/19/21 11:12, Richard Biener wrote: > > On Fri, Oct 15, 2021 at 5:22 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> All right, and there's second part that moves the code > >> from toplev.c to opts.c (finish_options) as I've done in the original version. > >> > >> The patch also handles PR102766 where nvptx.c target sets: > >> debug_nonbind_markers_p = 0; > >> > >> So the easiest approach is marking the flag as set in global_options_set, > >> I haven't found a better approach :/ Reason is that nvptx_option_override > >> is called before finish_options. > > > > So currently nvptx_option_override is called before we do this code > > blob (it's called at the beginning of process_options). > > Yes, happens early in process_options. > > > > > Why's the solution not to move this setting to finish_options as well? > > I would like to, but the option detection depends on target hooks and some debuginfo > functionality, leading to: > > g++ -no-pie -g -DIN_GCC -fPIC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-error=format-diag -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -static-libstdc++ -static-libgcc -o Tlto-wrapper \ > lto-wrapper.o collect-utils.o ggc-none.o libcommon-target.a libcommon.a ../libcpp/libcpp.a ../libbacktrace/.libs/libbacktrace.a ../libiberty/pic/libiberty.a ../libdecnumber/libdecnumber.a > /home/marxin/Programming/gcc/gcc/opts.c:1382: error: undefined reference to 'dwarf2out_default_as_loc_support()' > /home/marxin/Programming/gcc/gcc/opts.c:1384: error: undefined reference to 'dwarf2out_default_as_locview_support()' > /home/marxin/Programming/gcc/gcc/opts.c:1409: error: undefined reference to 'targetm' > > Or can we do better? Meh ... :/ Well, move the target override hook call down (try to shuffle things so diagnostics happen after but "inits" happen before). > > (and disabling it along var-tracking when we end with no -g in process_options) > > > > IMHO the target should have the last say, so the hook should be invoked > > after we are finished overriding stuff and finish_options should be > > _only_ doing diagnostics and disabling stuff we cannot handle. > > Well, that sounds good, but we are quite far from that :/ Yes, I know... > Martin > > > > > Richard. > > > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > >> > >> Ready to be installed? > >> Thanks, > >> Martin >
On 10/19/21 12:53, Richard Biener wrote: > Meh ... :/ > > Well, move the target override hook call down (try to shuffle things > so diagnostics happen after but > "inits" happen before). Not so easy. There are direct usages of the hooks (influences dwarf2out_as_loc_support and dwarf2out_as_locview_support) if (!OPTION_SET_P (dwarf2out_as_loc_support)) dwarf2out_as_loc_support = dwarf2out_default_as_loc_support (); if (!OPTION_SET_P (dwarf2out_as_locview_support)) dwarf2out_as_locview_support = dwarf2out_default_as_locview_support (); if (!OPTION_SET_P (debug_variable_location_views)) { debug_variable_location_views = (flag_var_tracking && debug_info_level >= DINFO_LEVEL_NORMAL && dwarf_debuginfo_p () && !dwarf_strict && dwarf2out_as_loc_support && dwarf2out_as_locview_support); } and then the warnings depend on debug_variable_location_views. I have another attempt which is about moving option detection of debug_nonbind_markers_p to finish_options. That works fine, except one needs to mark the option as PerFunction. That's because it depends on 'optimize' and that would trigger: 'global_options are modified in local context' verification error. What do you think about the patch? Cheers, Martin
On Wed, Oct 20, 2021 at 10:51 AM Martin Liška <mliska@suse.cz> wrote: > > On 10/19/21 12:53, Richard Biener wrote: > > Meh ... :/ > > > > Well, move the target override hook call down (try to shuffle things > > so diagnostics happen after but > > "inits" happen before). > > Not so easy. There are direct usages of the hooks > (influences dwarf2out_as_loc_support and dwarf2out_as_locview_support) > > if (!OPTION_SET_P (dwarf2out_as_loc_support)) > dwarf2out_as_loc_support = dwarf2out_default_as_loc_support (); > if (!OPTION_SET_P (dwarf2out_as_locview_support)) > dwarf2out_as_locview_support = dwarf2out_default_as_locview_support (); > > if (!OPTION_SET_P (debug_variable_location_views)) > { > debug_variable_location_views > = (flag_var_tracking > && debug_info_level >= DINFO_LEVEL_NORMAL > && dwarf_debuginfo_p () > && !dwarf_strict > && dwarf2out_as_loc_support > && dwarf2out_as_locview_support); > } > > and then the warnings depend on debug_variable_location_views. > > I have another attempt which is about moving option detection of debug_nonbind_markers_p > to finish_options. That works fine, except one needs to mark the option as PerFunction. > That's because it depends on 'optimize' and that would trigger: > 'global_options are modified in local context' verification error. That looks like a sensible change anyway - options that depend on options that are per function have to be per function as well. It also depends on flag_selective_scheduling. But note the checks now happen before if (flag_syntax_only) { write_symbols = NO_DEBUG; profile_flag = 0; } if (flag_gtoggle) { if (debug_info_level == DINFO_LEVEL_NONE) { debug_info_level = DINFO_LEVEL_NORMAL; if (write_symbols == NO_DEBUG) write_symbols = PREFERRED_DEBUGGING_TYPE; } else debug_info_level = DINFO_LEVEL_NONE; } which previously affected debug_nonbind_markers_p. I think it makes sense to move the above to finish_options as well. I suppose -help doesn't correctly dump the -g enabled state for -gtoggle at the moment? Richard. > > What do you think about the patch? > Cheers, > Martin
On 10/21/21 11:57, Richard Biener wrote: > which previously affected debug_nonbind_markers_p. I think it makes > sense to move > the above to finish_options as well. I suppose -help doesn't correctly dump > the -g enabled state for -gtoggle at the moment? All right, works for me. The updated patch was tested on nvptx cross compiler and can bootstrap on x86_64-linux-gnu and survives regression tests. The -g option is showed in -Q --help=common as: -g So no option value is displayed. Is the patch fine now? Thanks, Martin
On Thu, Oct 21, 2021 at 3:14 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/21/21 11:57, Richard Biener wrote: > > which previously affected debug_nonbind_markers_p. I think it makes > > sense to move > > the above to finish_options as well. I suppose -help doesn't correctly dump > > the -g enabled state for -gtoggle at the moment? > > All right, works for me. The updated patch was tested on nvptx cross compiler > and can bootstrap on x86_64-linux-gnu and survives regression tests. > > The -g option is showed in -Q --help=common as: > > -g > > So no option value is displayed. > > Is the patch fine now? OK. Thanks, Richard. > Thanks, > Martin
diff --git a/gcc/common.opt b/gcc/common.opt index 52693e226d2..ec020f4e642 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -3003,19 +3003,16 @@ Common Undocumented Var(flag_use_linker_plugin) ; Positive if we should track variables, negative if we should run ; the var-tracking pass only to discard debug annotations, zero if -; we're not to run it. When flag_var_tracking == 2 (AUTODETECT_VALUE) it -; will be set according to optimize, debug_info_level and debug_hooks -; in process_options (). +; we're not to run it. fvar-tracking -Common Var(flag_var_tracking) Init(2) PerFunction +Common Var(flag_var_tracking) PerFunction Perform variable tracking. ; Positive if we should track variables at assignments, negative if ; we should run the var-tracking pass only to discard debug -; annotations. When flag_var_tracking_assignments == -; AUTODETECT_VALUE it will be set according to flag_var_tracking. +; annotations. fvar-tracking-assignments -Common Var(flag_var_tracking_assignments) Init(2) PerFunction +Common Var(flag_var_tracking_assignments) PerFunction Perform variable tracking by annotating assignments. ; Nonzero if we should toggle flag_var_tracking_assignments after @@ -3026,8 +3023,7 @@ Toggle -fvar-tracking-assignments. ; Positive if we should track uninitialized variables, negative if ; we should run the var-tracking pass only to discard debug -; annotations. When flag_var_tracking_uninit == AUTODETECT_VALUE it -; will be set according to flag_var_tracking. +; annotations. fvar-tracking-uninit Common Var(flag_var_tracking_uninit) PerFunction Perform variable tracking and also tag variables that are uninitialized. diff --git a/gcc/opts.c b/gcc/opts.c index 2116c2991dd..eeb6b1dcc7c 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -1353,6 +1353,34 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, SET_OPTION_IF_UNSET (opts, opts_set, flag_vect_cost_model, VECT_COST_MODEL_CHEAP); + /* If the user specifically requested variable tracking with tagging + uninitialized variables, we need to turn on variable tracking. + (We already determined above that variable tracking is feasible.) */ + if (opts->x_flag_var_tracking_uninit == 1) + opts->x_flag_var_tracking = 1; + + if (!opts_set->x_flag_var_tracking) + opts->x_flag_var_tracking = optimize >= 1; + + if (!opts_set->x_flag_var_tracking_uninit) + opts->x_flag_var_tracking_uninit = opts->x_flag_var_tracking; + + if (!opts_set->x_flag_var_tracking_assignments) + opts->x_flag_var_tracking_assignments + = (opts->x_flag_var_tracking + && !(opts->x_flag_selective_scheduling + || opts->x_flag_selective_scheduling2)); + + if (opts->x_flag_var_tracking_assignments_toggle) + opts->x_flag_var_tracking_assignments = !opts->x_flag_var_tracking_assignments; + + if (opts->x_flag_var_tracking_assignments && !opts->x_flag_var_tracking) + opts->x_flag_var_tracking = opts->x_flag_var_tracking_assignments = -1; + + if (opts->x_flag_var_tracking_assignments + && (opts->x_flag_selective_scheduling || opts->x_flag_selective_scheduling2)) + warning_at (loc, 0, + "var-tracking-assignments changes selective scheduling"); } #define LEFT_COLUMN 27 diff --git a/gcc/testsuite/gcc.dg/pr102585.c b/gcc/testsuite/gcc.dg/pr102585.c new file mode 100644 index 00000000000..efd066b4a4e --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr102585.c @@ -0,0 +1,6 @@ +/* PR debug/102585 */ +/* { dg-do compile } */ +/* { dg-options "-fvar-tracking-assignments -fno-var-tracking" } */ + +#pragma GCC optimize 0 +void d_demangle_callback_Og() { int c = 0; } diff --git a/gcc/toplev.c b/gcc/toplev.c index 81748b1152a..2f13d740b98 100644 --- a/gcc/toplev.c +++ b/gcc/toplev.c @@ -1490,8 +1490,8 @@ process_options (bool no_backend) || !dwarf_debuginfo_p () || debug_hooks->var_location == do_nothing_debug_hooks.var_location) { - if (flag_var_tracking == 1 - || flag_var_tracking_uninit == 1) + if ((OPTION_SET_P (flag_var_tracking) && flag_var_tracking == 1) + || (OPTION_SET_P (flag_var_tracking_uninit) && flag_var_tracking_uninit == 1)) { if (debug_info_level < DINFO_LEVEL_NORMAL) warning_at (UNKNOWN_LOCATION, 0, @@ -1504,6 +1504,7 @@ process_options (bool no_backend) } flag_var_tracking = 0; flag_var_tracking_uninit = 0; + flag_var_tracking_assignments = 0; } /* The debug hooks are used to implement -fdump-go-spec because it @@ -1512,34 +1513,6 @@ process_options (bool no_backend) if (flag_dump_go_spec != NULL) debug_hooks = dump_go_spec_init (flag_dump_go_spec, debug_hooks); - /* If the user specifically requested variable tracking with tagging - uninitialized variables, we need to turn on variable tracking. - (We already determined above that variable tracking is feasible.) */ - if (flag_var_tracking_uninit == 1) - flag_var_tracking = 1; - - if (flag_var_tracking == AUTODETECT_VALUE) - flag_var_tracking = optimize >= 1; - - if (flag_var_tracking_uninit == AUTODETECT_VALUE) - flag_var_tracking_uninit = flag_var_tracking; - - if (flag_var_tracking_assignments == AUTODETECT_VALUE) - flag_var_tracking_assignments - = (flag_var_tracking - && !(flag_selective_scheduling || flag_selective_scheduling2)); - - if (flag_var_tracking_assignments_toggle) - flag_var_tracking_assignments = !flag_var_tracking_assignments; - - if (flag_var_tracking_assignments && !flag_var_tracking) - flag_var_tracking = flag_var_tracking_assignments = -1; - - if (flag_var_tracking_assignments - && (flag_selective_scheduling || flag_selective_scheduling2)) - warning_at (UNKNOWN_LOCATION, 0, - "var-tracking-assignments changes selective scheduling"); - if (debug_nonbind_markers_p == AUTODETECT_VALUE) debug_nonbind_markers_p = (optimize