diff mbox series

Record that -gtoggle is already used in gcc_options.

Message ID 435f9ee2-88c6-392e-3584-2337dd5dd9c1@suse.cz
State New
Headers show
Series Record that -gtoggle is already used in gcc_options. | expand

Commit Message

Martin Liška Nov. 2, 2021, 2:10 p.m. UTC 
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

When doing flip based on -gtoggle, record it. Otherwise, we will
apply it for the second time in finish_options.

	PR debug/102955

gcc/ChangeLog:

	* common.opt: Add new gtoggle_used variable.
	* opts.c (finish_options): Do not interpret flag_gtoggle twice.

gcc/testsuite/ChangeLog:

	* gcc.dg/pr102955.c: New test.
---
  gcc/common.opt                  |  4 ++++
  gcc/opts.c                      |  3 ++-
  gcc/testsuite/gcc.dg/pr102955.c | 14 ++++++++++++++
  3 files changed, 20 insertions(+), 1 deletion(-)
  create mode 100644 gcc/testsuite/gcc.dg/pr102955.c

Comments

Richard Biener Nov. 2, 2021, 2:33 p.m. UTC | #1 
On Tue, Nov 2, 2021 at 3:11 PM Martin Liška <mliska@suse.cz> wrote:
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

I think -gtoggle matches a Defered option and thus should be processed
in handle_common_deferred_options.  I'd argue that --help printing shouldn't
be part of decode_options but done only in toplev.c (not from the
parse_optimize_options caller).

Richard.

> Thanks,
> Martin
>
> When doing flip based on -gtoggle, record it. Otherwise, we will
> apply it for the second time in finish_options.
>
>         PR debug/102955
>
> gcc/ChangeLog:
>
>         * common.opt: Add new gtoggle_used variable.
>         * opts.c (finish_options): Do not interpret flag_gtoggle twice.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/pr102955.c: New test.
> ---
>   gcc/common.opt                  |  4 ++++
>   gcc/opts.c                      |  3 ++-
>   gcc/testsuite/gcc.dg/pr102955.c | 14 ++++++++++++++
>   3 files changed, 20 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.dg/pr102955.c
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 1a5b9bfcca9..2568ecb98b8 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -3316,6 +3316,10 @@ gdescribe-dies
>   Common Driver Var(flag_describe_dies) Init(0)
>   Add description attributes to some DWARF DIEs that have no name attribute.
>
> +; True if -gtoggle option was already handled.
> +Variable
> +bool gtoggle_used
> +
>   gtoggle
>   Common Driver Var(flag_gtoggle)
>   Toggle debug information generation.
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 3f80fce82bc..ef38b8dbab0 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1375,8 +1375,9 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
>         profile_flag = 0;
>       }
>
> -  if (flag_gtoggle)
> +  if (flag_gtoggle && !gtoggle_used)
>       {
> +      gtoggle_used = true;
>         if (debug_info_level == DINFO_LEVEL_NONE)
>         {
>           debug_info_level = DINFO_LEVEL_NORMAL;
> diff --git a/gcc/testsuite/gcc.dg/pr102955.c b/gcc/testsuite/gcc.dg/pr102955.c
> new file mode 100644
> index 00000000000..de9689edec4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr102955.c
> @@ -0,0 +1,14 @@
> +/* PR debug/102955 */
> +/* { dg-do compile } */
> +/* { dg-options "-g -gtoggle" } */
> +
> +#pragma GCC optimize "0"
> +struct j
> +{
> +  explicit j ();
> +  ~j ();
> +};
> +void g (void)
> +{
> +  new j();
> +}
> --
> 2.33.1
>
Martin Liška Nov. 2, 2021, 3:11 p.m. UTC | #2 
On 11/2/21 15:33, Richard Biener wrote:
> I think -gtoggle matches a Defered option and thus should be processed
> in handle_common_deferred_options.

Well, that's quite problematic as I handle_common_deferred_options is called
after decode_options (that calls finish_options).

Note there's direct dependency at very end of finish_options in between -gtoggle
and debug_nonbind_markers_p:


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

   if (!OPTION_SET_P (debug_nonbind_markers_p))
     debug_nonbind_markers_p
       = (optimize
	 && debug_info_level >= DINFO_LEVEL_NORMAL
	 && dwarf_debuginfo_p ()
	 && !(flag_selective_scheduling || flag_selective_scheduling2));

I don't see who you mean the possible fix?

Martin
Richard Biener Nov. 2, 2021, 4:45 p.m. UTC | #3 
On Tue, Nov 2, 2021 at 4:11 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 11/2/21 15:33, Richard Biener wrote:
> > I think -gtoggle matches a Defered option and thus should be processed
> > in handle_common_deferred_options.
>
> Well, that's quite problematic as I handle_common_deferred_options is called
> after decode_options (that calls finish_options).
>
> Note there's direct dependency at very end of finish_options in between -gtoggle
> and debug_nonbind_markers_p:
>
>
>    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;
>      }
>
>    if (!OPTION_SET_P (debug_nonbind_markers_p))
>      debug_nonbind_markers_p
>        = (optimize
>          && debug_info_level >= DINFO_LEVEL_NORMAL
>          && dwarf_debuginfo_p ()
>          && !(flag_selective_scheduling || flag_selective_scheduling2));
>
> I don't see who you mean the possible fix?

So at first I thought we might have a place that post-processes
'decoded_options' so we could reflect -gtoggle on those but
out-of-order (removing/adding -g).  But that's going to be mightly
complicated as well.

I wonder what the original issue is you fix?  You say we ap;ly
it for a second time but we should apply it onto the same
state as previously since we restore that for optimize attribute
processing?

Richard.

>
> Martin
Andrew Pinski Nov. 2, 2021, 10:49 p.m. UTC | #4 
On Tue, Nov 2, 2021 at 7:11 AM Martin Liška <mliska@suse.cz> wrote:
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> When doing flip based on -gtoggle, record it. Otherwise, we will
> apply it for the second time in finish_options.
>
>         PR debug/102955
>
> gcc/ChangeLog:
>
>         * common.opt: Add new gtoggle_used variable.
>         * opts.c (finish_options): Do not interpret flag_gtoggle twice.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/pr102955.c: New test.

Hmm, this is a C++ testcase so shouldn't it be at g++.dg/pr102955.C ?

Thanks,
Andrew Pinski

> ---
>   gcc/common.opt                  |  4 ++++
>   gcc/opts.c                      |  3 ++-
>   gcc/testsuite/gcc.dg/pr102955.c | 14 ++++++++++++++
>   3 files changed, 20 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.dg/pr102955.c
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 1a5b9bfcca9..2568ecb98b8 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -3316,6 +3316,10 @@ gdescribe-dies
>   Common Driver Var(flag_describe_dies) Init(0)
>   Add description attributes to some DWARF DIEs that have no name attribute.
>
> +; True if -gtoggle option was already handled.
> +Variable
> +bool gtoggle_used
> +
>   gtoggle
>   Common Driver Var(flag_gtoggle)
>   Toggle debug information generation.
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 3f80fce82bc..ef38b8dbab0 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1375,8 +1375,9 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
>         profile_flag = 0;
>       }
>
> -  if (flag_gtoggle)
> +  if (flag_gtoggle && !gtoggle_used)
>       {
> +      gtoggle_used = true;
>         if (debug_info_level == DINFO_LEVEL_NONE)
>         {
>           debug_info_level = DINFO_LEVEL_NORMAL;
> diff --git a/gcc/testsuite/gcc.dg/pr102955.c b/gcc/testsuite/gcc.dg/pr102955.c
> new file mode 100644
> index 00000000000..de9689edec4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr102955.c
> @@ -0,0 +1,14 @@
> +/* PR debug/102955 */
> +/* { dg-do compile } */
> +/* { dg-options "-g -gtoggle" } */
> +
> +#pragma GCC optimize "0"
> +struct j
> +{
> +  explicit j ();
> +  ~j ();
> +};
> +void g (void)
> +{
> +  new j();
> +}
> --
> 2.33.1
>
Martin Liška Nov. 4, 2021, 12:51 p.m. UTC | #5 
On 11/2/21 17:45, Richard Biener wrote:
> On Tue, Nov 2, 2021 at 4:11 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 11/2/21 15:33, Richard Biener wrote:
>>> I think -gtoggle matches a Defered option and thus should be processed
>>> in handle_common_deferred_options.
>>
>> Well, that's quite problematic as I handle_common_deferred_options is called
>> after decode_options (that calls finish_options).
>>
>> Note there's direct dependency at very end of finish_options in between -gtoggle
>> and debug_nonbind_markers_p:
>>
>>
>>     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;
>>       }
>>
>>     if (!OPTION_SET_P (debug_nonbind_markers_p))
>>       debug_nonbind_markers_p
>>         = (optimize
>>           && debug_info_level >= DINFO_LEVEL_NORMAL
>>           && dwarf_debuginfo_p ()
>>           && !(flag_selective_scheduling || flag_selective_scheduling2));
>>
>> I don't see who you mean the possible fix?
> 
> So at first I thought we might have a place that post-processes
> 'decoded_options' so we could reflect -gtoggle on those but
> out-of-order (removing/adding -g).  But that's going to be mightly
> complicated as well.

That would be very complicated.

> 
> I wonder what the original issue is you fix?  You say we ap;ly
> it for a second time but we should apply it onto the same
> state as previously since we restore that for optimize attribute
> processing?

Well, finish_options is always called once we parse options and we want to finalize them.
So that happens from toplev where we create initial global options. Later on, after the pragma
is parsed (where we start with current global options), the finish_options is called.

Problem of -gtoggle is that it does not directly influence an option, but it negates it.

That said, I think my patch with gtoggle_used is a reasonable workaround.

Cheers,
Martin

> 
> Richard.
> 
>>
>> Martin
Richard Biener Nov. 4, 2021, 1:09 p.m. UTC | #6 
On Thu, Nov 4, 2021 at 1:51 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 11/2/21 17:45, Richard Biener wrote:
> > On Tue, Nov 2, 2021 at 4:11 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 11/2/21 15:33, Richard Biener wrote:
> >>> I think -gtoggle matches a Defered option and thus should be processed
> >>> in handle_common_deferred_options.
> >>
> >> Well, that's quite problematic as I handle_common_deferred_options is called
> >> after decode_options (that calls finish_options).
> >>
> >> Note there's direct dependency at very end of finish_options in between -gtoggle
> >> and debug_nonbind_markers_p:
> >>
> >>
> >>     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;
> >>       }
> >>
> >>     if (!OPTION_SET_P (debug_nonbind_markers_p))
> >>       debug_nonbind_markers_p
> >>         = (optimize
> >>           && debug_info_level >= DINFO_LEVEL_NORMAL
> >>           && dwarf_debuginfo_p ()
> >>           && !(flag_selective_scheduling || flag_selective_scheduling2));
> >>
> >> I don't see who you mean the possible fix?
> >
> > So at first I thought we might have a place that post-processes
> > 'decoded_options' so we could reflect -gtoggle on those but
> > out-of-order (removing/adding -g).  But that's going to be mightly
> > complicated as well.
>
> That would be very complicated.
>
> >
> > I wonder what the original issue is you fix?  You say we ap;ly
> > it for a second time but we should apply it onto the same
> > state as previously since we restore that for optimize attribute
> > processing?
>
> Well, finish_options is always called once we parse options and we want to finalize them.
> So that happens from toplev where we create initial global options. Later on, after the pragma
> is parsed (where we start with current global options), the finish_options is called.

But we shouldn't start with the current global options but with ones
we saved for
optimize attribute/pragma processing, no?

> Problem of -gtoggle is that it does not directly influence an option, but it negates it.
>
> That said, I think my patch with gtoggle_used is a reasonable workaround.

Well, then we could as well unset flag_gtoggle after processing it, no?

Thanks,
Richard.

> Cheers,
> Martin
>
> >
> > Richard.
> >
> >>
> >> Martin
>
Martin Liška Nov. 4, 2021, 3:11 p.m. UTC | #7 
On 11/4/21 14:09, Richard Biener wrote:
> But we shouldn't start with the current global options but with ones
> we saved for
> optimize attribute/pragma processing, no?

We hit the issue when we combine cmdline and pragma optimize options.

> 
>> Problem of -gtoggle is that it does not directly influence an option, but it negates it.
>>
>> That said, I think my patch with gtoggle_used is a reasonable workaround.
> Well, then we could as well unset flag_gtoggle after processing it, no?

Yeah, that work! :)

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin
Richard Biener Nov. 5, 2021, 10:23 a.m. UTC | #8 
On Thu, Nov 4, 2021 at 4:11 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 11/4/21 14:09, Richard Biener wrote:
> > But we shouldn't start with the current global options but with ones
> > we saved for
> > optimize attribute/pragma processing, no?
>
> We hit the issue when we combine cmdline and pragma optimize options.
>
> >
> >> Problem of -gtoggle is that it does not directly influence an option, but it negates it.
> >>
> >> That said, I think my patch with gtoggle_used is a reasonable workaround.
> > Well, then we could as well unset flag_gtoggle after processing it, no?
>
> Yeah, that work! :)
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?

OK if you add a comment like

 /* Make sure to process -gtoggle only once.  */

Richard.

> Thanks,
> Martin
Martin Liška Nov. 5, 2021, 12:01 p.m. UTC | #9 
On 11/5/21 11:23, Richard Biener wrote:
> OK if you add a comment like
> 
>   /* Make sure to process -gtoggle only once.  */

Sure, added and installed as 14c7041a1f00ef4ee9a036e0b369c97646db5b5c.

Cheers,
Martin
diff mbox series

Patch

diff --git a/gcc/common.opt b/gcc/common.opt
index 1a5b9bfcca9..2568ecb98b8 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -3316,6 +3316,10 @@  gdescribe-dies
  Common Driver Var(flag_describe_dies) Init(0)
  Add description attributes to some DWARF DIEs that have no name attribute.
  
+; True if -gtoggle option was already handled.
+Variable
+bool gtoggle_used
+
  gtoggle
  Common Driver Var(flag_gtoggle)
  Toggle debug information generation.
diff --git a/gcc/opts.c b/gcc/opts.c
index 3f80fce82bc..ef38b8dbab0 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1375,8 +1375,9 @@  finish_options (struct gcc_options *opts, struct gcc_options *opts_set,
        profile_flag = 0;
      }
  
-  if (flag_gtoggle)
+  if (flag_gtoggle && !gtoggle_used)
      {
+      gtoggle_used = true;
        if (debug_info_level == DINFO_LEVEL_NONE)
  	{
  	  debug_info_level = DINFO_LEVEL_NORMAL;
diff --git a/gcc/testsuite/gcc.dg/pr102955.c b/gcc/testsuite/gcc.dg/pr102955.c
new file mode 100644
index 00000000000..de9689edec4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr102955.c
@@ -0,0 +1,14 @@ 
+/* PR debug/102955 */
+/* { dg-do compile } */
+/* { dg-options "-g -gtoggle" } */
+
+#pragma GCC optimize "0"
+struct j
+{
+  explicit j ();
+  ~j ();
+};
+void g (void)
+{
+  new j();
+}